FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-94182: run the PidfdChildWatcher on the running loop by graingert · Pull Request #94184 · python/cpython · GitHub

/ cpython Public

gh-94182: run the PidfdChildWatcher on the running loop - #94184

Merged
gvanrossum merged 33 commits into
python:mainfrom
graingert:run-pidfdchildwatcher-on-the-running-loop
Oct 8, 2022
Merged

gh-94182: run the PidfdChildWatcher on the running loop#94184
gvanrossum merged 33 commits into
python:mainfrom
graingert:run-pidfdchildwatcher-on-the-running-loop

Conversation

graingert commented Jun 23, 2022
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor



class GenericWatcherTests:
class GenericWatcherTests(test_utils.TestCase):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this test wasn't enabled?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I split this fix out, so this PR is easier to review: #95009


async def execute():
watcher = mock.create_authspec(asyncio.AbstractChildWatcher)
watcher.is_active.return_value = False

graingert Jun 24, 2022
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this was patching the wrong attribute - asyncio calls asyncio.get_child_watcher().__enter__().is_active()

graingert and others added 2 commits June 24, 2022 09:10
otherwise it invalidates test_create_subprocess_with_pidfd
graingert marked this pull request as ready for review June 24, 2022 08:50
graingert requested review from 1st1 and asvetlov as code owners June 24, 2022 08:50

Copy link
Copy Markdown
Contributor Author

@asvetlov can I get a review on this please?

gvanrossum self-requested a review as a code owner October 7, 2022 18:55

gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. We'll probably merge this ASAP.



@functools.cache
def _has_pidfd_support():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This same function is also introduced by @kumaraditya303's PR GH-98024 (which uses PidfdChildWatcher when supported). I propose to refactor things so that everything uses Kumar's version. We can do that in Kumar's PR once this PR is merged.

def _do_wait(self, pid):
pidfd, callback, args = self._callbacks.pop(pid)
self._loop._remove_reader(pidfd)
def _do_wait(self, pid, pidfd, callback, args):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Curious why we can't pass the loop as an argument as well? (Though maybe that creates an unhealthy self-reference to the loop.)

Copy link
Copy Markdown
Member

(Waiting for @1st1 review and passing tests.)

Copy link
Copy Markdown
Member

Not sure what's wrong with the docs build.

1st1 commented Oct 7, 2022

Copy link
Copy Markdown
Member

Not sure what's wrong with the docs build.

I've fixed it.

1st1 commented Oct 7, 2022

Copy link
Copy Markdown
Member

I've fixed it.

Reverted the "fix" because apparently it was caused by an unrelated commit in main.

JelleZijlstra self-assigned this Oct 7, 2022

Copy link
Copy Markdown
Member

I can take care of getting CI green and hitting the merge button here.

1st1 commented Oct 7, 2022

Copy link
Copy Markdown
Member

I can take care of getting CI green and hitting the merge button here.

Please do. You might need to rebase this PR to the latest main.

Copy link
Copy Markdown
Member

The macOS build hit some GitHub Actions limit. I'm going to merge without waiting for it.

gvanrossum merged commit 3d8b224 into python:main Oct 8, 2022
carljm added a commit to carljm/cpython that referenced this pull request Oct 8, 2022
* 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)
carljm added a commit to carljm/cpython that referenced this pull request Oct 9, 2022
* 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
…#94184)

There is no reason for this watcher to be attached to any particular loop.
This should make it safe to use regardless of the lifetime of the event loop running in the main thread
(relative to other loops).

Co-authored-by: Yury Selivanov <yury@edgedb.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
bdraco added a commit to bdraco/home-assistant that referenced this pull request Feb 12, 2023
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
balloob pushed a commit to home-assistant/core that referenced this pull request Feb 12, 2023
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
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
graingert deleted the run-pidfdchildwatcher-on-the-running-loop branch October 11, 2023 15:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.10 only security fixes 3.11 only security fixes 3.12 only security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL