NotificationsYou must be signed in to change notification settings
Fork32k
Star67.2k

gh-102780: Fix uncancel() call in asyncio timeouts#102815

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

gvanrossum merged 13 commits intopython:mainfrommainframeindustries:kristjan/uncancel

Mar 22, 2023

Merged

gh-102780: Fix uncancel() call in asyncio timeouts#102815

gvanrossum merged 13 commits intopython:mainfrommainframeindustries:kristjan/uncancel

Mar 22, 2023

Conversation

Copy link

Contributor

kristjanvalur commentedMar 18, 2023•
edited
Loading

Record the previous cancellation state when enteringasyncio.timeout.Timeout
This allows us to useasyncio.timeout even when handling aCancelledError
exception. TheTimeout context manager will still not handle a CancelledError
if anew cancellation request is delivered in its scope.

Fix Timeout context manager
Add regression tests
Add documentiation, clarifying when it is appropriate to callTask.uncancel()

This needs to be backported to 3.11

Issue:Catching CancelledError requires use of task.uncancel() #102780

kristjanvalur added4 commits

March 18, 2023 09:46

document use of uncancel() when suppressing CancelledError

9f96cea

Create unittest

07517b7

record previous cancel state in timeout context manager

1150db0

explicitly raise from cancelled error

d207fb1

kristjanvalur requested review from1st1,asvetlov,gvanrossum andkumaraditya303 ascode owners

March 18, 2023 13:57

bedevere-bot mentioned this pull request

Mar 18, 2023

Catching CancelledError requires use of task.uncancel()#102780

Closed

bedevere-bot added the awaiting review label

Mar 18, 2023

Copy link

Member

gvanrossum commentedMar 18, 2023

Thanks! This looks like the crux of the matter, and beautifully executed. I'll properly review it later this weekend.

gvanrossum approved these changes

Mar 19, 2023

View reviewed changes

Copy link

Member

gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Doc/library/asyncio-task.rstShow resolvedHide resolved

Doc/library/asyncio-task.rst OutdatedShow resolvedHide resolved

Lib/asyncio/timeouts.py

		# handling this.
		raise TimeoutError
		raise TimeoutError from exc_val

Copy link

Member

gvanrossumMar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.)

Copy link

Contributor

kumaraditya303Mar 19, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Isn't this done by default? I see the same traceback with and without this on this example:

importasyncioasyncdeffunc():awaitasyncio.sleep(1)asyncdefmain():asyncwithasyncio.timeout(0.1):awaitfunc()asyncio.run(main())

Copy link

Member

gvanrossumMar 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not quite. When usingraise exc from ... theCancelledError instance gets stored inexc.__cause__, while without it, it gets stored inexc.__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__.

Copy link

ContributorAuthor

kristjanvalurMar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 :)

Copy link

ContributorAuthor

kristjanvalurMar 20, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Contributor

kumaraditya303Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

kristjanvalurMar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'll add that in a few hours.

bedevere-bot added awaiting merge and removed awaiting review labels

Mar 19, 2023

kristjanvalurand others added2 commits

March 20, 2023 11:10

Update Doc/library/asyncio-task.rst

912f26d

Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>

Clarify how code should "ignore" CancelledError

ee574fc

kumaraditya303 approved these changes

Mar 20, 2023

View reviewed changes

Copy link

Contributor

kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 reviewed

Mar 20, 2023

View reviewed changes

Doc/library/asyncio-task.rst OutdatedShow resolvedHide resolved

AlexWaygood reviewed

Mar 20, 2023

View reviewed changes

Doc/library/asyncio-task.rst OutdatedShow resolvedHide resolved

kristjanvalurand others added3 commits

March 20, 2023 14:01

Apply suggestions from code review

c91dae7

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Add unittest for asyncio.TimeoutError.__cause__

902691e

Merge branch 'main' into kristjan/uncancel

0f7bde5

AlexWaygood reviewed

Mar 21, 2023

View reviewed changes

Copy link

Member

AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The docs read okay to me now, but I'll let Guido or Kumar merge since I'm not an asyncio expert!

gvanrossum reviewed

Mar 22, 2023

View reviewed changes

Copy link

Member

gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Needs a news entry, otherwise LGTM.

Copy link

ContributorAuthor

kristjanvalur commentedMar 22, 2023

Needs a news entry, otherwise LGTM.

I'll have a go at it.

kristjanvalur added2 commits

March 22, 2023 16:15

add news

07c40cc

Merge branch 'main' into kristjan/uncancel

2a8914e

AlexWaygood reviewed

Mar 22, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-03-22-16-15-18.bpo-102780.NEcljy.rst OutdatedShow resolvedHide resolved

Rename news file

a6f3114

AlexWaygood reviewed

Mar 22, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-03-22-16-15-18.gh-issue-102780.NEcljy.rst OutdatedShow resolvedHide resolved

fix news entry

6e299fa

gvanrossum approved these changes

Mar 22, 2023

View reviewed changes

gvanrossum changed the title~~gh-102780~~gh-102780: Fix uncancel() call in asyncio timeouts

Mar 22, 2023

gvanrossum merged commit04adf2d intopython:main

Mar 22, 2023

bedevere-bot removed the awaiting merge label

Mar 22, 2023

gvanrossum added the needs backport to 3.11only security fixes label

Mar 22, 2023

Copy link

Contributor

miss-islington commentedMar 22, 2023

Thanks@kristjanvalur for the PR, and@gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Mar 22, 2023

pythongh-102780: Fix uncancel() call in asyncio timeouts (pythonGH-10…

0314b21

…2815)Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is setin the `__cause__` field rather than in the `__context__` field.(cherry picked from commit04adf2d)Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com>Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Copy link

bedevere-bot commentedMar 22, 2023

GH-102923 is a backport of this pull request to the3.11 branch.

bedevere-bot removed the needs backport to 3.11only security fixes label

Mar 22, 2023

miss-islington added a commit that referenced this pull request

Mar 22, 2023

gh-102780: Fix uncancel() call in asyncio timeouts (GH-102815)

a9ece4a

Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is setin the `__cause__` field rather than in the `__context__` field.(cherry picked from commit04adf2d)Co-authored-by: Kristján Valur Jónsson <sweskman@gmail.com>Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

kristjanvalur deleted the kristjan/uncancel branch

March 23, 2023 08:48

bellini666 mentioned this pull request

Mar 23, 2023

Newasyncio.exceptions.CancelledError from 4.5.2redis/redis-py#2633

Closed

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request

Mar 27, 2023

pythongh-102780: Fix uncancel() call in asyncio timeouts (python#102815)

4080fb7

Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is setin the `__cause__` field rather than in the `__context__` field.Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

warsaw pushed a commit to warsaw/cpython that referenced this pull request

Apr 11, 2023

pythongh-102780: Fix uncancel() call in asyncio timeouts (python#102815)

bc2e444

Also use `raise TimeOut from <CancelledError instance>` so that the CancelledError is setin the `__cause__` field rather than in the `__context__` field.Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

kristjanvalur mentioned this pull request

Apr 24, 2023

Optionally disable disconnects in read_responseredis/redis-py#2695

Merged

6 tasks

Copy link

Contributor

graingert commentedAug 15, 2023

does this need a corresponding change in asyncio.TaskGroup? eg here

cpython/Lib/asyncio/taskgroups.py

Line 82 ine90036c

ifself._parent_task.uncancel()==0:

Copy link

ContributorAuthor

kristjanvalur commentedAug 15, 2023

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.