From ba2a3dc10b61ecab188e5d6787ab2046028334fd Mon Sep 17 00:00:00 2001
From: Victor Stinner <vstinner@python.org>
Date: Fri, 4 Jun 2021 23:03:32 +0200
Subject: [PATCH] bpo-38323, asyncio: Fix MultiLoopChildWatcher race condition

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.
---
 Lib/asyncio/unix_events.py                    | 50 ++++++++-----------
 .../2021-06-04-23-07-25.bpo-38323.dL7knT.rst  |  6 +++
 2 files changed, 26 insertions(+), 30 deletions(-)
 create mode 100644 Misc/NEWS.d/next/Library/2021-06-04-23-07-25.bpo-38323.dL7knT.rst

diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py
index a55b3a375fa22d..700198c8f6ff5b 100644
--- a/Lib/asyncio/unix_events.py
+++ b/Lib/asyncio/unix_events.py
@@ -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
@@ -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
@@ -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):
@@ -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):
diff --git a/Misc/NEWS.d/next/Library/2021-06-04-23-07-25.bpo-38323.dL7knT.rst b/Misc/NEWS.d/next/Library/2021-06-04-23-07-25.bpo-38323.dL7knT.rst
new file mode 100644
index 00000000000000..5bb25a0d00f515
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2021-06-04-23-07-25.bpo-38323.dL7knT.rst
@@ -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.