From 5f1df0887c1daefd137e66a90943d2afa292e8fb Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Wed, 20 Jun 2018 21:06:19 +0000 Subject: [PATCH 01/21] bpo-33613, test_semaphore_tracker_sigint: fix race condition Fail `test_semaphore_tracker_sigint` if no warnings are expected and one is received. Fix race condition when the child receives SIGINT before it can register signal handlers for it. The race condition occurs when the parent calls `_semaphore_tracker.ensure_running()` (which in turn spawns the semaphore_tracker using `_posixsubprocess.fork_exec`), the child registers the signal handlers and the parent tries to kill the child. What seem to happen is that in some slow systems, the parent sends the signal to kill the child before the child protects against the signal. There is no reliable and portable solution for the parent to wait until the child has register the signal handlers to send the signal to kill the child so a `sleep` is introduced between the spawning of the child and the parent sending the signal to give time to the child to register the handlers. --- Lib/test/_test_multiprocessing.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index c6a1f5ca905111..18212a959f15b9 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -20,6 +20,7 @@ import struct import operator import weakref +import warnings import test.support import test.support.script_helper from test import support @@ -4474,15 +4475,12 @@ def check_semaphore_tracker_death(self, signum, should_die): from multiprocessing.semaphore_tracker import _semaphore_tracker _semaphore_tracker.ensure_running() pid = _semaphore_tracker._pid + time.sleep(1.0) # Give time for the child to register the signal handlers os.kill(pid, signum) time.sleep(1.0) # give it time to die ctx = multiprocessing.get_context("spawn") - with contextlib.ExitStack() as stack: - if should_die: - stack.enter_context(self.assertWarnsRegex( - UserWarning, - "semaphore_tracker: process died")) + with warnings.catch_warnings(record=True) as all_warn: sem = ctx.Semaphore() sem.acquire() sem.release() @@ -4492,6 +4490,14 @@ def check_semaphore_tracker_death(self, signum, should_die): del sem gc.collect() self.assertIsNone(wr()) + if should_die: + self.assertEqual(len(all_warn), 1) + the_warn = all_warn[0] + issubclass(the_warn.category, UserWarning) + self.assertTrue("semaphore_tracker: process died" + in str(the_warn.message)) + else: + self.assertEqual(len(all_warn), 0) def test_semaphore_tracker_sigint(self): # Catchable signal (ignored by semaphore tracker) From 4f9babb13f980fd6e8420d38ce4471a1f355f1cf Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Sun, 24 Jun 2018 19:22:18 +0100 Subject: [PATCH 02/21] Implement PONG command in the semaphore tracker --- Lib/multiprocessing/semaphore_tracker.py | 8 +++++--- Lib/test/_test_multiprocessing.py | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 3e31bf8402ee2d..4c3d16b6f13d74 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -60,14 +60,14 @@ def ensure_running(self): fds_to_pass.append(sys.stderr.fileno()) except Exception: pass - cmd = 'from multiprocessing.semaphore_tracker import main;main(%d)' + cmd = 'from multiprocessing.semaphore_tracker import main;main({}, {})' r, w = os.pipe() try: fds_to_pass.append(r) # process will out live us, so no need to wait on pid exe = spawn.get_executable() args = [exe] + util._args_from_interpreter_flags() - args += ['-c', cmd % r] + args += ['-c', cmd.format(r, sys.stderr.fileno())] pid = util.spawnv_passfds(exe, args, fds_to_pass) except: os.close(w) @@ -105,7 +105,7 @@ def _send(self, cmd, name): getfd = _semaphore_tracker.getfd -def main(fd): +def main(fd, fd_write): '''Run semaphore tracker.''' # protect the process from ^C and "killall python" etc signal.signal(signal.SIGINT, signal.SIG_IGN) @@ -128,6 +128,8 @@ def main(fd): cache.add(name) elif cmd == b'UNREGISTER': cache.remove(name) + elif cmd == b'PING': + os.write(fd_write, b"PONG\n") else: raise RuntimeError('unrecognized command %r' % cmd) except Exception: diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 18212a959f15b9..d8d5acea5cd718 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4473,9 +4473,24 @@ def check_semaphore_tracker_death(self, signum, should_die): # bpo-31310: if the semaphore tracker process has died, it should # be restarted implicitly. from multiprocessing.semaphore_tracker import _semaphore_tracker - _semaphore_tracker.ensure_running() - pid = _semaphore_tracker._pid - time.sleep(1.0) # Give time for the child to register the signal handlers + old_stderr = sys.stderr + r, w = os.pipe() + try: + sys.stderr = open(w, "bw") + _semaphore_tracker.ensure_running() + pid = _semaphore_tracker._pid + # Wait until we receive the PONG from the child, indicating that + # the signal handlers have been registered. See bpo-33613 for more + # information. + _semaphore_tracker._send("PING", "") + with open(r, "rb") as pipe: + data = pipe.readline() + if b"PON" not in data: + raise ValueError("Invalid data in stderr!") + finally: + sys.stderr.close() + sys.stderr = old_stderr + os.kill(pid, signum) time.sleep(1.0) # give it time to die From 26d81a39959bb650364e755e30ec653f5b5b81ad Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Sun, 24 Jun 2018 21:29:29 +0100 Subject: [PATCH 03/21] Ensure the tracker is killed to allow multiple testing runs in the same process --- Lib/test/_test_multiprocessing.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index d8d5acea5cd718..deefd1ab55714b 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4473,11 +4473,16 @@ def check_semaphore_tracker_death(self, signum, should_die): # bpo-31310: if the semaphore tracker process has died, it should # be restarted implicitly. from multiprocessing.semaphore_tracker import _semaphore_tracker + pid = _semaphore_tracker._pid + if pid: + os.kill(pid, signal.SIGKILL) + time.sleep(0.5) # give it time to die old_stderr = sys.stderr r, w = os.pipe() try: sys.stderr = open(w, "bw") - _semaphore_tracker.ensure_running() + with warnings.catch_warnings(record=True) as all_warn: + _semaphore_tracker.ensure_running() pid = _semaphore_tracker._pid # Wait until we receive the PONG from the child, indicating that # the signal handlers have been registered. See bpo-33613 for more @@ -4485,7 +4490,7 @@ def check_semaphore_tracker_death(self, signum, should_die): _semaphore_tracker._send("PING", "") with open(r, "rb") as pipe: data = pipe.readline() - if b"PON" not in data: + if b"PONG" not in data: raise ValueError("Invalid data in stderr!") finally: sys.stderr.close() From a7d5c597c6c951e63289cb4b5a5efa526ad9f5c5 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 2 Jul 2018 00:54:25 +0100 Subject: [PATCH 04/21] fixup! Ensure the tracker is killed to allow multiple testing runs in the same process --- Lib/test/_test_multiprocessing.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index deefd1ab55714b..4d7d32a89e8dde 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4479,8 +4479,13 @@ def check_semaphore_tracker_death(self, signum, should_die): time.sleep(0.5) # give it time to die old_stderr = sys.stderr r, w = os.pipe() + # Make the pipe non blocking to not hang indefinitely + if sys.platform != "win32": + import fcntl + fcntl.fcntl(r, fcntl.F_SETFL, + fcntl.fcntl(r, fcntl.F_GETFL) | os.O_NONBLOCK) + sys.stderr = open(w, "bw") try: - sys.stderr = open(w, "bw") with warnings.catch_warnings(record=True) as all_warn: _semaphore_tracker.ensure_running() pid = _semaphore_tracker._pid @@ -4488,10 +4493,18 @@ def check_semaphore_tracker_death(self, signum, should_die): # the signal handlers have been registered. See bpo-33613 for more # information. _semaphore_tracker._send("PING", "") + deadline = time.monotonic() + 5 with open(r, "rb") as pipe: - data = pipe.readline() - if b"PONG" not in data: - raise ValueError("Invalid data in stderr!") + while True: + if time.monotonic() >= deadline: + raise TimeoutError("Reading data " + "from pipe took too long") + data = pipe.readline() + if not data: + continue + if b"PONG" not in data: + raise ValueError("Invalid data in stderr!") + break finally: sys.stderr.close() sys.stderr = old_stderr From a735d492b27dccedc69cd89e072d1a0ea7079f47 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Sat, 7 Jul 2018 21:23:26 +0100 Subject: [PATCH 05/21] fixup! fixup! Ensure the tracker is killed to allow multiple testing runs in the same process --- Lib/test/_test_multiprocessing.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 4d7d32a89e8dde..745718bf802709 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4478,13 +4478,9 @@ def check_semaphore_tracker_death(self, signum, should_die): os.kill(pid, signal.SIGKILL) time.sleep(0.5) # give it time to die old_stderr = sys.stderr - r, w = os.pipe() - # Make the pipe non blocking to not hang indefinitely - if sys.platform != "win32": - import fcntl - fcntl.fcntl(r, fcntl.F_SETFL, - fcntl.fcntl(r, fcntl.F_GETFL) | os.O_NONBLOCK) - sys.stderr = open(w, "bw") + testfn = test.support.TESTFN + self.addCleanup(test.support.unlink, testfn) + sys.stderr = open(testfn, "bw") try: with warnings.catch_warnings(record=True) as all_warn: _semaphore_tracker.ensure_running() @@ -4494,7 +4490,7 @@ def check_semaphore_tracker_death(self, signum, should_die): # information. _semaphore_tracker._send("PING", "") deadline = time.monotonic() + 5 - with open(r, "rb") as pipe: + with open(testfn, "rb") as pipe: while True: if time.monotonic() >= deadline: raise TimeoutError("Reading data " From 9460da4c04df77842ef84e61041886fc15960c69 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 24 Jul 2018 13:22:54 +0100 Subject: [PATCH 06/21] fixup! fixup! fixup! Ensure the tracker is killed to allow multiple testing runs in the same process --- Lib/test/_test_multiprocessing.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 745718bf802709..362f2bca4682c2 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4490,12 +4490,12 @@ def check_semaphore_tracker_death(self, signum, should_die): # information. _semaphore_tracker._send("PING", "") deadline = time.monotonic() + 5 - with open(testfn, "rb") as pipe: + with open(testfn, "rb") as tracker_stderr: while True: if time.monotonic() >= deadline: raise TimeoutError("Reading data " - "from pipe took too long") - data = pipe.readline() + "from tracker stderr took too long") + data = tracker_stderr.readline() if not data: continue if b"PONG" not in data: From fb136d004d65f416485c95e576ad67feb05f150b Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 24 Jul 2018 13:56:57 +0100 Subject: [PATCH 07/21] Use a buffered writer to answer to the PING command --- Lib/multiprocessing/semaphore_tracker.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 4c3d16b6f13d74..3df7f19404baea 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -10,7 +10,6 @@ # the next reboot. Without this semaphore tracker process, "killall # python" would probably leave unlinked semaphores. # - import os import signal import sys @@ -129,7 +128,8 @@ def main(fd, fd_write): elif cmd == b'UNREGISTER': cache.remove(name) elif cmd == b'PING': - os.write(fd_write, b"PONG\n") + with open(fd_write, "wb") as output: + output.write(b"PONG\n") else: raise RuntimeError('unrecognized command %r' % cmd) except Exception: From f3d0f0bb72faa9df0806d662b9c80ff97d0c1856 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 24 Jul 2018 14:18:03 +0100 Subject: [PATCH 08/21] Use the tracker own stderr for answering the PING command --- Lib/multiprocessing/semaphore_tracker.py | 8 ++++---- Lib/test/_test_multiprocessing.py | 12 +++++++----- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 3df7f19404baea..30a930968f80fc 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -59,14 +59,14 @@ def ensure_running(self): fds_to_pass.append(sys.stderr.fileno()) except Exception: pass - cmd = 'from multiprocessing.semaphore_tracker import main;main({}, {})' + cmd = 'from multiprocessing.semaphore_tracker import main;main({})' r, w = os.pipe() try: fds_to_pass.append(r) # process will out live us, so no need to wait on pid exe = spawn.get_executable() args = [exe] + util._args_from_interpreter_flags() - args += ['-c', cmd.format(r, sys.stderr.fileno())] + args += ['-c', cmd.format(r)] pid = util.spawnv_passfds(exe, args, fds_to_pass) except: os.close(w) @@ -104,7 +104,7 @@ def _send(self, cmd, name): getfd = _semaphore_tracker.getfd -def main(fd, fd_write): +def main(fd): '''Run semaphore tracker.''' # protect the process from ^C and "killall python" etc signal.signal(signal.SIGINT, signal.SIG_IGN) @@ -128,7 +128,7 @@ def main(fd, fd_write): elif cmd == b'UNREGISTER': cache.remove(name) elif cmd == b'PING': - with open(fd_write, "wb") as output: + with open(sys.stderr.fileno(), "wb") as output: output.write(b"PONG\n") else: raise RuntimeError('unrecognized command %r' % cmd) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 362f2bca4682c2..2b852eaf144b50 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4477,10 +4477,11 @@ def check_semaphore_tracker_death(self, signum, should_die): if pid: os.kill(pid, signal.SIGKILL) time.sleep(0.5) # give it time to die - old_stderr = sys.stderr testfn = test.support.TESTFN self.addCleanup(test.support.unlink, testfn) - sys.stderr = open(testfn, "bw") + tracker_stderr_r = open(testfn, "wb") + old_stderr = os.dup(sys.stderr.fileno()) + os.dup2(tracker_stderr_r.fileno(), sys.stderr.fileno()) try: with warnings.catch_warnings(record=True) as all_warn: _semaphore_tracker.ensure_running() @@ -4499,11 +4500,12 @@ def check_semaphore_tracker_death(self, signum, should_die): if not data: continue if b"PONG" not in data: - raise ValueError("Invalid data in stderr!") + raise ValueError("Invalid data in stderr: {}!".format(data)) break finally: - sys.stderr.close() - sys.stderr = old_stderr + tracker_stderr_r.close() + os.dup2(sys.stderr.fileno(), old_stderr) + os.close(old_stderr) os.kill(pid, signum) time.sleep(1.0) # give it time to die From f5e5cf904e640997dbb3bf5d7eaef3d7e850e790 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 27 Jul 2018 12:36:41 +0100 Subject: [PATCH 09/21] Use pthread_sigmask to avoid the race condition --- Lib/multiprocessing/semaphore_tracker.py | 17 +++++++---- Lib/test/_test_multiprocessing.py | 38 +++++------------------- 2 files changed, 19 insertions(+), 36 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 30a930968f80fc..786a3e8a22a459 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -10,6 +10,7 @@ # the next reboot. Without this semaphore tracker process, "killall # python" would probably leave unlinked semaphores. # + import os import signal import sys @@ -21,6 +22,8 @@ from . import util __all__ = ['ensure_running', 'register', 'unregister'] +_HAVE_SIGMASK = hasattr(signal, 'pthread_sigmask') +_IGNORED_SIGNALS = (signal.SIGINT, signal.SIGTERM) class SemaphoreTracker(object): @@ -59,15 +62,19 @@ def ensure_running(self): fds_to_pass.append(sys.stderr.fileno()) except Exception: pass - cmd = 'from multiprocessing.semaphore_tracker import main;main({})' + cmd = 'from multiprocessing.semaphore_tracker import main;main(%d)' r, w = os.pipe() try: fds_to_pass.append(r) # process will out live us, so no need to wait on pid exe = spawn.get_executable() args = [exe] + util._args_from_interpreter_flags() - args += ['-c', cmd.format(r)] + args += ['-c', cmd % r] + if _HAVE_SIGMASK: + signal.pthread_sigmask(signal.SIG_BLOCK, _IGNORED_SIGNALS) pid = util.spawnv_passfds(exe, args, fds_to_pass) + if _HAVE_SIGMASK: + signal.pthread_sigmask(signal.SIG_UNBLOCK, _IGNORED_SIGNALS) except: os.close(w) raise @@ -103,12 +110,13 @@ def _send(self, cmd, name): unregister = _semaphore_tracker.unregister getfd = _semaphore_tracker.getfd - def main(fd): '''Run semaphore tracker.''' # protect the process from ^C and "killall python" etc signal.signal(signal.SIGINT, signal.SIG_IGN) signal.signal(signal.SIGTERM, signal.SIG_IGN) + if _HAVE_SIGMASK: + signal.pthread_sigmask(signal.SIG_UNBLOCK, _IGNORED_SIGNALS) for f in (sys.stdin, sys.stdout): try: @@ -127,9 +135,6 @@ def main(fd): cache.add(name) elif cmd == b'UNREGISTER': cache.remove(name) - elif cmd == b'PING': - with open(sys.stderr.fileno(), "wb") as output: - output.write(b"PONG\n") else: raise RuntimeError('unrecognized command %r' % cmd) except Exception: diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 2b852eaf144b50..4dba44e63665b3 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4477,38 +4477,12 @@ def check_semaphore_tracker_death(self, signum, should_die): if pid: os.kill(pid, signal.SIGKILL) time.sleep(0.5) # give it time to die - testfn = test.support.TESTFN - self.addCleanup(test.support.unlink, testfn) - tracker_stderr_r = open(testfn, "wb") - old_stderr = os.dup(sys.stderr.fileno()) - os.dup2(tracker_stderr_r.fileno(), sys.stderr.fileno()) - try: - with warnings.catch_warnings(record=True) as all_warn: - _semaphore_tracker.ensure_running() - pid = _semaphore_tracker._pid - # Wait until we receive the PONG from the child, indicating that - # the signal handlers have been registered. See bpo-33613 for more - # information. - _semaphore_tracker._send("PING", "") - deadline = time.monotonic() + 5 - with open(testfn, "rb") as tracker_stderr: - while True: - if time.monotonic() >= deadline: - raise TimeoutError("Reading data " - "from tracker stderr took too long") - data = tracker_stderr.readline() - if not data: - continue - if b"PONG" not in data: - raise ValueError("Invalid data in stderr: {}!".format(data)) - break - finally: - tracker_stderr_r.close() - os.dup2(sys.stderr.fileno(), old_stderr) - os.close(old_stderr) + with warnings.catch_warnings(record=True) as all_warn: + _semaphore_tracker.ensure_running() + pid = _semaphore_tracker._pid os.kill(pid, signum) - time.sleep(1.0) # give it time to die + time.sleep(0.5) # give it time to die ctx = multiprocessing.get_context("spawn") with warnings.catch_warnings(record=True) as all_warn: @@ -4534,6 +4508,10 @@ def test_semaphore_tracker_sigint(self): # Catchable signal (ignored by semaphore tracker) self.check_semaphore_tracker_death(signal.SIGINT, False) + def test_semaphore_tracker_sigterm(self): + # Catchable signal (ignored by semaphore tracker) + self.check_semaphore_tracker_death(signal.SIGTERM, False) + def test_semaphore_tracker_sigkill(self): # Uncatchable signal. self.check_semaphore_tracker_death(signal.SIGKILL, True) From 4393021fdf9737bea74813dcaf445b5a54014e2a Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 27 Jul 2018 18:22:02 +0100 Subject: [PATCH 10/21] Add comment explaining the race condition --- Lib/multiprocessing/semaphore_tracker.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 786a3e8a22a459..6c2ee6cea9a02b 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -70,6 +70,12 @@ def ensure_running(self): exe = spawn.get_executable() args = [exe] + util._args_from_interpreter_flags() args += ['-c', cmd % r] + # Register a signal mask that will block the signals. This signal + # mask will be inherited by the child that is going to be spawned + # and will protect the child from a race condition that can make + # the child die before it registers signal handlers for SIGINT and + # SIGTERM. The mask is unregistered after spawning the child. + # For more info, see: https://bugs.python.org/issue33613 if _HAVE_SIGMASK: signal.pthread_sigmask(signal.SIG_BLOCK, _IGNORED_SIGNALS) pid = util.spawnv_passfds(exe, args, fds_to_pass) From d501f2fd97697ccad711e341f7ec0f0183c3b66e Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 31 Jul 2018 17:13:01 +0100 Subject: [PATCH 11/21] Restore timeout and use os.waitpid --- Lib/multiprocessing/semaphore_tracker.py | 7 ++++++- Lib/test/_test_multiprocessing.py | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 6c2ee6cea9a02b..e06cbc2cdf40b6 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -45,7 +45,12 @@ def ensure_running(self): with self._lock: if self._pid is not None: # semaphore tracker was launched before, is it still running? - pid, status = os.waitpid(self._pid, os.WNOHANG) + try: + pid, _ = os.waitpid(self._pid, os.WNOHANG) + except ChildProcessError: + # The process must be death (no process with pid self._pid). + pid = self._pid + if not pid: # => still alive return diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 4dba44e63665b3..d139241d04e036 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4476,13 +4476,13 @@ def check_semaphore_tracker_death(self, signum, should_die): pid = _semaphore_tracker._pid if pid: os.kill(pid, signal.SIGKILL) - time.sleep(0.5) # give it time to die + os.waitpid(pid, 0) with warnings.catch_warnings(record=True) as all_warn: _semaphore_tracker.ensure_running() pid = _semaphore_tracker._pid os.kill(pid, signum) - time.sleep(0.5) # give it time to die + time.sleep(1.0) # give it time to die ctx = multiprocessing.get_context("spawn") with warnings.catch_warnings(record=True) as all_warn: From 9e0243e169e8ec34b1c8917a0d5c9b1050989586 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 31 Jul 2018 18:40:25 +0100 Subject: [PATCH 12/21] fixup! Restore timeout and use os.waitpid --- Lib/multiprocessing/semaphore_tracker.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index e06cbc2cdf40b6..f3dbbc1dbb15e9 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -49,11 +49,12 @@ def ensure_running(self): pid, _ = os.waitpid(self._pid, os.WNOHANG) except ChildProcessError: # The process must be death (no process with pid self._pid). - pid = self._pid - - if not pid: + pass + else: + if not pid: # => still alive - return + return + # => dead, launch it again os.close(self._fd) self._fd = None From 26e077bc00aaf0f7ebcecb206a3882dc41be0b47 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 31 Jul 2018 23:33:10 +0100 Subject: [PATCH 13/21] Add News entry --- .../next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst diff --git a/Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst b/Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst new file mode 100644 index 00000000000000..e5d92ef1830edf --- /dev/null +++ b/Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst @@ -0,0 +1,5 @@ +Fix a race condition in ``multiprocessing.semaphore_tracker`` when the +tracker receives SIGINT before it can register signal handlers for ignoring +it. Improve ``_test_multiprocessing.test_semaphore_tracker_sigint`` and +related tests to fail when the tracker dies when delivering signals ignored +by it. From a50912b194a13cf665cfeab1162094bed4b4f4bf Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 31 Jul 2018 23:33:57 +0100 Subject: [PATCH 14/21] fixup! fixup! Restore timeout and use os.waitpid --- Lib/multiprocessing/semaphore_tracker.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index f3dbbc1dbb15e9..ca34db23faee3e 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -51,8 +51,8 @@ def ensure_running(self): # The process must be death (no process with pid self._pid). pass else: - if not pid: - # => still alive + if pid is not None: + # => still alive return # => dead, launch it again From bcbb94285ad0b640ee0a530e85d89a13a8966383 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Tue, 31 Jul 2018 23:56:08 +0100 Subject: [PATCH 15/21] fixup! Add News entry --- .../next/Library/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst | 3 +++ .../next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst | 5 ----- .../next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst | 3 +++ 3 files changed, 6 insertions(+), 5 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst delete mode 100644 Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst create mode 100644 Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst diff --git a/Misc/NEWS.d/next/Library/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst b/Misc/NEWS.d/next/Library/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst new file mode 100644 index 00000000000000..9574e43fcc6016 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst @@ -0,0 +1,3 @@ +Fix a race condition in ``multiprocessing.semaphore_tracker`` when the +tracker receives SIGINT before it can register signal handlers for ignoring +it. diff --git a/Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst b/Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst deleted file mode 100644 index e5d92ef1830edf..00000000000000 --- a/Misc/NEWS.d/next/Tests/2018-07-31-23-33-06.bpo-33613.Cdnt0i.rst +++ /dev/null @@ -1,5 +0,0 @@ -Fix a race condition in ``multiprocessing.semaphore_tracker`` when the -tracker receives SIGINT before it can register signal handlers for ignoring -it. Improve ``_test_multiprocessing.test_semaphore_tracker_sigint`` and -related tests to fail when the tracker dies when delivering signals ignored -by it. diff --git a/Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst b/Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst new file mode 100644 index 00000000000000..455732338ec57d --- /dev/null +++ b/Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst @@ -0,0 +1,3 @@ +Improve ``test_semaphore_tracker_sigint`` of multiprocessing tests to fail +when the semaphore tracker dies when delivering signals ignored by it. +Related test cases in multiprocessing tests were improved as well. From 0b46f07ab0fc4f0d3323966609e1e6f92bf3c88a Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Wed, 1 Aug 2018 09:33:25 +0100 Subject: [PATCH 16/21] fixup! fixup! fixup! Restore timeout and use os.waitpid --- Lib/multiprocessing/semaphore_tracker.py | 2 +- Lib/test/_test_multiprocessing.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index ca34db23faee3e..c56005950848fe 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -51,7 +51,7 @@ def ensure_running(self): # The process must be death (no process with pid self._pid). pass else: - if pid is not None: + if not pid: # => still alive return diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index d139241d04e036..a947e78475b1b3 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4474,7 +4474,7 @@ def check_semaphore_tracker_death(self, signum, should_die): # be restarted implicitly. from multiprocessing.semaphore_tracker import _semaphore_tracker pid = _semaphore_tracker._pid - if pid: + if pid is not None: os.kill(pid, signal.SIGKILL) os.waitpid(pid, 0) with warnings.catch_warnings(record=True) as all_warn: From 66ef7dca4496868a8fe6b04b83ac288d6fb27180 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Fri, 3 Aug 2018 01:16:44 +0100 Subject: [PATCH 17/21] Unregister sigmask if process fails to start --- Lib/multiprocessing/semaphore_tracker.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index c56005950848fe..449bf380903a97 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -22,6 +22,7 @@ from . import util __all__ = ['ensure_running', 'register', 'unregister'] + _HAVE_SIGMASK = hasattr(signal, 'pthread_sigmask') _IGNORED_SIGNALS = (signal.SIGINT, signal.SIGTERM) @@ -84,9 +85,11 @@ def ensure_running(self): # For more info, see: https://bugs.python.org/issue33613 if _HAVE_SIGMASK: signal.pthread_sigmask(signal.SIG_BLOCK, _IGNORED_SIGNALS) - pid = util.spawnv_passfds(exe, args, fds_to_pass) - if _HAVE_SIGMASK: - signal.pthread_sigmask(signal.SIG_UNBLOCK, _IGNORED_SIGNALS) + try: + pid = util.spawnv_passfds(exe, args, fds_to_pass) + finally: + if _HAVE_SIGMASK: + signal.pthread_sigmask(signal.SIG_UNBLOCK, _IGNORED_SIGNALS) except: os.close(w) raise From f7d2d4d454fa5aab0c994248f499153b4230f0be Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 3 Sep 2018 21:31:16 +0100 Subject: [PATCH 18/21] Include the signal registering in the try block --- Lib/multiprocessing/semaphore_tracker.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 449bf380903a97..7302687b2dc5f4 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -83,9 +83,9 @@ def ensure_running(self): # the child die before it registers signal handlers for SIGINT and # SIGTERM. The mask is unregistered after spawning the child. # For more info, see: https://bugs.python.org/issue33613 - if _HAVE_SIGMASK: - signal.pthread_sigmask(signal.SIG_BLOCK, _IGNORED_SIGNALS) try: + if _HAVE_SIGMASK: + signal.pthread_sigmask(signal.SIG_BLOCK, _IGNORED_SIGNALS) pid = util.spawnv_passfds(exe, args, fds_to_pass) finally: if _HAVE_SIGMASK: From 4a09fde6e64cd5608ca9e17c74b2baaf1050d181 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 3 Sep 2018 21:41:04 +0100 Subject: [PATCH 19/21] Simplify comment when handling ChildProcessError --- Lib/multiprocessing/semaphore_tracker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index 7302687b2dc5f4..bdb1b27b64a30c 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -49,7 +49,7 @@ def ensure_running(self): try: pid, _ = os.waitpid(self._pid, os.WNOHANG) except ChildProcessError: - # The process must be death (no process with pid self._pid). + # The process terminated pass else: if not pid: From c9f96c1edcaadafd5a2039c876a05b4ec0bbbcda Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 3 Sep 2018 21:45:54 +0100 Subject: [PATCH 20/21] Eliminate hardcoded reference to the bug tracker url --- Lib/multiprocessing/semaphore_tracker.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/multiprocessing/semaphore_tracker.py b/Lib/multiprocessing/semaphore_tracker.py index bdb1b27b64a30c..82833bcf861a49 100644 --- a/Lib/multiprocessing/semaphore_tracker.py +++ b/Lib/multiprocessing/semaphore_tracker.py @@ -77,12 +77,12 @@ def ensure_running(self): exe = spawn.get_executable() args = [exe] + util._args_from_interpreter_flags() args += ['-c', cmd % r] - # Register a signal mask that will block the signals. This signal - # mask will be inherited by the child that is going to be spawned - # and will protect the child from a race condition that can make - # the child die before it registers signal handlers for SIGINT and - # SIGTERM. The mask is unregistered after spawning the child. - # For more info, see: https://bugs.python.org/issue33613 + # bpo-33613: Register a signal mask that will block the signals. + # This signal mask will be inherited by the child that is going + # to be spawned and will protect the child from a race condition + # that can make the child die before it registers signal handlers + # for SIGINT and SIGTERM. The mask is unregistered after spawning + # the child. try: if _HAVE_SIGMASK: signal.pthread_sigmask(signal.SIG_BLOCK, _IGNORED_SIGNALS) From c0a088c26ebb3ac7c9d7a1ebc8c99792f1f82dac Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Tue, 4 Sep 2018 10:14:24 +0200 Subject: [PATCH 21/21] Remove extraneous NEWS file --- .../NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst diff --git a/Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst b/Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst deleted file mode 100644 index 455732338ec57d..00000000000000 --- a/Misc/NEWS.d/next/Tests/2018-07-31-23-55-50.bpo-33613.6T7zol.rst +++ /dev/null @@ -1,3 +0,0 @@ -Improve ``test_semaphore_tracker_sigint`` of multiprocessing tests to fail -when the semaphore tracker dies when delivering signals ignored by it. -Related test cases in multiprocessing tests were improved as well.