bpo-25942: make subprocess more graceful on ^C (GH-5026)
Do not allow receiving a SIGINT to cause the subprocess module to trigger an immediate SIGKILL of the child process. SIGINT is normally sent to all child processes 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. 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(). Also fixes comment typos.
This commit is contained in:
Gregory P. Smith
GitHub
parent
83e64c8a54
commit
f4d644f36f
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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'),
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
Loading…
Reference in New Issue