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 — bpo-38323: Change MultiLoopChildWatcher to install handlers for all the event loops by shreyanavigyan · Pull Request #26574 · python/cpython

Conversation

@shreyanavigyan

@shreyanavigyan shreyanavigyan commented Jun 7, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fix a race condition in MultiLoopChildWatcher. This patch does the following -

  1. Creates a list that will keep track of the event loops where the handler has been installed by MultiLoopChildWatcher.
  2. In attach_loop and add_child_handler, the handler is installed for the loop using add_signal_handler and the event loop instance is also added to the list.
  3. In remove_child_handler, the remove_signal_handler is called on the loop the provided pid is attached to.
  4. In close, the remove_signal_handler is called on every loop and the loop is removed from the list.

This patch changes the behavior of MultiLoopChildWatcher. The original idea remains the same but with this patch MultiLoopChildWatcher can only be used in the main thread.

https://bugs.python.org/issue38323

Sorry, something went wrong.

Uh oh!

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

All reactions
shreyanavigyan added 5 commits June 7, 2021 01:15
@shreyanavigyan
Fix the race condition
90c0e81
@shreyanavigyan
Fix a bug introduced in this patch
39e18a0
@shreyanavigyan
Adjust comments and notes
7807d79
@shreyanavigyan
Call remove_signal_handler in remove_child_handler
b38bd27
@shreyanavigyan
Remove warning
05d2263
@shreyanavigyan
shreyanavigyan requested review from 1st1 and asvetlov as code owners June 7, 2021 08:02
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jun 7, 2021
@bedevere-bot bedevere-bot added the awaiting review label Jun 7, 2021
@shreyanavigyan shreyanavigyan mentioned this pull request Jun 7, 2021
bpo-38323, asyncio: Fix MultiLoopChildWatcher race condition #26536
Closed
@shreyanavigyan
shreyanavigyan marked this pull request as draft June 7, 2021 08:04
@shreyanavigyan
Add news entry
7894fc7
@erlend-aasland
erlend-aasland requested a review from vstinner June 7, 2021 08:12
@erlend-aasland

erlend-aasland commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

Please remove the [asyncio] prefix from the title.

All reactions

Sorry, something went wrong.

Uh oh!

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

@shreyanavigyan

shreyanavigyan commented Jun 7, 2021

Copy link
Copy Markdown
Contributor Author

Ok

All reactions

Sorry, something went wrong.

Uh oh!

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

@shreyanavigyan shreyanavigyan changed the title [asyncio] bpo-38323: Change MultiLoopChildWatcher to install handlers for all the event loops bpo-38323: Change MultiLoopChildWatcher to install handlers for all the event loops Jun 7, 2021
shreyanavigyan added 2 commits June 7, 2021 14:44
@shreyanavigyan
Fix issues
e0f1788
@shreyanavigyan
Fix news entry issue
c69acff
@vstinner

vstinner commented Jun 7, 2021

Copy link
Copy Markdown
Member

I'm not sure that this issue solves MultiLoopChildWatcher design issue. The MultiLoopChildWatcher doc says that it can be used even if no event loop runs in the main thread, whereas add_signal_handler() must be called from the main thread:

https://docs.python.org/dev/library/asyncio-eventloop.html#asyncio.loop.add_signal_handler

All reactions

Sorry, something went wrong.

Uh oh!

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

@shreyanavigyan

shreyanavigyan commented Jun 7, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I'm not sure that this issue solves MultiLoopChildWatcher design issue. The MultiLoopChildWatcher doc says that it can be used even if no event loop runs in the main thread, whereas add_signal_handler() must be called from the main thread:

https://docs.python.org/dev/library/asyncio-eventloop.html#asyncio.loop.add_signal_handler

This PR changes MultiLoopChildWatcher in a way that it still doesn't require a running loop in the main thread. So the basic idea remains the same. The only change is that MultiLoopChildWatcher now can't be used from any other thread. I've tested it in the REPL and also the test ran for about 30 minutes, so I guess this patch works?

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

Breaking backwards compatibility is not an option for 3.9. I'd say the same for 3.10, since we're past feature freeze; beta phase is all about fixing regressions (and potentially ripping out newly introduced features).

All reactions

Sorry, something went wrong.

Uh oh!

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

@shreyanavigyan

shreyanavigyan commented Jun 7, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Breaking backwards compatibility is not an option for 3.9. I'd say the same for 3.10, since we're past feature freeze; beta phase is all about fixing regressions (and potentially ripping out newly introduced features).

For 3.9, I agree. But since this is a bug wouldn't it be better to fix it in 3.10 beta?

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 7, 2021

Copy link
Copy Markdown
Contributor

IMO, it's better to deprecate the class in 3.10 and remove it in 3.12, and for 3.9 do nothing. But, like Victor, I'm not an asyncio user, nor do I have any knowledge of that module, so my opinion does not really matter here.

All reactions

Sorry, something went wrong.

Uh oh!

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

@shreyanavigyan
Update
27a4af3
@github-actions

github-actions Bot commented Jul 10, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Jul 10, 2021
@vstinner vstinner mentioned this pull request Apr 10, 2022
asyncio: MultiLoopWatcher has a race condition (test_asyncio: test_close_kill_running() hangs on AMD64 RHEL7 Refleaks 3.x) #82504
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 9, 2022
@kumaraditya303

kumaraditya303 commented Nov 25, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

MultiLoopChildWatcher is deprecated now and the issue is closed so closing. Thanks.

All reactions

Sorry, something went wrong.

Uh oh!

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

@kumaraditya303 kumaraditya303 closed this Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@vstinner vstinner Awaiting requested review from vstinner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 will be requested when the pull request is marked ready for review kumaraditya303 is a code owner
@willingc willingc Awaiting requested review from willingc willingc will be requested when the pull request is marked ready for review willingc is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

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.

7 participants

@shreyanavigyan @erlend-aasland @vstinner @kumaraditya303 @the-knights-who-say-ni @ezio-melotti @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.