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

graingert · 2022-06-23T16:53:30Z

Issue: PidfdChildWatcher runs callbacks on the main thread eventloop and not the currently running loop #94182

…thread

graingert · 2022-06-23T17:24:39Z

Lib/test/test_asyncio/test_subprocess.py

@@ -712,7 +712,7 @@ def setUp(self):
            self.set_event_loop(self.loop)


-class GenericWatcherTests:
+class GenericWatcherTests(test_utils.TestCase):


this test wasn't enabled?

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

Lib/test/test_asyncio/test_subprocess.py

test_create_subprocess_fails_with_inactive_watcher was only setting the is_active() method on the context manager not the context manager result

graingert · 2022-06-24T07:59:02Z

Lib/test/test_asyncio/test_subprocess.py


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


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

otherwise it invalidates test_create_subprocess_with_pidfd

graingert · 2022-07-06T16:03:01Z

@asvetlov can I get a review on this please?

…atcher-on-the-running-loop

…ory does not

gvanrossum

LGTM. We'll probably merge this ASAP.

gvanrossum · 2022-10-07T19:00:27Z

Lib/test/test_asyncio/test_subprocess.py

@@ -30,6 +31,19 @@
              'sys.stdout.buffer.write(data)'))]


+@functools.cache
+def _has_pidfd_support():


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.

gvanrossum · 2022-10-07T19:00:27Z

Lib/asyncio/unix_events.py

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


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

gvanrossum · 2022-10-07T19:02:07Z

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

gvanrossum · 2022-10-07T20:26:20Z

Not sure what's wrong with the docs build.

Misc/NEWS.d/next/Library/2022-06-24-08-49-47.gh-issue-94182.Wknau0.rst

…nau0.rst

1st1 · 2022-10-07T20:28:12Z

Not sure what's wrong with the docs build.

I've fixed it.

Misc/NEWS.d/next/Library/2022-06-24-08-49-47.gh-issue-94182.Wknau0.rst

…nau0.rst

1st1 · 2022-10-07T20:31:52Z

I've fixed it.

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

JelleZijlstra · 2022-10-07T21:19:43Z

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

1st1 · 2022-10-07T21:20:15Z

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.

gvanrossum · 2022-10-08T00:21:48Z

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

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

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

…#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>

run the PidfdChildWatcher on the running loop

e49365f

bedevere-bot added the awaiting review label Jun 23, 2022

enable GenericWatcherTests and add a test for pidfd watcher run in a …

49cfa81

…thread