GH-95899: fix asyncio.Runner to call set_event_loop once

[kumaraditya303]

• Issue: asyncio.Runner+PidfdChildWatcher leaves zombie processes #95899

[gvanrossum]

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]

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

@gvanrossum Created #95979

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

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]

Thanks @kumaraditya303 for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-96003 is a backport of this pull request to the 3.11 branch.

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