gh-102780: Fix uncancel() call in asyncio timeouts#102815
gh-102780: Fix uncancel() call in asyncio timeouts#102815gvanrossum merged 13 commits intopython:mainfrom mainframeindustries:kristjan/uncancel
Conversation
|
Thanks! This looks like the crux of the matter, and beautifully executed. I'll properly review it later this weekend. |
gvanrossum
left a comment
There was a problem hiding this comment.
I only really have doc nits, plus a feeling that maybe possibly TaskGroup could have the same problem?
I'm asking @kumaraditya303 to also review this, to make sure I didn't miss anything.
| # Since there are no new cancel requests, we're | ||
| # handling this. | ||
| raise TimeoutError | ||
| raise TimeoutError from exc_val |
There was a problem hiding this comment.
This is a nice improvement that maybe ought to be called out in the commit message? (If you agree I can handle that when I merge it.)
There was a problem hiding this comment.
Isn't this done by default? I see the same traceback with and without this on this example:
import asyncio
async def func():
await asyncio.sleep(1)
async def main():
async with asyncio.timeout(0.1):
await func()
asyncio.run(main())There was a problem hiding this comment.
Not quite. When using raise exc from ... the CancelledError instance gets stored in exc.__cause__, while without it, it gets stored in exc.__context__, and the connecting phrase in the output is different:
__cause__:
The above exception was the direct cause of the following exception:
__context__:
During handling of the above exception, another exception occurred:
(If both are set it follows __cause__.
There was a problem hiding this comment.
Precicely. I has bothered me for a while that timeouts happen with the latter message, as if somehow, there was an error during exception handling. I wanted to use the opportunity to roll that fix in :)
There was a problem hiding this comment.
This is a nice improvement that maybe ought to be called out in the commit message? (If you agree I can handle that when I merge it.)
Thank you, agreed.
There was a problem hiding this comment.
Not quite. When using raise exc from ... the CancelledError instance gets stored in exc.cause, while without it, it gets stored in exc.context, and the connecting phrase in the output is different:
Ah, right! I missed that. Adding a test would be nice.
There was a problem hiding this comment.
I'll add that in a few hours.
Co-authored-by: Guido van Rossum <[email protected]>
kumaraditya303
left a comment
There was a problem hiding this comment.
I would like to have a test for the exception chaining else LGTM. I leave the docs for Guido to review.
@gvanrossum Feel free to merge as you like.
AlexWaygood
left a comment
There was a problem hiding this comment.
The docs read okay to me now, but I'll let Guido or Kumar merge since I'm not an asyncio expert!
gvanrossum
left a comment
There was a problem hiding this comment.
Needs a news entry, otherwise LGTM.
I'll have a go at it. |
Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst
Outdated
Show resolved
Hide resolved
|
Thanks @kristjanvalur for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
…2815) Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is set in the `__cause__` field rather than in the `__context__` field. (cherry picked from commit 04adf2d) Co-authored-by: Kristján Valur Jónsson <[email protected]> Co-authored-by: Guido van Rossum <[email protected]> Co-authored-by: Alex Waygood <[email protected]>
|
GH-102923 is a backport of this pull request to the 3.11 branch. |
Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is set in the `__cause__` field rather than in the `__context__` field. (cherry picked from commit 04adf2d) Co-authored-by: Kristján Valur Jónsson <[email protected]> Co-authored-by: Guido van Rossum <[email protected]> Co-authored-by: Alex Waygood <[email protected]>
Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is set in the `__cause__` field rather than in the `__context__` field. Co-authored-by: Guido van Rossum <[email protected]> Co-authored-by: Alex Waygood <[email protected]>
Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is set in the `__cause__` field rather than in the `__context__` field. Co-authored-by: Guido van Rossum <[email protected]> Co-authored-by: Alex Waygood <[email protected]>
|
does this need a corresponding change in asyncio.TaskGroup? eg here cpython/Lib/asyncio/taskgroups.py Line 82 in e90036c |
|
Interesting. It would be an issue if we entered a TaskGroup while cancellation state was not 0, e.g. during a finally of an outer cancel. I'm not qualified, though, to judge if that would ever be a real issue. |
Record the previous cancellation state when entering
asyncio.timeout.TimeoutThis allows us to use
asyncio.timeouteven when handling aCancelledErrorexception. The
Timeoutcontext manager will still not handle a CancelledErrorif a new cancellation request is delivered in its scope.
Task.uncancel()This needs to be backported to 3.11