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.

Sign up for GitHub

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-124958: fix asyncio.TaskGroup and _PyFuture refcycles #124959

Merged
merged 4 commits into from
Oct 14, 2024
Merged

gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles #124959

merged 4 commits into from
Oct 14, 2024

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Oct 4, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Oct 4, 2024
Closed
@graingert graingert marked this pull request as ready for review October 4, 2024 10:51
1st1
1st1 reviewed Oct 4, 2024
View reviewed changes
Lib/asyncio/taskgroups.py
@@ -66,94 +66,103 @@ async def __aenter__(self):
return self

async def __aexit__(self, et, exc, tb):
self._exiting = True
try:
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this huge diff. Can you instead factor out the code of __aexit__ in a helper function (so that code keeps all of the existing indentation) and nest a call to it in try..finally in __aexit__?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to delete locals as well as instance attributes so it's not possible

Copy link
Member

Choose a reason for hiding this comment

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

I think it is. Instance attributes are not a problem at all. Locals are solvable too:

try:
    raise propagate_cancellation_error
finally:
    del propagate_cancellation_error

Copy link
Contributor Author

@graingert graingert Oct 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I pushed the changes but it introduces a bunch of extra places to delete things, so I prefer the one big try/finally. You can also just view the diff with whitespace hidden: 1a4f2b5?w=1 which should look like this:
image

@graingert graingert requested a review from 1st1 October 8, 2024 08:42
1st1
1st1 approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

lgtm, sorry for being a bit of a PITA with this review, but I think this version of the PR does read better by having most of the cleanup code more visible.

@graingert graingert added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Oct 8, 2024
@graingert graingert requested a review from 1st1 October 13, 2024 08:26
@1st1 1st1 merged commit d5dbbf4 into python:main Oct 14, 2024
38 checks passed
@miss-islington-app
Copy link

Thanks @graingert for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

@graingert graingert deleted the fix-tg-refcycles branch October 14, 2024 15:20
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 14, 2024
@graingert @miss-islington
pythongh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (pytho…
e754fd8 
…nGH-124959)

(cherry picked from commit d5dbbf4)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
@1st1
Copy link
Member

1st1 commented Oct 14, 2024

Thank you Thomas!

@miss-islington-app
Copy link

Sorry, @graingert and @1st1, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker d5dbbf4372cd3dbf3eead1cc70ddc4261c061fd9 3.12

@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2024

GH-125463 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 14, 2024
@1st1
Copy link
Member

1st1 commented Oct 14, 2024

@graingert Please submit a separate PR to 3.12, albeit I'm not sure we should port it to 3.12.

@bedevere-app
Copy link

bedevere-app bot commented Oct 14, 2024

GH-125466 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 14, 2024
1st1 pushed a commit that referenced this pull request Oct 14, 2024
@miss-islington @graingert
[3.13] gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (GH-1…
4e08c98 
…24959) (#125463)

gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (GH-124959)
(cherry picked from commit d5dbbf4)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Eclips4 added a commit to Eclips4/cpython that referenced this pull request Oct 14, 2024
@Eclips4
Revert "pythongh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (
b8cc4ca 
python#124959)"

This reverts commit d5dbbf4.
kumaraditya303 pushed a commit that referenced this pull request Oct 14, 2024
@Eclips4
gh-125472: Revert "gh-124958: fix asyncio.TaskGroup and _PyFuture ref…
e99650b 
…cycles (#12… (#125476)

Revert "gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (#124959)"

This reverts commit d5dbbf4.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 14, 2024
@Eclips4 @miss-islington
pythongh-125472: Revert "pythongh-124958: fix asyncio.TaskGroup and _…
874bfad 
…PyFuture refcycles (pythonGH-12… (pythonGH-125476)

Revert "pythongh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (pythonGH-124959)"

This reverts commit d5dbbf4.
(cherry picked from commit e99650b)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
@miss-islington miss-islington mentioned this pull request Oct 14, 2024
Closed
1st1 pushed a commit that referenced this pull request Oct 17, 2024
@graingert
[3.12] gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (#124959
32d4579 
) (#125466)

gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (#124959)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1st1 1st1 1st1 approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@gvanrossum gvanrossum Awaiting requested review from gvanrossum gvanrossum is a code owner

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

Assignees

@1st1 1st1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@graingert @1st1