GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles by graingert · Pull Request #124959 · python/cpython

Conversation

@graingert

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: refcycles in exceptions raised from asyncio.TaskGroup #124958

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@graingert
pythongh-124958: fix asyncio.TaskGroup and _PyFuture refcycles
1a4f2b5
@bedevere-app bedevere-app Bot mentioned this pull request Oct 4, 2024
refcycles in exceptions raised from asyncio.TaskGroup #124958
Closed
graingert
graingert commented Oct 4, 2024
View reviewed changes
Comment thread Lib/test/test_asyncio/test_taskgroups.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
graingert and others added 2 commits October 4, 2024 09:43
@graingert
remove detrititus from testing PyFuture/PyTask
33ddc70
@blurb-it
📜🤖 Added by blurb_it.
5c5c057
@graingert
graingert marked this pull request as ready for review October 4, 2024 10:51
@graingert
graingert requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners October 4, 2024 10:51
@bedevere-app bedevere-app Bot added the awaiting review label Oct 4, 2024
1st1
1st1 reviewed Oct 4, 2024
View reviewed changes
Comment thread Lib/asyncio/taskgroups.py

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

@1st1 1st1 Oct 4, 2024

Copy link
Copy Markdown
Member

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.

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__?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@graingert graingert Oct 4, 2024

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@1st1 1st1 Oct 4, 2024

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@graingert graingert Oct 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@graingert
move finally into utility function
c9e0b73
@graingert
graingert requested a review from 1st1 October 8, 2024 08:42
1st1
1st1 approved these changes Oct 8, 2024
View reviewed changes

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Oct 8, 2024
@graingert graingert added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Oct 8, 2024
@graingert
graingert requested a review from 1st1 October 13, 2024 08:26
1st1
1st1 approved these changes Oct 14, 2024
View reviewed changes
@1st1
1st1 merged commit d5dbbf4 into python:main Oct 14, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 14, 2024
@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 <[email protected]>
@1st1

1st1 commented Oct 14, 2024

Copy link
Copy Markdown
Member

Thank you Thomas!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app

miss-islington-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington-app miss-islington-app Bot assigned 1st1 Oct 14, 2024
@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Oct 14, 2024
@1st1

1st1 commented Oct 14, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only 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 <[email protected]>
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 <[email protected]>
@miss-islington miss-islington mentioned this pull request Oct 14, 2024
[3.13] gh-125472: Revert "gh-124958: fix asyncio.TaskGroup and _PyFuture refcycles (GH-12… (GH-125476) #125478
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
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
@willingc willingc Awaiting requested review from willingc willingc is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@1st1 1st1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@graingert @1st1
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.