Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

GH-98023: Change default child watcher to PidfdChildWatcher on supported systems #98024

Merged
merged 4 commits into from
Oct 8, 2022
Merged

GH-98023: Change default child watcher to PidfdChildWatcher on supported systems #98024

merged 4 commits into from
Oct 8, 2022
+29 −2

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Oct 7, 2022 •
edited by bedevere-bot

@kumaraditya303 kumaraditya303 marked this pull request as draft October 7, 2022 09:50
@kumaraditya303 kumaraditya303 added type-feature A feature request or enhancement topic-asyncio 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Oct 7, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 2e26bf5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 7, 2022
@kumaraditya303
Copy link
Contributor Author

The unicode failure will be fixed by #98036 and is unrelated.

@kumaraditya303 kumaraditya303 marked this pull request as ready for review October 7, 2022 18:05
gvanrossum
gvanrossum reviewed Oct 7, 2022
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but...

I reviewed the child watcher architecture with @1st1 and we want to first do a refactoring on PidfdChildWatcher. Basically the current PidfdChildWatcher attaches itself to the event loop in the main thread, and if it is called from a loop in another thread it won't work. ThreadedChildWatcher doesn't have this problem.

HOWEVER, Yury has a simple refactoring of PidfdChildWatcher in mind that will solve this problem! It will make attach_loop() a no-op, and is_active() will always be True -- just as it is for the ThreadedChildWatcher.

So once Yury's change is merged, this can be merged. (We don't quite have a PR but there will be one soon, I promise.)

This was referenced Oct 7, 2022
Open
Merged
@gvanrossum
Copy link
Member

gvanrossum commented Oct 7, 2022 •
edited

Actually, @grangert already has a complete PR (GH-94184) to make PidfdChildWatcher independent from the loop, so we'll merge that, then this.

@gvanrossum
Copy link
Member

The docs failure is due to something that was accidentally merged, no point in updating and re-running all the tests. The Azure failure is due to the same thing. Still waiting for GH-94184 to go green and merge.

@gvanrossum gvanrossum merged commit 8ba9378 into python:main Oct 8, 2022
@kumaraditya303 kumaraditya303 deleted the pidfd branch October 8, 2022 06:15
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
@carljm
Merge branch 'main' into typewatch
d67d6fd 
* main:
  pythongh-68686: Retire eptag ptag scripts (python#98064)
  pythongh-97922: Run the GC only on eval breaker (python#97920)
  GitHub Workflows security hardening (python#96492)
  Add `@ezio-melotti` as codeowner for `.github/`. (python#98079)
  pythongh-97913 Docs: Add walrus operator to the index (python#97921)
  [doc] Fix broken links to C extensions accelerating stdlib modules (python#96914)
  pythongh-97822: Fix http.server documentation reference to test() function (python#98027)
  pythongh-91052: Add PyDict_Unwatch for unwatching a dictionary (python#98055)
  pythonGH-98023: Change default child watcher to PidfdChildWatcher on supported systems (python#98024)
  pythonGH-94182: Run the PidfdChildWatcher on the running loop (python#94184)
@kumaraditya303 kumaraditya303 mentioned this pull request Oct 9, 2022
Closed
carljm added a commit to carljm/cpython that referenced this pull request Oct 9, 2022
@carljm
Merge branch 'main' into urllibnonet
132c32e 
* main: (5519 commits)
  Minor edits to the Descriptor HowTo Guide (pythonGH-24901)
  Fix link to Lifecycle of a Pull Request in CONTRIBUTING (python#98102)
  pythonGH-94597: deprecate `SafeChildWatcher`, `FastChildWatcher` and `MultiLoopChildWatcher` child watchers  (python#98089)
  Auto-cancel old builds when new commit pushed to branch (python#98009)
  pythongh-95011: Migrate syslog module to Argument Clinic (pythonGH-95012)
  pythongh-68686: Retire eptag ptag scripts (python#98064)
  pythongh-97922: Run the GC only on eval breaker (python#97920)
  GitHub Workflows security hardening (python#96492)
  Add `@ezio-melotti` as codeowner for `.github/`. (python#98079)
  pythongh-97913 Docs: Add walrus operator to the index (python#97921)
  [doc] Fix broken links to C extensions accelerating stdlib modules (python#96914)
  pythongh-97822: Fix http.server documentation reference to test() function (python#98027)
  pythongh-91052: Add PyDict_Unwatch for unwatching a dictionary (python#98055)
  pythonGH-98023: Change default child watcher to PidfdChildWatcher on supported systems (python#98024)
  pythonGH-94182: Run the PidfdChildWatcher on the running loop (python#94184)
  pythongh-92886: make test_ast pass with -O (assertions off) (pythonGH-98058)
  pythongh-92886: make test_coroutines pass with -O (assertions off) (pythonGH-98060)
  pythongh-57179: Add note on symlinks for os.walk (python#94799)
  pythongh-94808: Fix regex on exotic platforms (python#98036)
  pythongh-90085: Remove vestigial -t and -c timeit options (python#94941)
  ...
mpage pushed a commit to mpage/cpython that referenced this pull request Oct 11, 2022
@kumaraditya303 @mpage
pythonGH-98023: Change default child watcher to PidfdChildWatcher on …
776f894 
…supported systems (python#98024)
bdraco added a commit to bdraco/home-assistant that referenced this pull request Feb 12, 2023
@bdraco
Use PidfdChildWatcher by default when available
29f4931 
This is a backport from cpython 3.12

https://docs.python.org/3/library/asyncio-policy.html
> PidfdChildWatcher is a “Goldilocks” child watcher implementation. It doesn’t require signals or threads, doesn’t interfere with any processes launched outside the event loop, and scales linearly with the number of subprocesses launched by the event loop. The main disadvantage is that pidfds are specific to Linux, and only work on recent (5.3+) kernels.

python/cpython#98024
There are some additional fixes in cpython 3.12
in python/cpython#94184
when there is no event loop running in the main thread
but this is not a problem we have
@bdraco bdraco mentioned this pull request Feb 12, 2023
Merged
19 tasks
balloob pushed a commit to home-assistant/core that referenced this pull request Feb 12, 2023
@bdraco
Use PidfdChildWatcher by default when available (#87951)
d1e1734 
This is a backport from cpython 3.12

https://docs.python.org/3/library/asyncio-policy.html
> PidfdChildWatcher is a “Goldilocks” child watcher implementation. It doesn’t require signals or threads, doesn’t interfere with any processes launched outside the event loop, and scales linearly with the number of subprocesses launched by the event loop. The main disadvantage is that pidfds are specific to Linux, and only work on recent (5.3+) kernels.

python/cpython#98024
There are some additional fixes in cpython 3.12
in python/cpython#94184
when there is no event loop running in the main thread
but this is not a problem we have
AlePerla pushed a commit to AlePerla/homeassistant_core that referenced this pull request Feb 17, 2023
@bdraco @AlePerla
Use PidfdChildWatcher by default when available (home-assistant#87951)
638c431 
This is a backport from cpython 3.12

https://docs.python.org/3/library/asyncio-policy.html
> PidfdChildWatcher is a “Goldilocks” child watcher implementation. It doesn’t require signals or threads, doesn’t interfere with any processes launched outside the event loop, and scales linearly with the number of subprocesses launched by the event loop. The main disadvantage is that pidfds are specific to Linux, and only work on recent (5.3+) kernels.

python/cpython#98024
There are some additional fixes in cpython 3.12
in python/cpython#94184
when there is no event loop running in the main thread
but this is not a problem we have
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum left review comments

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees
No one assigned
Labels
topic-asyncio type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kumaraditya303 @bedevere-bot @gvanrossum