-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
closes bpo-38692: Add a pidfd child process watcher to asyncio. #17069
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -878,6 +878,73 @@ def __exit__(self, a, b, c): | |||||||||||||
raise NotImplementedError() | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
class PidfdChildWatcher(AbstractChildWatcher): | ||||||||||||||
"""Child watcher implementation using Linux's pid file descriptors. | ||||||||||||||
|
||||||||||||||
This child watcher polls process file descriptors (pidfds) to await child | ||||||||||||||
process termination. In some respects, 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. | ||||||||||||||
""" | ||||||||||||||
|
||||||||||||||
def __init__(self): | ||||||||||||||
self._loop = None | ||||||||||||||
self._callbacks = {} | ||||||||||||||
|
||||||||||||||
def __enter__(self): | ||||||||||||||
return self | ||||||||||||||
|
||||||||||||||
def __exit__(self, exc_type, exc_value, exc_traceback): | ||||||||||||||
pass | ||||||||||||||
|
||||||||||||||
def is_active(self): | ||||||||||||||
return self._loop is not None and self._loop.is_running() | ||||||||||||||
|
||||||||||||||
def close(self): | ||||||||||||||
self.attach_loop(None) | ||||||||||||||
|
||||||||||||||
def attach_loop(self, loop): | ||||||||||||||
if self._loop is not None and loop is None and self._callbacks: | ||||||||||||||
warnings.warn( | ||||||||||||||
'A loop is being detached ' | ||||||||||||||
'from a child watcher with pending handlers', | ||||||||||||||
RuntimeWarning) | ||||||||||||||
for pidfd, _, _ in self._callbacks.values(): | ||||||||||||||
self._loop._remove_reader(pidfd) | ||||||||||||||
os.close(pidfd) | ||||||||||||||
self._callbacks.clear() | ||||||||||||||
self._loop = loop | ||||||||||||||
|
||||||||||||||
def add_child_handler(self, pid, callback, *args): | ||||||||||||||
existing = self._callbacks.get(pid) | ||||||||||||||
if existing is not None: | ||||||||||||||
self._callbacks[pid] = existing[0], callback, args | ||||||||||||||
else: | ||||||||||||||
pidfd = os.pidfd_open(pid) | ||||||||||||||
self._loop._add_reader(pidfd, self._do_wait, pid) | ||||||||||||||
self._callbacks[pid] = pidfd, callback, args | ||||||||||||||
|
||||||||||||||
def _do_wait(self, pid): | ||||||||||||||
pidfd, callback, args = self._callbacks.pop(pid) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A warning is also logged in
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||
self._loop._remove_reader(pidfd) | ||||||||||||||
_, status = os.waitpid(pid, 0) | ||||||||||||||
os.close(pidfd) | ||||||||||||||
returncode = _compute_returncode(status) | ||||||||||||||
callback(pid, returncode, *args) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear to me if this should be Is it inherently thread-safe because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thread safety requirement is on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, I see. I had thought it was on both ends for some reason. Thanks for the clarification. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I'm just reading the docstring you quoted. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 cpython/Lib/asyncio/unix_events.py Line 1325 in 5c0c325
|
||||||||||||||
|
||||||||||||||
def remove_child_handler(self, pid): | ||||||||||||||
try: | ||||||||||||||
pidfd, _, _ = self._callbacks.pop(pid) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that syntax adds value here. If someone changes the shape of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah it's fine as it is then, it's not a big deal either way. |
||||||||||||||
except KeyError: | ||||||||||||||
return False | ||||||||||||||
self._loop._remove_reader(pidfd) | ||||||||||||||
os.close(pidfd) | ||||||||||||||
return True | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def _compute_returncode(status): | ||||||||||||||
if os.WIFSIGNALED(status): | ||||||||||||||
# The child process died because of a signal. | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Add :class:`asyncio.PidfdChildWatcher`, a Linux-specific child watcher | ||
implementation that polls process file descriptors. |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it seems like
close()
should clear the callbacks rather thanattach_loop()
.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
FastChildWatcher
:cpython/Lib/asyncio/unix_events.py
Lines 1038 to 1039 in 5c0c325
MultiLoopChildWatcher
:cpython/Lib/asyncio/unix_events.py
Lines 1153 to 1154 in 5c0c325
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.