◐ Shell
clean mode source ↗

GH-95899: fix asyncio.Runner to call set_event_loop once by kumaraditya303 · Pull Request #95900 · python/cpython

gvanrossum

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.

@gvanrossum

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.

@kumaraditya303

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.

@gvanrossum

Between you and Thomas please come up with a doc PR.

@graingert

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

@gvanrossum

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

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.

@kumaraditya303

@kumaraditya303

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.

gvanrossum

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

@kumaraditya303

@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.

@pablogsal

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

@bedevere-bot

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Aug 15, 2022
…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

Aug 15, 2022
…) (#96003)

(cherry picked from commit 914f636)

Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

@gvanrossum

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