Skip to content

bpo-38323, asyncio: Fix MultiLoopChildWatcher race condition #26536

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
wants to merge 1 commit into from
Closed

bpo-38323, asyncio: Fix MultiLoopChildWatcher race condition #26536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
50 changes: 20 additions & 30 deletions Lib/asyncio/unix_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,7 @@ def _do_waitpid_all(self):


class MultiLoopChildWatcher(AbstractChildWatcher):
"""A watcher that doesn't require running loop in the main thread.
"""A watcher that requires running loop in the main thread.

This implementation registers a SIGCHLD signal handler on
instantiation (which may conflict with other code that
Expand All @@ -1211,30 +1211,25 @@ class MultiLoopChildWatcher(AbstractChildWatcher):
"""

# Implementation note:
# The class keeps compatibility with AbstractChildWatcher ABC
# To achieve this it has empty attach_loop() method
# and doesn't accept explicit loop argument
# for add_child_handler()/remove_child_handler()
# but retrieves the current loop by get_running_loop()
# The class keeps compatibility with AbstractChildWatcher ABC.
# add_child_handler() and remove_child_handler() retrieve the current loop
# by get_running_loop()

def __init__(self):
self._callbacks = {}
self._saved_sighandler = None
self._loop = None

def is_active(self):
return self._saved_sighandler is not None
return (self._loop is not None)

def _remove_signal_handler(self):
self._loop.remove_signal_handler(signal.SIGCHLD)
self._loop = None

def close(self):
self._callbacks.clear()
if self._saved_sighandler is None:
return

handler = signal.getsignal(signal.SIGCHLD)
if handler != self._sig_chld:
logger.warning("SIGCHLD handler was changed by outside code")
else:
signal.signal(signal.SIGCHLD, self._saved_sighandler)
self._saved_sighandler = None
if self._loop is not None:
self._remove_signal_handler()

def __enter__(self):
return self
Expand All @@ -1257,21 +1252,16 @@ def remove_child_handler(self, pid):
return False

def attach_loop(self, loop):
# 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 self._saved_sighandler is not None:
if loop is None:
return

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
# attach_loop() is called from unix policy only for the main thread.
# Main thread is required for subscription on SIGCHLD signal
if self._loop is not None:
return

# Set SA_RESTART to limit EINTR occurrences.
signal.siginterrupt(signal.SIGCHLD, False)
loop.add_signal_handler(signal.SIGCHLD, self._sig_chld)
self._loop = loop

def _do_waitpid_all(self):
for pid in list(self._callbacks):
Expand Down Expand Up @@ -1314,7 +1304,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):
try:
self._do_waitpid_all()
except (SystemExit, KeyboardInterrupt):
Expand Down
6 changes: 6 additions & 0 deletions Misc/NEWS.d/next/Library/2021-06-04-23-07-25.bpo-38323.dL7knT.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Fix a race condition in asyncio MultiLoopChildWatcher: call the loop
add_signal_handler() rather than calling directly signal.signal(), so
signal.set_wakeup_fd() is used to wake up the event loop when the SIGCHLD
signal is received. Previously, the event loop could block forever if the
signal was received while the loop was idle. The signal was only proceed
when another event woke up the event loop. Patch by Victor Stinner.