Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-115957: Close coroutine if the TaskGroup is inactive#116009

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

Merged
gvanrossum merged 3 commits intopython:mainfromJason-Y-Z:fix-issue-115957
Mar 6, 2024

Conversation

Jason-Y-Z
Copy link
Contributor

@Jason-Y-ZJason-Y-Z commentedFeb 27, 2024•
edited by terryjreedy
Loading

This PR addresses the issue where when we callTaskGroup.create_task() with an inactiveTaskGroup, we still get aRuntimeWarning for the coroutine. This change makes sure that the coroutine is closed in this case and updated the tests accordingly.

📚 Documentation preview 📚:https://cpython-previews--116009.org.readthedocs.build/

qin-yu reacted with thumbs up emoji
@arthur-tacca
Copy link

Thanks for doing this! I was dangerously close to doing it myself even though I really don't have time, just for the kudos of being a "Python contributor", so I'm really glad you did 😄

My comments:

  • You have described this as affecting "...if the task group has been closed" but it also affects task groups that have not yet been entered and those that are shutting down (but not yet closed). Perhaps the wording should be something like "when create_task is called on a TaskGroup that is not active (because it has not yet been entered, is shutting down or has closed)"
  • I could be mistaken but I think that test would pass even without your change? It looks like it tests thatasyncio.TaskGroup.create_task() will raiseRuntimeError if the task group has closed, but it already did that. To test the actual change it would need to check that the coroutine has been closed (by callingcoro.send(None) and checking that givesRuntimeError: cannot reuse already awaited coroutine).

@Jason-Y-Z
Copy link
ContributorAuthor

Thanks for doing this! I was dangerously close to doing it myself even though I really don't have time, just for the kudos of being a "Python contributor", so I'm really glad you did 😄

My comments:

  • You have described this as affecting "...if the task group has been closed" but it also affects task groups that have not yet been entered and those that are shutting down (but not yet closed). Perhaps the wording should be something like "when create_task is called on a TaskGroup that is not active (because it has not yet been entered, is shutting down or has closed)"
  • I could be mistaken but I think that test would pass even without your change? It looks like it tests thatasyncio.TaskGroup.create_task() will raiseRuntimeError if the task group has closed, but it already did that. To test the actual change it would need to check that the coroutine has been closed (by callingcoro.send(None) and checking that givesRuntimeError: cannot reuse already awaited coroutine).

Thanks for the comments Arthur! Yeah that makes sense, I will update the wording there.

Regarding the test, it raises a RuntimeWarning without the change for me. Maybe we have slightly different configs? But yeah let me try more specifically sending on the coroutine as well. Thanks again!

@arthur-tacca
Copy link

Sorry, to be honest I haven't actually run the tests (or for that matter cloned the CPython repo at all), I was just reading the test code. If the test suite automatically fails if there's a warning then you can ignore that comment.

@Jason-Y-ZJason-Y-Z changed the titlegh-115957: Close coroutine if the TaskGroup has been closedgh-115957: Close coroutine if the TaskGroup is inactiveFeb 28, 2024
@Jason-Y-Z
Copy link
ContributorAuthor

Sorry for tagging but a gentle nudge for reviewing please 🙏@gvanrossum

@gvanrossum
Copy link
Member

Sorry for tagging but a gentle nudge for reviewing please 🙏@gvanrossum

Yeah, it's in my queue.

Jason-Y-Z reacted with thumbs up emoji

@Jason-Y-Z
Copy link
ContributorAuthor

Sorry for tagging but a gentle nudge for reviewing please 🙏@gvanrossum

Yeah, it's in my queue.

Got it, thank you 🙏

@Jason-Y-ZJason-Y-Zforce-pushed thefix-issue-115957 branch 2 times, most recently from542d7c2 toe7c3a5eCompareMarch 2, 2024 11:30
Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

It looks like the tests you changed and added still pass even if I comment out the threecoro.close() calls added to taskgroups.py. While they print the RuntimeWarning message, they don't fail. Could you try to detect the warnings in at least one of the tests?

Comment on lines 338 to 344
.. versionchanged:: 3.13

Close the given coroutine if the task group is not active.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that the convention is that a description of the new/changed behavior is added to the main text of the documentation (in this case the two lines right above) and then theversionchanged directive is used to clarify when this behavior was added or changed. In this case that would be a little redundant but I still feel it's appropriate.

For example, the main text could include information about when a task group is considered inactive (e.g. not yet entered, already finished, or in the process of shutting down).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the explanation! I've updated the main text

@@ -185,6 +185,11 @@ Other Language Changes

(Contributed by Sebastian Pipping in :gh:`115623`.)

* When :func:`asyncio.TaskGroup.create_task` is called on an inactive
:class:`asyncio.TaskGroup`, the given coroutine will be closed (which
prevents a :exc:`RuntimeWarning`).
Copy link
Member

Choose a reason for hiding this comment

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

I'd clarify that the RuntimeWarning is about "coroutine 'blank' was never awaited".

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for the comments! I've updated the wording

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@terryjreedyterryjreedy added the interpreter-core(Objects, Python, Grammar, and Parser dirs) labelMar 5, 2024
@terryjreedyterryjreedy changed the titlegh-115957: Close coroutine if the TaskGroup is inactivegh-115957: Close coroutine if the TaskGroup is inactiveMar 5, 2024
@Jason-Y-Z
Copy link
ContributorAuthor

It looks like the tests you changed and added still pass even if I comment out the threecoro.close() calls added to taskgroups.py. While they print the RuntimeWarning message, they don't fail. Could you try to detect the warnings in at least one of the tests?

Thanks for the detailed explanations@gvanrossum ! I have updated one of the tests so that it now explicitly captures and asserts no warning. I've tried it on my machine (MacOS) and it is now failing without the change. Please let me know if there is any other improvement I can apply. Thanks!

I have made the requested changes; please review again

@bedevere-app
Copy link

Thanks for making the requested changes!

@gvanrossum: please review the changes made to this pull request.

Copy link
Member

@gvanrossumgvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! The tests pass in CI, one of the tests fails if I undo the changes in taskgroups.py only, so I will merge.

@gvanrossum
Copy link
Member

PS. Next time please don't force push. We squash changes when we merge into main anyway, and the force push makes it harder to re-review the code.

@gvanrossumgvanrossum merged commitce0ae1d intopython:mainMar 6, 2024
@Jason-Y-Z
Copy link
ContributorAuthor

Thanks@gvanrossum for the review and for the note! I will remember not to force push next time 👌

gvanrossum reacted with heart emoji

adorilson pushed a commit to adorilson/cpython that referenced this pull requestMar 25, 2024
diegorusso pushed a commit to diegorusso/cpython that referenced this pull requestApr 17, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes

@1st11st1Awaiting requested review from 1st11st1 is a code owner

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 is a code owner

@willingcwillingcAwaiting requested review from willingcwillingc is a code owner

Assignees
No one assigned
Labels
interpreter-core(Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@Jason-Y-Z@arthur-tacca@gvanrossum@terryjreedy
[8]ページ先頭
©2009-2025 Movatter.jp