GeezerVison

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 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!

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.

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

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

Yeah, it's in my queue.

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

Yeah, it's in my queue.

Got it, thank you 🙏

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.

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

Thanks for making the requested changes!

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

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.

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