also abort connections on CancelledError#6574
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ± 0 15 suites ±0 6h 37m 40s ⏱️ + 26m 40s For more details on these failures and errors, see this check. Results for commit b20932d. ± Comparison against base commit 344868a. ♻️ This comment has been updated with latest results. |
fjetter
left a comment
There was a problem hiding this comment.
Makes a lot of sense. Do you see an easy way to test this? I think I wouldn't want to go through a sophisticated mocking/faking exercise just to test this exception handler, though.
FWIW we introduced this abort in #4239 and added test_comm_closed_on_buffer_error to test this which does patch the stream write. Better than nothing?
|
maybe you're on to something :) |
|
You can do an |
| # was already read from the underlying socket, so it is not even safe to retry | ||
| # here using the same stream. The only safe thing to do is to abort. | ||
| # (See also GitHub #4133). | ||
| except BaseException: |
There was a problem hiding this comment.
when #4133 was introduced CancelledError was still a subclass of Exception since 3.8 CancelledError is no-longer caught here
We may want to fix TCP only at first and delay everything else until it becomes an actual problem (and if possible asyncio_tcp). The other backends are used very rarely. Fixing this for TCP already would be a big win |
|
I added a commit that adds a test and changes the exception handling in the write path to handle BaseExceptions as well. I can easily see a KeyboardInterrupt happening during write that is handled upstream, e.g. in a client jupyter session, and we wouldn't want the stream to be corrupted. |
This change will ensure `CancelledError`s are catched upon shutting down the Dask cluster, which may otherwise raise various errors. See also dask#6574 .
refs #4133, #6548
pre-commit run --all-files