closes bpo-38692: Add a pidfd child process watcher to asyncio. #17069

benjaminp · 2019-11-06T03:23:56Z

https://bugs.python.org/issue38692

aeros

Thanks for the PR @benjaminp. I have a few recommendations (mostly minor):

Doc/library/asyncio-policy.rst

aeros · 2019-11-06T03:35:35Z

Doc/library/asyncio-policy.rst

+   threads, doesn't interfere with any processes launched outside the event
+   loop, and should scale 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.


I think we could remove recent, as this will likely become outdated in the relatively near future.

Suggested change

and only work on recent (5.3+) kernels.

and only work on 5.3+ kernels.

The time period before a Linux kernel becomes non-"recent" is long. There will be RHEL users in 5 years that can't use PidfdChildWatcher.

@benjaminp

There will be RHEL users in 5 years that can't use PidfdChildWatcher.

Good point. I hadn't considered that it would be quite that long, especially as someone who primarily uses the faster to update distros (such as Arch and Fedora).

aeros · 2019-11-06T03:47:38Z

Lib/asyncio/unix_events.py

+        return self._loop is not None and self._loop.is_running()
+
+    def close(self):
+        self.attach_loop(None)


IMO, it seems like close() should clear the callbacks rather than attach_loop().

Suggested change

self.attach_loop(None)

self._callbacks.clear()

self.attach_loop(None)

This is the current behavior in all three of the other child watchers that use a callbacks dict:

SafeChildWatcher:

cpython/Lib/asyncio/unix_events.py

Lines 959 to 960 in 5c0c325

def close(self):

self._callbacks.clear()

FastChildWatcher:

cpython/Lib/asyncio/unix_events.py

Lines 1038 to 1039 in 5c0c325

def close(self):

self._callbacks.clear()

MultiLoopChildWatcher:

cpython/Lib/asyncio/unix_events.py

Lines 1153 to 1154 in 5c0c325

def close(self):

self._callbacks.clear()

As far as I'm aware, the examples you list don't keep state in the event loop. Closing a PidfdChildWatcher should remove it completely from the loop.

Lib/asyncio/unix_events.py

aeros · 2019-11-06T03:57:43Z

Lib/asyncio/unix_events.py

+            self._callbacks[pid] = pidfd, callback, args
+
+    def _do_wait(self, pid):
+        pidfd, callback, args = self._callbacks.pop(pid)


A warning is also logged in MultiLoopWatcher if a pid with the given value does not exist in the callbacks. This can be helpful for debugging purposes:

Suggested change

pidfd, callback, args = self._callbacks.pop(pid)

try:

pidfd, callback, args = self._callbacks.pop(pid)

except KeyError:

logger.warning("Child watcher got an unexpected pid: %r",

pid, exc_info=True)

_do_wait being called multiple times indicates a serious programming error, and I don't think knowing the affected pid is going to be more interesting that the traceback from an exception.

aeros · 2019-11-06T04:03:52Z

Lib/asyncio/unix_events.py

+        _, status = os.waitpid(pid, 0)
+        os.close(pidfd)
+        returncode = _compute_returncode(status)
+        callback(pid, returncode, *args)


It's not clear to me if this should be callback(pid, returncode, *args) or self._loop.call_soon_threadsafe(callback, pid, returncode, *args), based on: https://github.com/python/cpython/blob/7725352d5ae92e018e6e590e50bc0f465b404807/Lib/asyncio/unix_events.py#L825-L834

Is it inherently thread-safe because PidfdChildWatcher doesn't utilize threads (outside of the main thread)?

The thread safety requirement is on callback not my invocation of it. There also aren't any threads in PidfdChildWatcher, and _do_wait runs from the event loop.

@benjaminp

The thread safety requirement is on callback not my invocation of it

Ah, I see. I had thought it was on both ends for some reason. Thanks for the clarification.

Well, I'm just reading the docstring you quoted. :)

@benjaminp

Well, I'm just reading the docstring you quoted. :)

Yeah, fair point. I had just misunderstood "callback() must be thread-safe", as the invocation and the callback itself must be thread-safe, rather than just the callback. I'm also the most familiar with ThreadedChildWatcher (since that one is the default), where multiple threads are utilized and loop.call_soon_threadsafe() is a must (within its _do_waitpid() method).

cpython/Lib/asyncio/unix_events.py

Line 1325 in 5c0c325

loop.call_soon_threadsafe(callback, pid, returncode, *args)

aeros · 2019-11-06T04:09:13Z

Lib/asyncio/unix_events.py

+
+    def remove_child_handler(self, pid):
+        try:
+            pidfd, _, _ = self._callbacks.pop(pid)


I'm not sure which is preferable (it doesn't come up very often), but I find this slightly easier to read when showing that the remaining items in the tuple aren't significant and there's more than one. Not particularly important though.

Suggested change

pidfd, _, _ = self._callbacks.pop(pid)

pidfd, *_ = self._callbacks.pop(pid)

I don't think that syntax adds value here. If someone changes the shape of self._callbacks, it's probably good for them to consider this unpacking site.

@benjaminp

I don't think that syntax adds value here.

Yeah it's fine as it is then, it's not a big deal either way.

Lib/test/test_asyncio/test_subprocess.py

aeros · 2019-11-14T03:40:45Z

Thanks @benjaminp! Meant to approve this after the most recent commit, everything looks good to me.

…onGH-17069)

benjaminp requested review from 1st1 and asvetlov as code owners November 6, 2019 03:23

the-knights-who-say-ni added the CLA signed label Nov 6, 2019

bedevere-bot added the awaiting core review label Nov 6, 2019

aeros suggested changes Nov 6, 2019

View reviewed changes

aeros added the topic-asyncio label Nov 6, 2019

closes bpo-38692: Add a pidfd child process watcher to asyncio.

56aa8f6

benjaminp merged commit 3ccdd9b into python:master Nov 14, 2019

benjaminp deleted the asyncio-pidfd branch November 14, 2019 03:08

bedevere-bot removed the awaiting core review label Nov 14, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

closes bpo-38692: Add a pidfd child process watcher to asyncio. (pyth…

999d3fa

…onGH-17069)

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

closes bpo-38692: Add a pidfd child process watcher to asyncio. (pyth…

88f6b3e

…onGH-17069)

	and only work on recent (5.3+) kernels.
	and only work on 5.3+ kernels.

	self.attach_loop(None)
	self._callbacks.clear()
	self.attach_loop(None)

-        pidfd, callback, args = self._callbacks.pop(pid)
+        try:
+            pidfd, callback, args = self._callbacks.pop(pid)
+        except KeyError:
+            logger.warning("Child watcher got an unexpected pid: %r",
+                        pid, exc_info=True)

	pidfd, _, _ = self._callbacks.pop(pid)
	pidfd, *_ = self._callbacks.pop(pid)

Uh oh!

closes bpo-38692: Add a pidfd child process watcher to asyncio. #17069

closes bpo-38692: Add a pidfd child process watcher to asyncio. #17069

Uh oh!

Conversation

benjaminp commented Nov 6, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeros left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeros Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeros Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeros Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeros Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeros Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeros commented Nov 14, 2019

Uh oh!

Uh oh!

benjaminp commented Nov 6, 2019 •

edited by bedevere-bot

Loading

aeros Nov 6, 2019 •

edited

Loading

aeros Nov 6, 2019 •

edited

Loading

aeros Nov 7, 2019 •

edited

Loading

aeros Nov 7, 2019 •

edited

Loading

aeros Nov 7, 2019 •

edited

Loading