GH-95899: fix asyncio.Runner to call set_event_loop once by kumaraditya303 · Pull Request #95900 · python/cpython
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 loop_factory needs to be documented. Preferably as a separate PR since then it can be merged into RC2 even if these fixes don't make it. That, or #94593 should be rolled back and whatever it fixes should be fixed in a different way, that doesn't introduce a difference for passing or not passing loop_factory. But with proper docs I think this and #95898 will be fine.
the difference between passing and not passing loop_factory needs to be documented.
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).
That, or #94593 should be rolled back and whatever it fixes should be fixed in a different way, that doesn't introduce a difference for passing or not passing loop_factory.
Not sure how it could be fixed otherwise.
Not sure how it could be fixed otherwise
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 set_event_loop
Honestly I prefer this approach anyway because it looks like Runner is adding a bunch of frames to test and asyncio.run tracebacks
Not sure how it could be fixed otherwise
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
set_event_loopHonestly 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.
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 set_event_loop
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.
Honestly I prefer this approach anyway because it looks like Runner is adding a bunch of frames to test and asyncio.run tracebacks
"adding a bunch of frames to test and asyncio.run tracebacks" seems like a non-issue to me.
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.
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.)
I reviewed it and I am fine landing it in 3.11 if we run the buildbots first.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request
…ythonGH-95900) (cherry picked from commit 914f636) Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
gvanrossum pushed a commit that referenced this pull request
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).