Skip to content

[WIP] bpo-38323: Fix MultiLoopChildWatcher hangs#20142

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

Closed
cjerdonek wants to merge 5 commits into from
Closed

[WIP] bpo-38323: Fix MultiLoopChildWatcher hangs #20142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d10132
bpo-38323: Fix rare MultiLoopChildWatcher hangs.
cjerdonek May 16, 2020
9618884
Add docstring.
cjerdonek May 17, 2020
4d4c147
Address a couple review comments.
cjerdonek Oct 16, 2020
14f6cfc
Revert tearDown() change.
cjerdonek Oct 16, 2020
22ba0b1
Tweak documentation change.
cjerdonek Oct 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Doc/library/asyncio-eventloop.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1036,7 +1036,10 @@ Unix signals
The callback will be invoked by *loop*, along with other queued callbacks
and runnable coroutines of that event loop. Unlike signal handlers
registered using :func:`signal.signal`, a callback registered with this
function is allowed to interact with the event loop.
method is allowed to interact with the event loop. Also, if
:func:`signal.signal` is used instead of this method, the event loop
is not guaranteed to awaken when a signal is received, which can be a
cause of application hangs.

Raise :exc:`ValueError` if the signal number is invalid or uncatchable.
Raise :exc:`RuntimeError` if there is a problem setting up the handler.
Expand Down
13 changes: 12 additions & 1 deletion Doc/library/asyncio-policy.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -219,7 +219,7 @@ implementation used by the asyncio event loop:

This implementation registers a :py:data:`SIGCHLD` signal handler on
instantiation. That can break third-party code that installs a custom handler for
`SIGCHLD`. signal).
the :py:data:`SIGCHLD` signal).

The watcher avoids disrupting other code spawning processes
by polling every process explicitly on a :py:data:`SIGCHLD` signal.
Expand All@@ -233,6 +233,17 @@ implementation used by the asyncio event loop:

.. versionadded:: 3.8

.. method:: attach_loop(loop)

Registers the :py:data:`SIGCHLD` signal handler. Like
:meth:`loop.add_signal_handler`, this method can only be invoked
from the main thread.

.. versionchanged:: 3.10

The method now calls :func:`signal.set_wakeup_fd` as part of the
handler initialization.

.. class:: SafeChildWatcher

This implementation uses active event loop from the main thread to handle
Expand Down
39 changes: 30 additions & 9 deletions Lib/asyncio/unix_events.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -78,6 +78,8 @@ def _process_self_data(self, data):
def add_signal_handler(self, sig, callback, *args):
"""Add a handler for a signal. UNIX only.

This method can only be called from the main thread.

Raise ValueError if the signal number is invalid or uncatchable.
Raise RuntimeError if there is a problem setting up the handler.
"""
Expand DownExpand Up@@ -1232,10 +1234,15 @@ def close(self):
self._callbacks.clear()
if self._saved_sighandler is not None:
handler = signal.getsignal(signal.SIGCHLD)
if handler != self._sig_chld:
# add_signal_handler() sets the handler to _sighandler_noop.
if handler != _sighandler_noop:
logger.warning("SIGCHLD handler was changed by outside code")
else:
loop = self._loop
# This clears the wakeup file descriptor if necessary.
loop.remove_signal_handler(signal.SIGCHLD)
signal.signal(signal.SIGCHLD, self._saved_sighandler)

self._saved_sighandler = None

def __enter__(self):
Expand All@@ -1259,19 +1266,33 @@ def remove_child_handler(self, pid):
return False

def attach_loop(self, loop):
"""
This registers the SIGCHLD signal handler.

This method can only be called from the main thread.
"""
# Don't save the loop but initialize itself if called first time
# The reason to do it here is that attach_loop() is called from
# unix policy only for the main thread.
# Main thread is required for subscription on SIGCHLD signal
if loop is None or self._saved_sighandler is not None:
Copy link
MemberAuthor

@cjerdonekcjerdonekOct 17, 2020

Choose a reason for hiding this comment

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

@aeros I would think through what should happen if either loop is None or self._saved_sighandler is not None. For example, the previous implementation didn't use the loop argument, so there is a risk that code will stop working for people who were passing loop=None.

return

self._loop = loop
Copy link
MemberAuthor

@cjerdonekcjerdonekOct 16, 2020

Choose a reason for hiding this comment

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

Part of why I was saying to remove this line is to honor the code comment 7 lines above:

# Don't save the loop but initialize itself if called first time

self._saved_sighandler = signal.getsignal(signal.SIGCHLD)
if self._saved_sighandler is None:
self._saved_sighandler = signal.signal(signal.SIGCHLD, self._sig_chld)
if self._saved_sighandler is None:
logger.warning("Previous SIGCHLD handler was set by non-Python code, "
"restore to default handler on watcher close.")
self._saved_sighandler = signal.SIG_DFL
logger.warning("Previous SIGCHLD handler was set by non-Python code, "
"restore to default handler on watcher close.")
self._saved_sighandler = signal.SIG_DFL

# Set SA_RESTART to limit EINTR occurrences.
signal.siginterrupt(signal.SIGCHLD, False)
if self._callbacks:
warnings.warn(
'A loop is being detached '
'from a child watcher with pending handlers',
RuntimeWarning)

# This also sets up the wakeup file descriptor.
loop.add_signal_handler(signal.SIGCHLD, self._sig_chld)

def _do_waitpid_all(self):
for pid in list(self._callbacks):
Expand DownExpand Up@@ -1314,7 +1335,7 @@ def _do_waitpid(self, expected_pid):
expected_pid, returncode)
loop.call_soon_threadsafe(callback, pid, returncode, *args)

def _sig_chld(self, signum, frame):
def _sig_chld(self, *args):
try:
self._do_waitpid_all()
except (SystemExit, KeyboardInterrupt):
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2020-05-16-17-50-10.bpo-38323.Ar35np.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
Fix rare cases with :class:`asyncio.MultiLoopChildWatcher` where the event
loop can fail to awaken in response to a :py:data:`SIGCHLD` signal.