gh-115957: Close coroutine if the TaskGroup is inactive

[Jason-Y-Z]

This PR addresses the issue where when we call TaskGroup.create_task() with an inactive TaskGroup, we still get a RuntimeWarning 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/

• Issue: asyncio.TaskGroup does not close unawaited coroutines #115957

[arthur-tacca]

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 that asyncio.TaskGroup.create_task() will raise RuntimeError 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 calling coro.send(None) and checking that gives RuntimeError: cannot reuse already awaited coroutine).

[Jason-Y-Z]

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 that asyncio.TaskGroup.create_task() will raise RuntimeError 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 calling coro.send(None) and checking that gives RuntimeError: 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]

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-Z]

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

[gvanrossum]

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

Yeah, it's in my queue.

[Jason-Y-Z]

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

Yeah, it's in my queue.

Got it, thank you 🙏

[gvanrossum]

It looks like the tests you changed and added still pass even if I comment out the three coro.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?

[gvanrossum]

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

[Jason-Y-Z]

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

[gvanrossum]

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

[Jason-Y-Z]

Thanks for the comments! I've updated the wording

[bedevere-app]

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 phrase I 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.

[Jason-Y-Z]

It looks like the tests you changed and added still pass even if I comment out the three coro.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]

Thanks for making the requested changes!

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

[gvanrossum]

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]

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.

[Jason-Y-Z]

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