From 90c0e81855c6ae603df461aca0150cdf228d7cac Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 01:15:27 +0530 Subject: [PATCH 1/9] Fix the race condition --- Lib/asyncio/unix_events.py | 72 +++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index a55b3a375fa22d..1ca24bcc45a2a8 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1219,22 +1219,34 @@ class MultiLoopChildWatcher(AbstractChildWatcher): def __init__(self): self._callbacks = {} - self._saved_sighandler = None + self._saved_sighandler = {} + self._handler_added_loops = [] def is_active(self): - return self._saved_sighandler is not None + return self._handler_added_loops is not [] def close(self): self._callbacks.clear() - if self._saved_sighandler is None: + if self._handler_added_loops is []: 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 + # 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._handler_added_loops is not [] and self._callbacks: + warnings.warn( + 'Event Loops are being detached ' + 'from a child watcher with pending handlers', + RuntimeWarning) + + if self._handler_added_loops is not []: + for loop in self._handler_added_loops: + loop.remove_signal_handler(signal.SIGCHLD) + self._handler_added_loops.remove(loop) def __enter__(self): return self @@ -1244,6 +1256,9 @@ def __exit__(self, exc_type, exc_val, exc_tb): def add_child_handler(self, pid, callback, *args): loop = events.get_running_loop() + if loop not in self._handler_added_loops: + self.attach_loop(loop) + self._callbacks[pid] = (loop, callback, args) # Prevent a race condition in case the child is already terminated. @@ -1257,21 +1272,30 @@ 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: - 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 + # # 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: + # 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 + + # # Set SA_RESTART to limit EINTR occurrences. + # signal.siginterrupt(signal.SIGCHLD, False) + assert loop is None or isinstance(loop, events.AbstractEventLoop) + + if loop is not None: + loop.add_signal_handler(signal.SIGCHLD, self._sig_chld) - # Set SA_RESTART to limit EINTR occurrences. - signal.siginterrupt(signal.SIGCHLD, False) + # Prevent a race condition in case a child terminated + # during the switch. + self._do_waitpid_all() + self._handler_added_loops.append(loop) def _do_waitpid_all(self): for pid in list(self._callbacks): @@ -1314,7 +1338,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): From 39e18a0aa9dce33937037ba8eb8be2debfc187b6 Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 11:29:52 +0530 Subject: [PATCH 2/9] Fix a bug introduced in this patch --- Lib/asyncio/unix_events.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index 1ca24bcc45a2a8..121cf63eb7678c 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1256,8 +1256,10 @@ def __exit__(self, exc_type, exc_val, exc_tb): def add_child_handler(self, pid, callback, *args): loop = events.get_running_loop() - if loop not in self._handler_added_loops: - self.attach_loop(loop) + for handler_added_loop in self._handler_added_loops: + if loop is handler_added_loop: + self.attach_loop(loop) + break self._callbacks[pid] = (loop, callback, args) From 7807d79c7a9263fc8265a83f24c2c6c055c8011d Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 12:11:57 +0530 Subject: [PATCH 3/9] Adjust comments and notes --- Lib/asyncio/unix_events.py | 50 ++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index 121cf63eb7678c..34792613d8baef 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1201,9 +1201,10 @@ def _do_waitpid_all(self): class MultiLoopChildWatcher(AbstractChildWatcher): """A watcher that doesn't require running loop in the main thread. - This implementation registers a SIGCHLD signal handler on - instantiation (which may conflict with other code that - install own handler for this signal). + This implementation registers a SIGCHLD signal handler on every event loop + subprocesses are created from (which may conflict with other code that + install own handler for this signal). This implementation can only be used + in the main thread. The solution is safe but it has a significant overhead when handling a big number of processes (*O(n)* each time a @@ -1212,10 +1213,9 @@ 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() + # It retrieves the current loop by get_running_loop() and installs + # handler on it using add_child_handler()/remove_child_handler() + def __init__(self): self._callbacks = {} @@ -1230,19 +1230,14 @@ def close(self): if self._handler_added_loops is []: 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._handler_added_loops is not [] and self._callbacks: warnings.warn( 'Event Loops are being detached ' 'from a child watcher with pending handlers', RuntimeWarning) + # Remove handler from every event loop we registered the handler + # on previously if self._handler_added_loops is not []: for loop in self._handler_added_loops: loop.remove_signal_handler(signal.SIGCHLD) @@ -1255,6 +1250,8 @@ def __exit__(self, exc_type, exc_val, exc_tb): pass def add_child_handler(self, pid, callback, *args): + # Retreives the current running loop and installs the handler + # on it. Can only be called from the main thread. loop = events.get_running_loop() for handler_added_loop in self._handler_added_loops: if loop is handler_added_loop: @@ -1274,21 +1271,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: - # 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 - - # # Set SA_RESTART to limit EINTR occurrences. - # signal.siginterrupt(signal.SIGCHLD, False) + # Install the handler on the loop and save it in a list. + # The reason to do it here is to avoid a race condition by + # indirectly calling set_wakeup_fd. Previously attach_loop + # installed the handler globally. See bpo-38323 for more + # details. + # Main thread is required for installing SIGCHLD handler + # on the event loop therefore attach_loop is only callable + # from the main thread. + + assert loop is None or isinstance(loop, events.AbstractEventLoop) if loop is not None: From b38bd273c22dc871de8a8f027f73d48596f0e0ba Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 12:26:34 +0530 Subject: [PATCH 4/9] Call remove_signal_handler in remove_child_handler --- Lib/asyncio/unix_events.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index 34792613d8baef..e752316a47b5f0 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1265,7 +1265,8 @@ def add_child_handler(self, pid, callback, *args): def remove_child_handler(self, pid): try: - del self._callbacks[pid] + loop, callback, args = self._callbacks.pop(pid) + loop.remove_signal_handler(signal.SIGCHLD) return True except KeyError: return False From 05d2263737d6a1c796d77e673835830d60ad87e1 Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 13:09:19 +0530 Subject: [PATCH 5/9] Remove warning --- Lib/asyncio/unix_events.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index e752316a47b5f0..23a683339fadf5 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1230,12 +1230,6 @@ def close(self): if self._handler_added_loops is []: return - if self._handler_added_loops is not [] and self._callbacks: - warnings.warn( - 'Event Loops are being detached ' - 'from a child watcher with pending handlers', - RuntimeWarning) - # Remove handler from every event loop we registered the handler # on previously if self._handler_added_loops is not []: From 7894fc74d08d634f6484e74a97765719524ca2dd Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 13:42:05 +0530 Subject: [PATCH 6/9] Add news entry --- .../2021-06-07-13-39-16.bpo-38323.FGViHn.rst | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst diff --git a/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst b/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst new file mode 100644 index 00000000000000..2cd3752af0aef1 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst @@ -0,0 +1,20 @@ +Fix a race condition in `MultiLoopChildWatcher`. The patch does the +following - + +1) Creates a list that will keep track of the event loops where the handler +has been installed by `MultiLoopChildWatcher`. + +2) In `attach_loop` and `add_child_handler`, the handler is installed for the +loop using `add_signal_handler` and the event loop instance is also added to + the list. + +3) In `remove_child_handler`, the `remove_signal_handler` is called on the +loop the provided pid is attached to. + +4) In `close`, the `remove_signal_handler` is called on every loop and the +loop is removed from the list. + +The patch changes the behavior of `MultiLoopChildWatcher`. `MultiLoopChildWatcher` + can now only be used in the main thread. + +Patch by Shreyan Avigyan From e0f1788fbd5b6e74cbce115db6e437294bd419df Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 14:44:40 +0530 Subject: [PATCH 7/9] Fix issues --- Lib/asyncio/unix_events.py | 8 ++++---- .../next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index 23a683339fadf5..e3836550640e07 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1215,7 +1215,7 @@ class MultiLoopChildWatcher(AbstractChildWatcher): # The class keeps compatibility with AbstractChildWatcher ABC # It retrieves the current loop by get_running_loop() and installs # handler on it using add_child_handler()/remove_child_handler() - + def __init__(self): self._callbacks = {} @@ -1266,18 +1266,18 @@ def remove_child_handler(self, pid): return False def attach_loop(self, loop): - # Install the handler on the loop and save it in a list. + # Install the handler on the loop and save it in a list. # The reason to do it here is to avoid a race condition by # indirectly calling set_wakeup_fd. Previously attach_loop # installed the handler globally. See bpo-38323 for more # details. # Main thread is required for installing SIGCHLD handler - # on the event loop therefore attach_loop is only callable + # on the event loop therefore attach_loop is only callable # from the main thread. assert loop is None or isinstance(loop, events.AbstractEventLoop) - + if loop is not None: loop.add_signal_handler(signal.SIGCHLD, self._sig_chld) diff --git a/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst b/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst index 2cd3752af0aef1..53e1ceb56027a3 100644 --- a/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst +++ b/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst @@ -6,7 +6,7 @@ has been installed by `MultiLoopChildWatcher`. 2) In `attach_loop` and `add_child_handler`, the handler is installed for the loop using `add_signal_handler` and the event loop instance is also added to - the list. +the list. 3) In `remove_child_handler`, the `remove_signal_handler` is called on the loop the provided pid is attached to. @@ -15,6 +15,6 @@ loop the provided pid is attached to. loop is removed from the list. The patch changes the behavior of `MultiLoopChildWatcher`. `MultiLoopChildWatcher` - can now only be used in the main thread. +can now only be used in the main thread. Patch by Shreyan Avigyan From c69acff7b868b0fbc16c7d8bd47f4eb2f10b6f55 Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Mon, 7 Jun 2021 15:07:26 +0530 Subject: [PATCH 8/9] Fix news entry issue --- .../2021-06-07-13-39-16.bpo-38323.FGViHn.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst b/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst index 53e1ceb56027a3..e23f4725cf2a34 100644 --- a/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst +++ b/Misc/NEWS.d/next/Library/2021-06-07-13-39-16.bpo-38323.FGViHn.rst @@ -1,20 +1,20 @@ -Fix a race condition in `MultiLoopChildWatcher`. The patch does the +Fix a race condition in ``MultiLoopChildWatcher``. The patch does the following - 1) Creates a list that will keep track of the event loops where the handler -has been installed by `MultiLoopChildWatcher`. +has been installed by ``MultiLoopChildWatcher``. -2) In `attach_loop` and `add_child_handler`, the handler is installed for the -loop using `add_signal_handler` and the event loop instance is also added to +2) In ``attach_loop`` and ``add_child_handler``, the handler is installed for the +loop using ``add_signal_handler`` and the event loop instance is also added to the list. -3) In `remove_child_handler`, the `remove_signal_handler` is called on the +3) In ``remove_child_handler``, the ``remove_signal_handler`` is called on the loop the provided pid is attached to. -4) In `close`, the `remove_signal_handler` is called on every loop and the +4) In ``close``, the ``remove_signal_handler`` is called on every loop and the loop is removed from the list. -The patch changes the behavior of `MultiLoopChildWatcher`. `MultiLoopChildWatcher` +The patch changes the behavior of ``MultiLoopChildWatcher``. ``MultiLoopChildWatcher`` can now only be used in the main thread. Patch by Shreyan Avigyan From 27a4af3a74ed4ad8c3c1cfade7af4eee77322c10 Mon Sep 17 00:00:00 2001 From: Shreyan Avigyan Date: Wed, 9 Jun 2021 20:22:00 +0530 Subject: [PATCH 9/9] Update --- Lib/asyncio/unix_events.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index e3836550640e07..b7e2d0dd5a5fb8 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -1219,7 +1219,6 @@ class MultiLoopChildWatcher(AbstractChildWatcher): def __init__(self): self._callbacks = {} - self._saved_sighandler = {} self._handler_added_loops = [] def is_active(self):