From 84e68dee9b1ce06b7e5efd6306f3686b25f9e956 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Fri, 22 Dec 2017 17:20:58 -0800 Subject: [PATCH 1/9] Don't SIGKILL children upon KeyboardInterrupt. bpo-25942: Do not allow receiving a SIGINT to cause the subprocess module to trigger a SIGKILL of the child process. SIGINT is normally sent to the child process by the OS at the same time already as was the established normal behavior in 2.7 and 3.2. this behavior change was introduced during the fix to https://bugs.python.org/issue12494 and is generally surprising to command line tool users who expect other tools launched in child processes to get their own SIGINT and do their own cleanup. --- Lib/subprocess.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index db6342fa491239..882d78a4af4a6c 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -304,7 +304,7 @@ def call(*popenargs, timeout=None, **kwargs): with Popen(*popenargs, **kwargs) as p: try: return p.wait(timeout=timeout) - except: + except Exception: # Ignore KeyboardInterrupt and SystemExit. p.kill() p.wait() raise @@ -450,7 +450,7 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): stdout, stderr = process.communicate() raise TimeoutExpired(process.args, timeout, output=stdout, stderr=stderr) - except: + except Exception: # Ignore KeyboardInterrupt and SystemExit. process.kill() process.wait() raise From 4ee70f7c5c1ae7e02b00e2dcbbc5a636a50ca52b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 27 Dec 2017 17:23:53 -0800 Subject: [PATCH 2/9] bpo-25942: subprocess is more graceful on SIGINT In Python 3.3-3.6 subprocess.call and subprocess.run would immediately SIGKILL the child process upon receiving a SIGINT (which raises a KeyboardInterrupt). We now give the child a small amount of time to exit gracefully before resorting to a SIGKILL. This is also the case for subprocess.Popen.__exit__ which would previously block indefinitely waiting for the child to die. This was hidden from many users by virtue of subprocess.call and subprocess.run sending the signal immediately. Behavior change: subprocess.Popen.__exit__ will not block indefinitely when the exiting exception is a KeyboardInterrupt. This is done for user friendliness as people expect their ^C to actually happen. This could cause occasional orphaned Popen objects when not using `call` or `run` with a child process that hasn't exited. Refactoring involved: The Popen.wait method deals with the KeyboardInterrupt second chance, existing platform specific internals have been renamed to _wait(). --- Lib/subprocess.py | 91 ++++++++++++++++++++++++++++++++----- Lib/test/test_subprocess.py | 65 ++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 11 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 882d78a4af4a6c..fa30d662e9169e 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -304,10 +304,14 @@ def call(*popenargs, timeout=None, **kwargs): with Popen(*popenargs, **kwargs) as p: try: return p.wait(timeout=timeout) - except Exception: # Ignore KeyboardInterrupt and SystemExit. + except Exception: # Excludes KeyboardInterrupt p.kill() p.wait() raise + except KeyboardInterrupt: + # https://bugs.python.org/issue25942 + p.kill() # It already got a chance within wait. + raise def check_call(*popenargs, **kwargs): @@ -443,6 +447,8 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): kwargs['stdin'] = PIPE with Popen(*popenargs, **kwargs) as process: + if timeout is not None: + endtime = _time() + timeout try: stdout, stderr = process.communicate(input, timeout=timeout) except TimeoutExpired: @@ -450,10 +456,14 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): stdout, stderr = process.communicate() raise TimeoutExpired(process.args, timeout, output=stdout, stderr=stderr) - except Exception: # Ignore KeyboardInterrupt and SystemExit. + except Exception: # excludes KeyboardInterrupt. process.kill() process.wait() raise + except KeyboardInterrupt: + # https://bugs.python.org/issue25942 + process.kill() # It already got a chance within wait. + raise retcode = process.poll() if check and retcode: raise CalledProcessError(retcode, process.args, @@ -714,6 +724,9 @@ def __init__(self, args, bufsize=-1, executable=None, self.text_mode = encoding or errors or text or universal_newlines + # How long to resume waiting on a child after the first ^C. + self._sigint_wait_secs = 0.125 # 1/xkcd221.getRandomNumber()/2 + self._closed_child_pipe_fds = False try: @@ -787,7 +800,7 @@ def _translate_newlines(self, data, encoding, errors): def __enter__(self): return self - def __exit__(self, type, value, traceback): + def __exit__(self, exc_type, value, traceback): if self.stdout: self.stdout.close() if self.stderr: @@ -796,6 +809,22 @@ def __exit__(self, type, value, traceback): if self.stdin: self.stdin.close() finally: + if exc_type == KeyboardInterrupt: + # https://bugs.python.org/issue25942 + # In the case of a KeyboardInterrupt we assume the SIGINT + # was also already sent to our child processes. We can't + # block indefinitely as that is not user friendly. + # If we have not already waited a brief amount of time in + # an interrupted .wait() or .communicate() call, do so here + # for consistency. + if self._sigint_wait_secs > 0: + try: + self._wait(timeout=self._sigint_wait_secs) + except TimeoutExpired: + pass + self._sigint_wait_secs = 0 # Note that this has been done. + return # resume the KeyboardInterrupt + # Wait for the process to terminate, to avoid zombies. self.wait() @@ -804,7 +833,7 @@ def __del__(self, _maxsize=sys.maxsize, _warn=warnings.warn): # We didn't get to successfully create a child process. return if self.returncode is None: - # Not reading subprocess exit status creates a zombi process which + # Not reading subprocess exit status creates a zombie process which # is only destroyed at the parent python process exit _warn("subprocess %s is still running" % self.pid, ResourceWarning, source=self) @@ -889,6 +918,21 @@ def communicate(self, input=None, timeout=None): try: stdout, stderr = self._communicate(input, endtime, timeout) + except KeyboardInterrupt: + # https://bugs.python.org/issue25942 + # See the detailed comment in .wait(). + if timeout is not None: + sigint_timeout = min(self._sigint_wait_secs, + self._remaining_time(endtime)) + else: + sigint_timeout = self._sigint_wait_secs + self._sigint_wait_secs = 0 # prevent __exit__ from repeating this. + try: + self._wait(timeout=sigint_timeout) + except TimeoutExpired: + pass + raise # resume the KeyboardInterrupt + finally: self._communication_started = True @@ -919,6 +963,32 @@ def _check_timeout(self, endtime, orig_timeout): raise TimeoutExpired(self.args, orig_timeout) + def wait(self, timeout=None): + """Wait for child process to terminate; returns self.returncode.""" + if timeout is not None: + endtime = _time() + timeout + try: + return self._wait(timeout=timeout) + except KeyboardInterrupt: + # https://bugs.python.org/issue25942 + # The first keyboard interrupt waits briefly for the child to + # exit under the common assumption that it also received the ^C + # generated SIGINT and will exit rapidly. A second will ^C + # will abort this immediately. Matching user patterns of hitting + # ^C more than once before resorting to ^\ (SIGKILL). + if timeout is not None: + sigint_timeout = min(self._sigint_wait_secs, + self._remaining_time(endtime)) + else: + sigint_timeout = self._sigint_wait_secs + self._sigint_wait_secs = 0 # prevent __exit__ from repeating this. + try: + self._wait(timeout=sigint_timeout) + except TimeoutExpired: + pass + raise # resume the KeyboardInterrupt + + if _mswindows: # # Windows methods @@ -1127,16 +1197,16 @@ def _internal_poll(self, _deadstate=None, return self.returncode - def wait(self, timeout=None): - """Wait for child process to terminate. Returns returncode - attribute.""" + def _wait(self, timeout): + """Internal implementation of wait() on Windows.""" if timeout is None: timeout_millis = _winapi.INFINITE else: timeout_millis = int(timeout * 1000) if self.returncode is None: + # API note: Returns immediately if timeout_millis == 0. result = _winapi.WaitForSingleObject(self._handle, - timeout_millis) + timeout_millis) if result == _winapi.WAIT_TIMEOUT: raise TimeoutExpired(self.args, timeout) self.returncode = _winapi.GetExitCodeProcess(self._handle) @@ -1498,9 +1568,8 @@ def _try_wait(self, wait_flags): return (pid, sts) - def wait(self, timeout=None): - """Wait for child process to terminate. Returns returncode - attribute.""" + def _wait(self, timeout): + """Internal implementation of wait() on POSIX.""" if self.returncode is not None: return self.returncode diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 540ad34d3f5ea0..c6ef40c99d83ac 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2922,6 +2922,71 @@ def test_terminate_dead(self): self._kill_dead_process('terminate') class MiscTests(unittest.TestCase): + + class RecordingPopen(subprocess.Popen): + """A Popen that saves a reference to each instance for testing.""" + instances_created = [] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.instances_created.append(self) + + @mock.patch.object(subprocess.Popen, "_communicate") + def _test_keyboardinterrupt_no_kill(self, popener, mock__communicate, + **kwargs): + """Fake a SIGINT happening during Popen._communicate() and ._wait(). + + This avoids the need to actually try and get test environments to send + and receive signals reliably across platforms. The net effect of a ^C + happening during a blocking subprocess execution which we want to clean + up from is a KeyboardInterrupt coming out of communicate() or wait(). + """ + + mock__communicate.side_effect = KeyboardInterrupt + try: + with mock.patch.object(subprocess.Popen, "_wait") as mock__wait: + # We patch out _wait() as no signal was involved so the + # child process isn't actually going to exit rapidly. + mock__wait.side_effect = KeyboardInterrupt + with mock.patch.object(subprocess, "Popen", + self.RecordingPopen): + with self.assertRaises(KeyboardInterrupt): + popener([sys.executable, "-c", + "import time\ntime.sleep(9)\nimport sys\n" + "sys.stderr.write('\\n!runaway child!\\n')"], + stdout=subprocess.DEVNULL, **kwargs) + for call in mock__wait.call_args_list[1:]: + self.assertNotEqual( + call, mock.call(timeout=None), + "no open-ended wait() after the first allowed: " + f"{mock__wait.call_args_list}") + sigint_calls = [] + for call in mock__wait.call_args_list: + if call == mock.call(timeout=0.125): # from Popen.__init__ + sigint_calls.append(call) + self.assertLessEqual(mock__wait.call_count, 2, + msg=mock__wait.call_args_list) + self.assertEqual(len(sigint_calls), 1, + msg=mock__wait.call_args_list) + finally: + # cleanup the forgotten (due to our mocks) child process + process = self.RecordingPopen.instances_created.pop() + process.kill() + process.wait() + self.assertEqual([], self.RecordingPopen.instances_created) + + def test_call_keyboardinterrupt_no_kill(self): + self._test_keyboardinterrupt_no_kill(subprocess.call, timeout=6.282) + + def test_run_keyboardinterrupt_no_kill(self): + self._test_keyboardinterrupt_no_kill(subprocess.run, timeout=6.282) + + def test_context_manager_keyboardinterrupt_no_kill(self): + def popen_via_context_manager(*args, **kwargs): + with subprocess.Popen(*args, **kwargs) as unused_process: + raise KeyboardInterrupt # Test how __exit__ handles ^C. + self._test_keyboardinterrupt_no_kill(popen_via_context_manager) + def test_getoutput(self): self.assertEqual(subprocess.getoutput('echo xyzzy'), 'xyzzy') self.assertEqual(subprocess.getstatusoutput('echo xyzzy'), From 5190fe4779f6b4751d0966351abb945870dd3069 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 27 Dec 2017 17:32:39 -0800 Subject: [PATCH 3/9] Mention the purpose of 0.125. --- Lib/subprocess.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index fa30d662e9169e..6d0da3d97cad06 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -725,6 +725,8 @@ def __init__(self, args, bufsize=-1, executable=None, self.text_mode = encoding or errors or text or universal_newlines # How long to resume waiting on a child after the first ^C. + # There is no right value for this. The purpose is to be polite + # yet remain good for interactive users trying to exit a tool. self._sigint_wait_secs = 0.125 # 1/xkcd221.getRandomNumber()/2 self._closed_child_pipe_fds = False From 1c7cabb9ae0bf419406b15c6ee4530754cf7fcb0 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 27 Dec 2017 19:43:51 -0800 Subject: [PATCH 4/9] remove some leftover unneeded new lines. --- Lib/subprocess.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 6d0da3d97cad06..61d0bd5f74dd56 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -447,8 +447,6 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): kwargs['stdin'] = PIPE with Popen(*popenargs, **kwargs) as process: - if timeout is not None: - endtime = _time() + timeout try: stdout, stderr = process.communicate(input, timeout=timeout) except TimeoutExpired: From 3bd102104355c8e953d17f24e29ba5e341e96f7b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 27 Dec 2017 19:45:15 -0800 Subject: [PATCH 5/9] correct a new comment in run --- Lib/subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 61d0bd5f74dd56..d2264de2a9da5d 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -460,7 +460,7 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): raise except KeyboardInterrupt: # https://bugs.python.org/issue25942 - process.kill() # It already got a chance within wait. + process.kill() # It already got a chance within communicate. raise retcode = process.poll() if check and retcode: From ba5235bcb825446afde241e14a356668a592e356 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Wed, 27 Dec 2017 20:15:56 -0800 Subject: [PATCH 6/9] Add a NEWS entry --- .../next/Library/2017-12-27-20-15-51.bpo-25942.Giyr8v.rst | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2017-12-27-20-15-51.bpo-25942.Giyr8v.rst diff --git a/Misc/NEWS.d/next/Library/2017-12-27-20-15-51.bpo-25942.Giyr8v.rst b/Misc/NEWS.d/next/Library/2017-12-27-20-15-51.bpo-25942.Giyr8v.rst new file mode 100644 index 00000000000000..b898345b2b32ce --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-12-27-20-15-51.bpo-25942.Giyr8v.rst @@ -0,0 +1,6 @@ +The subprocess module is now more graceful when handling a Ctrl-C +KeyboardInterrupt during subprocess.call, subprocess.run, or a Popen context +manager. It now waits a short amount of time for the child (presumed to +have also gotten the SIGINT) to exit, before continuing the +KeyboardInterrupt exception handling. This still includes a SIGKILL in the +call() and run() APIs, but at least the child had a chance first. From db4498013ba30c4db09b46600bd708695cc8240a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Thu, 28 Dec 2017 12:38:34 -0800 Subject: [PATCH 7/9] Simplify call and run logic. Also fixes comment typos. --- Lib/subprocess.py | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index d2264de2a9da5d..8d8ab00b2628ce 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -304,13 +304,9 @@ def call(*popenargs, timeout=None, **kwargs): with Popen(*popenargs, **kwargs) as p: try: return p.wait(timeout=timeout) - except Exception: # Excludes KeyboardInterrupt + except: # Including KeyboardInterrupt, wait handled that. p.kill() - p.wait() - raise - except KeyboardInterrupt: - # https://bugs.python.org/issue25942 - p.kill() # It already got a chance within wait. + # We don't call p.wait() as p.__exit__ does that for us. raise @@ -454,13 +450,9 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): stdout, stderr = process.communicate() raise TimeoutExpired(process.args, timeout, output=stdout, stderr=stderr) - except Exception: # excludes KeyboardInterrupt. + except: # Including KeyboardInterrupt, communicate handled that. process.kill() - process.wait() - raise - except KeyboardInterrupt: - # https://bugs.python.org/issue25942 - process.kill() # It already got a chance within communicate. + # We don't call process.wait() as .__exit__ does that for us. raise retcode = process.poll() if check and retcode: @@ -926,7 +918,7 @@ def communicate(self, input=None, timeout=None): self._remaining_time(endtime)) else: sigint_timeout = self._sigint_wait_secs - self._sigint_wait_secs = 0 # prevent __exit__ from repeating this. + self._sigint_wait_secs = 0 # nothing else should wait. try: self._wait(timeout=sigint_timeout) except TimeoutExpired: @@ -973,15 +965,15 @@ def wait(self, timeout=None): # https://bugs.python.org/issue25942 # The first keyboard interrupt waits briefly for the child to # exit under the common assumption that it also received the ^C - # generated SIGINT and will exit rapidly. A second will ^C - # will abort this immediately. Matching user patterns of hitting + # generated SIGINT and will exit rapidly. A second ^C will + # abort this immediately. Matching user patterns of hitting # ^C more than once before resorting to ^\ (SIGKILL). if timeout is not None: sigint_timeout = min(self._sigint_wait_secs, self._remaining_time(endtime)) else: sigint_timeout = self._sigint_wait_secs - self._sigint_wait_secs = 0 # prevent __exit__ from repeating this. + self._sigint_wait_secs = 0 # nothing else should wait. try: self._wait(timeout=sigint_timeout) except TimeoutExpired: From 424de820d1732e5602675f44b35f575d02527496 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google]" Date: Thu, 28 Dec 2017 12:41:13 -0800 Subject: [PATCH 8/9] Change the arbitrary small delay to another. --- Lib/subprocess.py | 2 +- Lib/test/test_subprocess.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 8d8ab00b2628ce..55bd71a47ac540 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -717,7 +717,7 @@ def __init__(self, args, bufsize=-1, executable=None, # How long to resume waiting on a child after the first ^C. # There is no right value for this. The purpose is to be polite # yet remain good for interactive users trying to exit a tool. - self._sigint_wait_secs = 0.125 # 1/xkcd221.getRandomNumber()/2 + self._sigint_wait_secs = 0.25 # 1/xkcd221.getRandomNumber() self._closed_child_pipe_fds = False diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index c6ef40c99d83ac..582590f51ce71e 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2962,7 +2962,7 @@ def _test_keyboardinterrupt_no_kill(self, popener, mock__communicate, f"{mock__wait.call_args_list}") sigint_calls = [] for call in mock__wait.call_args_list: - if call == mock.call(timeout=0.125): # from Popen.__init__ + if call == mock.call(timeout=0.25): # from Popen.__init__ sigint_calls.append(call) self.assertLessEqual(mock__wait.call_count, 2, msg=mock__wait.call_args_list) From 4acf172b234d4c26b85acdc97d83224d0acf7774 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Mon, 29 Jan 2018 20:41:29 -0800 Subject: [PATCH 9/9] clarify a couple of comments. --- Lib/subprocess.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 55bd71a47ac540..f69159e3aadcf9 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -306,7 +306,7 @@ def call(*popenargs, timeout=None, **kwargs): return p.wait(timeout=timeout) except: # Including KeyboardInterrupt, wait handled that. p.kill() - # We don't call p.wait() as p.__exit__ does that for us. + # We don't call p.wait() again as p.__exit__ does that for us. raise @@ -965,9 +965,7 @@ def wait(self, timeout=None): # https://bugs.python.org/issue25942 # The first keyboard interrupt waits briefly for the child to # exit under the common assumption that it also received the ^C - # generated SIGINT and will exit rapidly. A second ^C will - # abort this immediately. Matching user patterns of hitting - # ^C more than once before resorting to ^\ (SIGKILL). + # generated SIGINT and will exit rapidly. if timeout is not None: sigint_timeout = min(self._sigint_wait_secs, self._remaining_time(endtime))