diff options
| -rw-r--r-- | Lib/subprocess.py | 85 | ||||
| -rw-r--r-- | Lib/test/test_subprocess.py | 65 | ||||
| -rw-r--r-- | Misc/NEWS.d/next/Library/2017-12-27-20-15-51.bpo-25942.Giyr8v.rst | 6 |
3 files changed, 143 insertions, 13 deletions
diff --git a/Lib/subprocess.py b/Lib/subprocess.py index db6342f..f69159e 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -304,9 +304,9 @@ def call(*popenargs, timeout=None, **kwargs): with Popen(*popenargs, **kwargs) as p: try: return p.wait(timeout=timeout) - except: + except: # Including KeyboardInterrupt, wait handled that. p.kill() - p.wait() + # We don't call p.wait() again as p.__exit__ does that for us. raise @@ -450,9 +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: + except: # Including KeyboardInterrupt, communicate handled that. process.kill() - process.wait() + # We don't call process.wait() as .__exit__ does that for us. raise retcode = process.poll() if check and retcode: @@ -714,6 +714,11 @@ class Popen(object): 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.25 # 1/xkcd221.getRandomNumber() + self._closed_child_pipe_fds = False try: @@ -787,7 +792,7 @@ class Popen(object): 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 +801,22 @@ class Popen(object): 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 +825,7 @@ class Popen(object): # 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 +910,21 @@ class Popen(object): 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 # nothing else should wait. + try: + self._wait(timeout=sigint_timeout) + except TimeoutExpired: + pass + raise # resume the KeyboardInterrupt + finally: self._communication_started = True @@ -919,6 +955,30 @@ class Popen(object): 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. + 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 # nothing else should wait. + try: + self._wait(timeout=sigint_timeout) + except TimeoutExpired: + pass + raise # resume the KeyboardInterrupt + + if _mswindows: # # Windows methods @@ -1127,16 +1187,16 @@ class Popen(object): 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 +1558,8 @@ class Popen(object): 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 2e2721e..dd63818 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2921,6 +2921,71 @@ class Win32ProcessTestCase(BaseTestCase): 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.25): # 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'), 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 0000000..b898345 --- /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. |