Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cancelled operation triggering asyncio uncaught exception handler #21

Merged
merged 2 commits into from
Aug 13, 2022

Conversation

h4l
Copy link
Contributor

@h4l h4l commented Aug 13, 2022

The asyncio uncaught exception handler can be triggered when operations are cancelled, causing an error to be printed to stderr.

AsyncioContextBase._on_done() schedules a cross-thread call to Future.set_result, but doesn't check that the future is still not done before the call later runs on the asyncio thread. This allows for an operation to be cancelled in between the initial done() check and the set_result() call, which then fails, triggering the uncaught exception handler.

This PR fixes this by re-checking if the Future is done before calling set_result().

I can imagine you don't necessarily need a test for this, as it's a deterministic thing, but putting in a test was the easiest way to reproduce/demonstrate the issue. You can see it failing for the current version by checking out the test commit and running:

$ pytest tests/test_asyncio_adapter.py::test_operations_cancel_cleanly

You should see the test fail, and the captured output to stderr will contain errors logged by the asyncio uncaught exception handler like this:

Exception in callback Future.set_result(True)
handle: <Handle Future.set_result(True)>
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/asyncio/events.py", line 80, in _run
    self._context.run(self._callback, *self._args)
asyncio.exceptions.InvalidStateError: invalid state

I'm getting these kind of errors logged by my app due to this (I'm doing something similar to the test here, reading a bunch of chunks from a file concurrently/eagerly, and at some point deciding to cancel the remaining read operations.

h4l added 2 commits August 13, 2022 14:57
This currently fails due to a AsyncioContextBase._on_done() calling
set_result() on a future that's already done. This triggers the asyncio
exception handler.
The asyncio uncaught exception handler can be triggered when operations
are cancelled, causing an error to be printed to stderr.

AsyncioContextBase._on_done() schedules a cross-thread call to
Future.set_result, but doesn't check that the future is still not done
before the call later runs on the asyncio thread. This allows for an
operation to be cancelled in between the initial done() check and the
set_result() call, which then fails, triggering the uncaught exception
handler.

Now the callback re-checks if the Future is done before calling
set_result().
@mosquito mosquito merged commit d3becc5 into mosquito:master Aug 13, 2022
@mosquito
Copy link
Owner

Will be released soon.

@h4l
Copy link
Contributor Author

h4l commented Aug 13, 2022

Great, thanks!

@mosquito
Copy link
Owner

@h4l this fix included in 0.9.8 release

@h4l
Copy link
Contributor Author

h4l commented Aug 14, 2022

Fantastic, thanks for releasing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants