-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-95899: fix asyncio.Runner to call set_event_loop once #95900
Conversation
kumaraditya303
commented
Aug 11, 2022
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: asyncio.Runner+PidfdChildWatcher leaves zombie processes #95899
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.
In the end I think this is probably okay, except that there is an undocumented difference for loop_factory
that was introduced when Runner
was introduced.
Okay, before we do anything (here or in #95898) the difference between passing and not passing |
It would be confusing for any average user to understand as the behavior is ought to be deprecated. I stated this already in #94593 (comment).
Not sure how it could be fixed otherwise. |
Between you and Thomas please come up with a doc PR. |
It can be rolled back to the 3.10 behavior by not using Runner in IsolatedAsyncioTestCase or asyncio.run and restoring the original Runner behavior of never calling Honestly I prefer this approach anyway because it looks like Runner is adding a bunch of frames to test and asyncio.run tracebacks |
Unfortunately it's kind of late in the 3.11 release cycle to do something drastic like that. If this is your only suggestion then you better go talk to Pablo. I will keep reiterating that I don't follow this code well enough to understand the consequences of your suggestion anyway. |
@gvanrossum Created #95979 |
You are late in the game for that to happen, it will break the new signal handling and after rc1 these changes are not allowed anyways.
"adding a bunch of frames to test and asyncio.run tracebacks" seems like a non-issue to me. |
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.
Now that the docs are clear, this LGTM.
But I don't know about the 3.11 backport. @pablogsal What do you think? (There's another one in the queue too, #95898.)
@gvanrossum Can you merge this at least in main? Regarding 3.11, the bot would create the PR and the RM can decide later when it should be merged, that shouldn't be blocker for this. |
I reviewed it and I am fine landing it in 3.11 if we run the buildbots first. |
Thanks @kumaraditya303 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
GH-96003 is a backport of this pull request to the 3.11 branch. |
…ythonGH-95900) (cherry picked from commit 914f636) Co-authored-by: Kumar Aditya <[email protected]>
…) (#96003) (cherry picked from commit 914f636) Co-authored-by: Kumar Aditya <[email protected]>
At some point we can revisit this -- we now have plans to deprecate set_event_loop() and eventually we can just skip calling it altogether here. We are also working on making it so that attach_loop() is always a no-op -- it already is for ThreadedChildWatcher and Yury has a plan for making it so for PidfdChildWatcher, and then we can deprecate all other child watchers. See GH-94597 (where we will keep an up to date log of where we are in this plan). |