From a24a5f3947233649b07f1391a7d32f3a611f5269 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 17:23:25 +0200 Subject: [PATCH 1/9] bpo-31308: If multiprocessing's forkserver dies, launch it again when necessary. --- Lib/multiprocessing/forkserver.py | 22 +++++++++++++++++-- Lib/test/_test_multiprocessing.py | 36 +++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/Lib/multiprocessing/forkserver.py b/Lib/multiprocessing/forkserver.py index 7a952e2ba69781..82e017dcc7bffc 100644 --- a/Lib/multiprocessing/forkserver.py +++ b/Lib/multiprocessing/forkserver.py @@ -34,6 +34,7 @@ class ForkServer(object): def __init__(self): self._forkserver_address = None self._forkserver_alive_fd = None + self._forkserver_pid = None self._inherited_fds = None self._lock = threading.Lock() self._preload_modules = ['__main__'] @@ -90,8 +91,24 @@ def ensure_running(self): ''' with self._lock: semaphore_tracker.ensure_running() - if self._forkserver_alive_fd is not None: - return + if self._forkserver_pid is not None: + # forkserver was launched before, is it still running? + try: + pid, status = os.waitpid(self._forkserver_pid, os.WNOHANG) + except ChildProcessError: + # On Linux at least, this means the forkserver died, + # as Python forces SIGCHLD to SIG_DFL. + # (https://linux.die.net/man/2/wait) + pass + else: + if not pid: + # forkserver still alive + return + # forkserver is dead, launch it again + os.close(self._forkserver_alive_fd) + self._forkserver_address = None + self._forkserver_alive_fd = None + self._forkserver_pid = None cmd = ('from multiprocessing.forkserver import main; ' + 'main(%d, %d, %r, **%r)') @@ -127,6 +144,7 @@ def ensure_running(self): os.close(alive_r) self._forkserver_address = address self._forkserver_alive_fd = alive_w + self._forkserver_pid = pid # # diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index d6fe7d62675631..7da1b02b8d2878 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -567,6 +567,42 @@ def test_wait_for_threads(self): proc.join() self.assertTrue(evt.is_set()) + @classmethod + def _set_event(self, evt): + evt.set() + + def check_forkserver_death(self, signum): + # bpo-31308: if the forkserver process has died, we should still + # be able to create and run new Process instances (the forkserver + # is implicitly restarted). + if self.TYPE == 'threads': + self.skipTest('test not appropriate for {}'.format(self.TYPE)) + sm = multiprocessing.get_start_method() + if sm != 'forkserver': + # The fork method by design inherits all fds from the parent, + # trying to go against it is a lost battle + self.skipTest('test not appropriate for {}'.format(sm)) + + from multiprocessing.forkserver import _forkserver + _forkserver.ensure_running() + pid = _forkserver._forkserver_pid + os.kill(pid, signum) + os.waitpid(pid, 0) + + evt = self.Event() + proc = self.Process(target=self._set_event, args=(evt,)) + proc.start() + proc.join() + self.assertTrue(evt.is_set()) + + def test_forkserver_sigint(self): + # Catchable signal + self.check_forkserver_death(signal.SIGINT) + + def test_forkserver_sigkill(self): + # Uncatchable signal + self.check_forkserver_death(signal.SIGKILL) + # # From 9132d1efb8203910ec714e570b72a90fae8511ae Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 17:58:42 +0200 Subject: [PATCH 2/9] Fix test on Windows --- Lib/test/_test_multiprocessing.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 7da1b02b8d2878..68f8c9b1a90900 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -601,7 +601,8 @@ def test_forkserver_sigint(self): def test_forkserver_sigkill(self): # Uncatchable signal - self.check_forkserver_death(signal.SIGKILL) + if os.name != 'nt': + self.check_forkserver_death(signal.SIGKILL) # From 571ccde42b849be92bfa07613409a67fd51b3e73 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 17:59:44 +0200 Subject: [PATCH 3/9] Add NEWS entry --- .../next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst diff --git a/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst b/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst new file mode 100644 index 00000000000000..fe6b58798d0a50 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst @@ -0,0 +1,2 @@ +If multiprocessing's forkserver dies (for example because of Ctrl-C), launch +it again when necessary. From 84fbfcb973f2c1ff4f88ec7fade72e32838ab686 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 18:04:51 +0200 Subject: [PATCH 4/9] Adopt a different approach: ignore SIGINT and SIGTERM, as in semaphore tracker. --- Lib/multiprocessing/forkserver.py | 7 ++++--- Lib/test/_test_multiprocessing.py | 9 +++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/Lib/multiprocessing/forkserver.py b/Lib/multiprocessing/forkserver.py index 82e017dcc7bffc..0cfc04d18276ff 100644 --- a/Lib/multiprocessing/forkserver.py +++ b/Lib/multiprocessing/forkserver.py @@ -175,11 +175,12 @@ def sigchld_handler(*_unused): # Dummy signal handler, doesn't do anything pass - # letting SIGINT through avoids KeyboardInterrupt tracebacks - # unblocking SIGCHLD allows the wakeup fd to notify our event loop handlers = { + # unblocking SIGCHLD allows the wakeup fd to notify our event loop signal.SIGCHLD: sigchld_handler, - signal.SIGINT: signal.SIG_DFL, + # protect the process from ^C and "killall python" etc + signal.SIGINT: signal.SIG_IGN, + signal.SIGTERM: signal.SIG_IGN, } old_handlers = {sig: signal.signal(sig, val) for (sig, val) in handlers.items()} diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 68f8c9b1a90900..2a6627d6ef7957 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -571,7 +571,7 @@ def test_wait_for_threads(self): def _set_event(self, evt): evt.set() - def check_forkserver_death(self, signum): + def check_forkserver_death(self, signum, should_die): # bpo-31308: if the forkserver process has died, we should still # be able to create and run new Process instances (the forkserver # is implicitly restarted). @@ -587,7 +587,8 @@ def check_forkserver_death(self, signum): _forkserver.ensure_running() pid = _forkserver._forkserver_pid os.kill(pid, signum) - os.waitpid(pid, 0) + if should_die: + os.waitpid(pid, 0) evt = self.Event() proc = self.Process(target=self._set_event, args=(evt,)) @@ -597,12 +598,12 @@ def check_forkserver_death(self, signum): def test_forkserver_sigint(self): # Catchable signal - self.check_forkserver_death(signal.SIGINT) + self.check_forkserver_death(signal.SIGINT, False) def test_forkserver_sigkill(self): # Uncatchable signal if os.name != 'nt': - self.check_forkserver_death(signal.SIGKILL) + self.check_forkserver_death(signal.SIGKILL, True) # From 0bb15e856751adeecb2a99f73cf18241d016b4b9 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 18:08:43 +0200 Subject: [PATCH 5/9] Fix comment --- Lib/multiprocessing/forkserver.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Lib/multiprocessing/forkserver.py b/Lib/multiprocessing/forkserver.py index 0cfc04d18276ff..8ebba32385574d 100644 --- a/Lib/multiprocessing/forkserver.py +++ b/Lib/multiprocessing/forkserver.py @@ -96,9 +96,7 @@ def ensure_running(self): try: pid, status = os.waitpid(self._forkserver_pid, os.WNOHANG) except ChildProcessError: - # On Linux at least, this means the forkserver died, - # as Python forces SIGCHLD to SIG_DFL. - # (https://linux.die.net/man/2/wait) + # Happens during the test suite, which also calls waitpid() pass else: if not pid: From 429547dbd76fa8d7f9de94a14e4f75f47644c55c Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 18:11:17 +0200 Subject: [PATCH 6/9] Make sure the test doesn't muck with process state --- Lib/multiprocessing/forkserver.py | 15 +++++---------- Lib/test/_test_multiprocessing.py | 9 ++++----- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/Lib/multiprocessing/forkserver.py b/Lib/multiprocessing/forkserver.py index 8ebba32385574d..d66a4799647574 100644 --- a/Lib/multiprocessing/forkserver.py +++ b/Lib/multiprocessing/forkserver.py @@ -93,16 +93,11 @@ def ensure_running(self): semaphore_tracker.ensure_running() if self._forkserver_pid is not None: # forkserver was launched before, is it still running? - try: - pid, status = os.waitpid(self._forkserver_pid, os.WNOHANG) - except ChildProcessError: - # Happens during the test suite, which also calls waitpid() - pass - else: - if not pid: - # forkserver still alive - return - # forkserver is dead, launch it again + pid, status = os.waitpid(self._forkserver_pid, os.WNOHANG) + if not pid: + # still alive + return + # dead, launch it again os.close(self._forkserver_alive_fd) self._forkserver_address = None self._forkserver_alive_fd = None diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 2a6627d6ef7957..dd5da397803dcc 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -571,7 +571,7 @@ def test_wait_for_threads(self): def _set_event(self, evt): evt.set() - def check_forkserver_death(self, signum, should_die): + def check_forkserver_death(self, signum): # bpo-31308: if the forkserver process has died, we should still # be able to create and run new Process instances (the forkserver # is implicitly restarted). @@ -587,8 +587,7 @@ def check_forkserver_death(self, signum, should_die): _forkserver.ensure_running() pid = _forkserver._forkserver_pid os.kill(pid, signum) - if should_die: - os.waitpid(pid, 0) + time.sleep(1.0) # give it time to die evt = self.Event() proc = self.Process(target=self._set_event, args=(evt,)) @@ -598,12 +597,12 @@ def check_forkserver_death(self, signum, should_die): def test_forkserver_sigint(self): # Catchable signal - self.check_forkserver_death(signal.SIGINT, False) + self.check_forkserver_death(signal.SIGINT) def test_forkserver_sigkill(self): # Uncatchable signal if os.name != 'nt': - self.check_forkserver_death(signal.SIGKILL, True) + self.check_forkserver_death(signal.SIGKILL) # From 7eab6aba55cc6bffa1faadf2b2da312b5377d5ab Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 21:12:04 +0200 Subject: [PATCH 7/9] Also test previously-started processes --- Lib/test/_test_multiprocessing.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index dd5da397803dcc..56ed25acec1dc6 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -568,7 +568,8 @@ def test_wait_for_threads(self): self.assertTrue(evt.is_set()) @classmethod - def _set_event(self, evt): + def _sleep_and_set_event(self, evt, delay=0.0): + time.sleep(delay) evt.set() def check_forkserver_death(self, signum): @@ -585,15 +586,25 @@ def check_forkserver_death(self, signum): from multiprocessing.forkserver import _forkserver _forkserver.ensure_running() + + evt = self.Event() + proc = self.Process(target=self._sleep_and_set_event, args=(evt, 1.0)) + proc.start() + pid = _forkserver._forkserver_pid os.kill(pid, signum) time.sleep(1.0) # give it time to die - evt = self.Event() - proc = self.Process(target=self._set_event, args=(evt,)) - proc.start() + evt2 = self.Event() + proc2 = self.Process(target=self._sleep_and_set_event, args=(evt2,)) + proc2.start() + proc2.join() + self.assertTrue(evt2.is_set()) + self.assertEqual(proc2.exitcode, 0) + proc.join() self.assertTrue(evt.is_set()) + self.assertIn(proc.exitcode, (0, 255)) def test_forkserver_sigint(self): # Catchable signal From 805e9e6a4734f40581327744a7057f508e2ecfd8 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 21:14:33 +0200 Subject: [PATCH 8/9] Update 2017-08-30-17-59-36.bpo-31308.KbexyC.rst --- .../next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst b/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst index fe6b58798d0a50..6068b7fd32e7c8 100644 --- a/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst +++ b/Misc/NEWS.d/next/Library/2017-08-30-17-59-36.bpo-31308.KbexyC.rst @@ -1,2 +1,2 @@ -If multiprocessing's forkserver dies (for example because of Ctrl-C), launch -it again when necessary. +Make multiprocessing's forkserver process immune to Ctrl-C and other user interruptions. +If it crashes, restart it when necessary. From 4ad9b6bd42d802fa195749ed1a359a7d88705b57 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 30 Aug 2017 23:23:44 +0200 Subject: [PATCH 9/9] Avoid masking SIGTERM in forkserver. It's not necessary and causes a race condition in test_many_processes. --- Lib/multiprocessing/forkserver.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/multiprocessing/forkserver.py b/Lib/multiprocessing/forkserver.py index d66a4799647574..040b46e66a0330 100644 --- a/Lib/multiprocessing/forkserver.py +++ b/Lib/multiprocessing/forkserver.py @@ -171,9 +171,8 @@ def sigchld_handler(*_unused): handlers = { # unblocking SIGCHLD allows the wakeup fd to notify our event loop signal.SIGCHLD: sigchld_handler, - # protect the process from ^C and "killall python" etc + # protect the process from ^C signal.SIGINT: signal.SIG_IGN, - signal.SIGTERM: signal.SIG_IGN, } old_handlers = {sig: signal.signal(sig, val) for (sig, val) in handlers.items()}