Fix deadlock caused by an erred task (executing->cancelled->error) #5503
Conversation
| msg = error_message(e) | ||
| for k in self.in_flight_workers[worker]: | ||
| ts = self.tasks[k] | ||
| recommendations[ts] = tuple(msg.values()) | ||
| raise |
There was a problem hiding this comment.
This unhandled exception was also capable of causing a deadlock. That's also fixed now
| try: | ||
| self.data[ts.key] = value | ||
| except Exception as e: | ||
| msg = error_message(e) | ||
| ts.exception = msg["exception"] | ||
| ts.traceback = msg["traceback"] | ||
| recommendations[ts] = ("error", msg["exception"], msg["traceback"]) | ||
| return recommendations, [] | ||
| self.data[ts.key] = value |
There was a problem hiding this comment.
Exception handling is now handled by the caller. That's ugly but everything else would likely end up more ugly even. Reasoning explained in the above doc string
There was a problem hiding this comment.
IMHO, the buffer should never raise and instead handle this gracefully by not spilling. Apart from being more work, that's also a change of behaviour and an API question which is why I didn't feel comfortable with mixing such a change up in here
| @@ -2488,70 +2559,69 @@ def ensure_communicating(self): | |||
| for el in skipped_worker_in_flight: | |||
| heapq.heappush(self.data_needed, el) | |||
|
|
|||
| def get_task_state_for_scheduler(self, ts): | |||
There was a problem hiding this comment.
This function was awkward after the refactoring. Instead, there is now a single place where the error message is generated and the task-finished is factored into a dedicated private function. Ultimately, we also should have a single place were this is generated but that's currently not possible without more work
There was a problem hiding this comment.
Offline @fjetter mentioned he is confident in the changes here and would be good to include them in the upcoming release (xref dask/community#206). To that end, I'm planning to merge this PR in a bit if there are no further comments
|
The two new tests are failing in main |
|
Thanks for reporting @crusaderky -- tracking over in #5527 |
This fixes a deadlock caused by an erred task not freeing slots on the threadpool
Supersedes #5501 and #5500
Closes #5497
Most notable change to the other proposed solutions is that this includes tests. Writing the tests also revealed problems around how
_put_key_in_memoryis implemented leading me to factor the exception handling out. Mid-/Long term I believe this should be addressed by changing the zict buffer to not raise in case of spillage/serialization error. It could instead simply store the item in memory even if it is too large. However, that's way beyond the scope of this fix.