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

Jump to bottom

GH-95289: Always call uncancel() when parent cancellation is requested #95602

Merged
merged 4 commits into from Aug 4, 2022
Merged

GH-95289: Always call uncancel() when parent cancellation is requested #95602

merged 4 commits into from Aug 4, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Aug 3, 2022 •
edited

@kumaraditya303 kumaraditya303 added type-bug An unexpected behavior, bug, or error release-blocker needs backport to 3.11 labels Aug 3, 2022
@kumaraditya303 kumaraditya303 mentioned this pull request Aug 3, 2022
Closed
@kumaraditya303 kumaraditya303 requested a review from gvanrossum Aug 3, 2022
graingert
graingert requested changes Aug 3, 2022
View changes
Lib/asyncio/taskgroups.py Show resolved Hide resolved
gvanrossum
gvanrossum reviewed Aug 3, 2022
View changes
Lib/test/test_asyncio/test_taskgroups.py Outdated Show resolved Hide resolved
Lib/asyncio/taskgroups.py Show resolved Hide resolved
@gvanrossum
Copy link
Member

gvanrossum commented Aug 4, 2022

We're getting very close to the RC1 release date (this Friday!). I'd like to get this in. @graingert can you bless this as strictly better than what we had before?

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Aug 4, 2022

We're getting very close to the RC1 release date (this Friday!). I'd like to get this in

I agree, I would like to get this in ASAP.

@graingert
Copy link
Contributor

graingert commented Aug 4, 2022 •
edited

We're getting very close to the RC1 release date (this Friday!). I'd like to get this in. @graingert can you bless this as strictly better than what we had before?

This now follows the five (six? Cc @ambv) rules of uncancellation so I think therefore it's strictly better. However I can't say for sure. It looks to me like TaskGroup was relying on task.cancelling() >= 1 traveling up the stack to communicate with parent scopes without anticipating that it also travels down the rest of the function body.

@graingert
Copy link
Contributor

graingert commented Aug 4, 2022 •
edited

In addition I think there's other more fundamental problems with uncancel, for example when a cancellation is delivered from a chained concurrent.fututes.Future or when a TaskGroup task waits on another TaskGroup task and cancellation is chained between them

@gvanrossum gvanrossum merged commit 2fef275 into python:main Aug 4, 2022
14 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Aug 4, 2022

Thanks @kumaraditya303 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 Aug 4, 2022
@kumaraditya303 @miss-islington
pythonGH-95289: Always call uncancel() when parent cancellation is re…
fb5fe45 
…quested (pythonGH-95602)

Co-authored-by: Guido van Rossum <guido@python.org>
(cherry picked from commit 2fef275)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot
Copy link

bedevere-bot commented Aug 4, 2022

GH-95654 is a backport of this pull request to the 3.11 branch.

miss-islington added a commit that referenced this pull request Aug 4, 2022
@miss-islington @kumaraditya303
GH-95289: Always call uncancel() when parent cancellation is requested (
2d84fe5 
GH-95602)

Co-authored-by: Guido van Rossum <guido@python.org>
(cherry picked from commit 2fef275)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@kumaraditya303 kumaraditya303 deleted the fix-taskgroup-uncancel branch Aug 4, 2022
@kumaraditya303 kumaraditya303 mentioned this pull request Aug 4, 2022
Closed
@gvanrossum gvanrossum mentioned this pull request Aug 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graingert graingert

@gvanrossum gvanrossum

@1st1 1st1

@asvetlov asvetlov

Assignees

@gvanrossum gvanrossum

Labels
release-blocker type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kumaraditya303 @gvanrossum @graingert @miss-islington @bedevere-bot