-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-40607: Reraise exception during task cancelation in asyncio.wait_for() #20054
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -496,7 +496,15 @@ async def wait_for(fut, timeout, *, loop=None): | |
# after wait_for() returns. | ||
# See https://bugs.python.org/issue32751 | ||
await _cancel_and_wait(fut, loop=loop) | ||
raise exceptions.TimeoutError() | ||
# In case task cancellation failed with some | ||
# exception, we should re-raise it | ||
# See https://bugs.python.org/issue40607 | ||
try: | ||
fut.result() | ||
except exceptions.CancelledError as exc: | ||
raise exceptions.TimeoutError() from exc | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd put also an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, but I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but your new code is simply not equivalent to the previous code that raised TimeoutError unconditionally. Now I see that there might be a condition for clear return and I don't want that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The linked issue talks about the correct exception propagation. Basically, The code here is slightly different though and a bit hard to trace so it would be easier to read if it was replaced with: try:
fut.result()
except exceptions.CancelledError as exc:
raise exceptions.TimeoutError() from exc
else:
raise exceptions.TimeoutError() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the issue here is that there is a race condition where the task can complete after the timeout has elapsed. So the question is whether the caller should get to learn that the task completed in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the current implementation discards the result, as the probability of actually having it is low. I'd like to keep it that way. If we want to change the current behavior that should be done in a separate issue, with a new PR and new discussion. |
||
else: | ||
raise exceptions.TimeoutError() | ||
finally: | ||
timeout_handle.cancel() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
When cancelling a task due to timeout, :meth:`asyncio.wait_for` will now | ||
propagate the exception if an error happens during cancellation. | ||
Patch by Roman Skurikhin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the case when task cancellation failed with some result (not exception)? Is it OK to ignore it? I mean something like