GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-95899: fix asyncio.Runner to call set_event_loop once by kumaraditya303 · Pull Request #95900 · python/cpython

Conversation

@kumaraditya303

@kumaraditya303 kumaraditya303 commented Aug 11, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: asyncio.Runner+PidfdChildWatcher leaves zombie processes #95899

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@kumaraditya303
fix asyncio.Runner call set_event_loop once
b6beea4
@kumaraditya303
kumaraditya303 requested review from 1st1 and asvetlov as code owners August 11, 2022 18:49
@bedevere-bot bedevere-bot added the awaiting review label Aug 11, 2022
@blurb-it
📜🤖 Added by blurb_it.
843b612
@kumaraditya303
kumaraditya303 requested a review from gvanrossum August 11, 2022 18:52
@kumaraditya303 kumaraditya303 added topic-asyncio needs backport to 3.11 only security fixes type-bug An unexpected behavior, bug, or error labels Aug 11, 2022
@graingert
graingert self-requested a review August 11, 2022 19:53
gvanrossum
gvanrossum reviewed Aug 12, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/asyncio/runners.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum gvanrossum mentioned this pull request Aug 13, 2022
GH-95736: fix IsolatedAsyncioTestCase to set event loop before calling setup functions #95898
Merged
@gvanrossum

gvanrossum commented Aug 13, 2022

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Aug 13, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Aug 13, 2022

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@graingert

graingert commented Aug 13, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Aug 13, 2022

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Aug 14, 2022

Copy link
Copy Markdown
Contributor Author

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

@gvanrossum Created #95979

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303

kumaraditya303 commented Aug 14, 2022

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum approved these changes Aug 14, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 14, 2022
@kumaraditya303

kumaraditya303 commented Aug 15, 2022

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pablogsal

pablogsal commented Aug 15, 2022

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum
gvanrossum merged commit 914f636 into python:main Aug 15, 2022
@miss-islington

miss-islington commented Aug 15, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Aug 15, 2022
@bedevere-bot

bedevere-bot commented Aug 15, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 15, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 15, 2022
@kumaraditya303 @miss-islington
pythonGH-95899: fix asyncio.Runner to call set_event_loop only once (p…
d500d1a
…ythonGH-95900)

(cherry picked from commit 914f636)

Co-authored-by: Kumar Aditya <[email protected]>
@kumaraditya303
kumaraditya303 deleted the fix-child-watcher branch August 15, 2022 17:03
gvanrossum pushed a commit that referenced this pull request Aug 15, 2022
@miss-islington @kumaraditya303
GH-95899: fix asyncio.Runner to call set_event_loop only once (GH-95900…
8bd7a0b
…) (#96003)

(cherry picked from commit 914f636)

Co-authored-by: Kumar Aditya <[email protected]>
@gvanrossum

gvanrossum commented Oct 7, 2022

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@graingert graingert Awaiting requested review from graingert

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

topic-asyncio type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@kumaraditya303 @gvanrossum @graingert @pablogsal @miss-islington @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.