diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 693355cce7f93d..2451993dd65994 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -76,6 +76,14 @@ compatibility with older versions, see the :ref:`call-function-trio` section. The *universal_newlines* argument is equivalent to *text* and is provided for backwards compatibility. By default, file objects are opened in binary mode. + If the current process encounters an exception (e.g. :exc:`KeyboardInterrupt`) + while waiting for the child process and *cleanup_timeout* is non-zero, + the child process is given additional time to finish before it is killed, + to allow potential clean-up operations in the child to complete. + During this time, a second exception will kill the child immediately. + If *cleanup_timeout* is not specified, then any remaining time from the original + *timeout* is used, or 1 second if no original *timeout* was specified. + Examples:: >>> subprocess.run(["ls", "-l"]) # doesn't capture output @@ -97,9 +105,11 @@ compatibility with older versions, see the :ref:`call-function-trio` section. Added *encoding* and *errors* parameters .. versionchanged:: 3.7 - Added the *text* parameter, as a more understandable alias of *universal_newlines* + .. versionchanged:: 3.7 + *cleanup_timeout* was added. + .. class:: CompletedProcess The return value from :func:`run`, representing a process that has finished. @@ -860,7 +870,7 @@ Prior to Python 3.5, these three functions comprised the high level API to subprocess. You can now use :func:`run` in many cases, but lots of existing code calls these functions. -.. function:: call(args, *, stdin=None, stdout=None, stderr=None, shell=False, cwd=None, timeout=None) +.. function:: call(args, *, stdin=None, stdout=None, stderr=None, shell=False, cwd=None, timeout=None, cleanup_timeout=None) Run the command described by *args*. Wait for command to complete, then return the :attr:`~Popen.returncode` attribute. @@ -886,6 +896,9 @@ calls these functions. .. versionchanged:: 3.3 *timeout* was added. + .. versionchanged:: 3.7 + *cleanup_timeout* was added. + .. function:: check_call(args, *, stdin=None, stdout=None, stderr=None, shell=False, cwd=None, timeout=None) Run command with arguments. Wait for command to complete. If the return diff --git a/Lib/subprocess.py b/Lib/subprocess.py index f6d03f89a85788..e51c98da7260e2 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -255,22 +255,23 @@ def _args_from_interpreter_flags(): return args -def call(*popenargs, timeout=None, **kwargs): +def call(*popenargs, timeout=None, cleanup_timeout=None, **kwargs): """Run command with arguments. Wait for command to complete or timeout, then return the returncode attribute. - The arguments are the same as for the Popen constructor. Example: + This is equivalent to: - retcode = call(["ls", "-l"]) - """ - with Popen(*popenargs, **kwargs) as p: - try: - return p.wait(timeout=timeout) - except: - p.kill() - p.wait() - raise + run(...).returncode + + (except that the input and check parameters are not supported). + See run() for argument details. + + Example: + retcode = call(["ls", "-l"]) + """ + return run(*popenargs, timeout=timeout, cleanup_timeout=cleanup_timeout, + **kwargs).returncode def check_call(*popenargs, **kwargs): """Run command with arguments. Wait for command to complete. If @@ -371,7 +372,7 @@ def check_returncode(self): self.stderr) -def run(*popenargs, input=None, timeout=None, check=False, **kwargs): +def run(*popenargs, input=None, timeout=None, check=False, cleanup_timeout=None, **kwargs): """Run command with arguments and return a CompletedProcess instance. The returned instance will have attributes args, returncode, stdout and @@ -386,6 +387,17 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): If timeout is given, and the process takes too long, a TimeoutExpired exception will be raised. + If the current process encounters any exception (e.g. KeyboardInterrupt) and + cleanup_timeout is non-zero, the child process is given some additional + time to finish before it is killed, to allow potential clean-up + operations in the child to complete. During this time, a second exception + will kill the child immediately. + + If cleanup_timeout is not specified, then a default value is chosen: + If timeout was provided, then any remaining time from the original timeout + is used. Otherwise, if the exception is a KeyboardInterrupt, the child is + given 1 second to clean up. Otherwise, the child is killed immediately. + There is an optional argument "input", allowing you to pass bytes or a string to the subprocess's stdin. If you use this argument you may not also use the Popen constructor's "stdin" argument, as @@ -404,6 +416,8 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): raise ValueError('stdin and input arguments may not both be used.') kwargs['stdin'] = PIPE + start_time = _time() + with Popen(*popenargs, **kwargs) as process: try: stdout, stderr = process.communicate(input, timeout=timeout) @@ -412,10 +426,53 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): stdout, stderr = process.communicate() raise TimeoutExpired(process.args, timeout, output=stdout, stderr=stderr) - except: - process.kill() - process.wait() - raise + except BaseException as ex: + try: + if cleanup_timeout is None: + # Choose a reasonable default + if timeout: + remaining_timeout = timeout - (_time() - start_time) + cleanup_timeout = max(0.0, remaining_timeout) + elif isinstance(ex, KeyboardInterrupt): + # KeyboardInterrupts are: + # 1. Typically automatically triggered in the child, too. + # Therefore, the child is likely to be already cleaning up + # (if we give it the chance). + # 2. Generated interactively by humans at a keyboard. + # If the child is well-written (exits in response to SIGINT), + # a 1 second timeout is usually enough time for it to clean up. + # Othwerwise, 1 second seems like a tolerable delay in response to Ctrl+C, + # and a resonable price to pay for correct behavior in the well-written case. + cleanup_timeout = 1.0 + else: + # The above reasoning does not apply to arbitrary exceptions, + # so kill the child immediately. + cleanup_timeout = 0.0 + + if cleanup_timeout == 0.0: + raise # Skip cleanup + + # Close streams to/from child + streams = (process.stdin, process.stdout, process.stderr) + for f in filter(None, streams): + try: + f.close() + except: + pass # Ignore EBADF or other errors. + + try: + process.wait(timeout=cleanup_timeout) + except: + # Ignore cleanup errors; + # original exception is always re-raised below + pass + finally: + # Kill the child and re-raise original exception, + # regardless of successful/failed cleanup + process.kill() + process.wait() + raise ex + retcode = process.poll() if check and retcode: raise CalledProcessError(retcode, process.args, diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index fff1b0db1051b6..fa9ce5b694a68d 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1476,6 +1476,82 @@ def test_run_kwargs(self): env=newenv) self.assertEqual(cp.returncode, 33) + def test_run_interrupted_cleanup_succeeds(self): + # In the event of KeyboardInterrupt, run() should allow the + # child time to cleanup (if cleanup_timeout was given). + # In this case, child's cleanup completes before cleanup_timeout runs out. + self._test_run_interrupted(0.2, 1.0) + + def test_run_interrupted_cleanup_timed_out(self): + # In the event of KeyboardInterrupt, run() should allow the + # child time to cleanup (if cleanup_timeout was given). + # In this case, child's cleanup takes too long for the cleanup_timout. + self._test_run_interrupted(3.0, 0.1) + + @unittest.skipIf(mswindows, "cannot use 'ps' command windows") + @unittest.skipIf(threading is None, "threading required") + def _test_run_interrupted(self, cleanup_time, cleanup_timeout): + # In the event of KeyboardInterrupt, run() should allow the + # child time to cleanup (if cleanup_timeout was given). + + parent_pid = os.getpid() + def generate_sigint(): + # Send SIGINT to the current process and its child after a short delay + time.sleep(0.5) + children = self._find_children(parent_pid) + for (pid, command) in children: + if 'python' in command: + os.kill(pid, signal.SIGINT) + os.kill(parent_pid, signal.SIGINT) + + interrupter_thread = threading.Thread(target=generate_sigint) + + tf = tempfile.NamedTemporaryFile(delete=False) + tf.close() + + try: + with self.assertRaises(KeyboardInterrupt): + interrupter_thread.start() + subprocess.run([sys.executable, "-c", + textwrap.dedent(f"""\ + import time + try: + time.sleep(3.0) + except KeyboardInterrupt: + time.sleep({cleanup_time}) + with open('{tf.name}', 'w') as f: + f.write('cleaned up') + """)], + timeout=None, cleanup_timeout=cleanup_timeout, + check=True) + + with open(tf.name, 'r') as tf: + if cleanup_time < cleanup_timeout: + self.assertEqual(tf.read(), 'cleaned up') + else: + # child should have been killed before the cleanup executed + self.assertEqual(tf.read(), '') + finally: + os.unlink(tf.name) + interrupter_thread.join() + + def _find_children(self, parent_pid): + """Return a list of tuples (child_pid, command) + for all direct children of the given parent_id. + (Unix only.) + Note: Output may include the 'ps' command itself. + """ + children = [] + ps_output = subprocess.check_output(['ps', '-o', 'pid,ppid,command']) + for line in ps_output.decode().strip().split('\n')[1:]: + words = line.split() + pid = int(words[0]) + ppid = int(words[1]) + command = ' '.join(words[2:]) + if ppid == parent_pid: + children.append( (pid, command) ) + return children + @unittest.skipIf(mswindows, "POSIX specific tests") class POSIXProcessTestCase(BaseTestCase): diff --git a/Misc/NEWS.d/next/Library/2017-11-05-11-42-56.bpo-25942.o5lBhR.rst b/Misc/NEWS.d/next/Library/2017-11-05-11-42-56.bpo-25942.o5lBhR.rst new file mode 100644 index 00000000000000..eb77606736111a --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-11-05-11-42-56.bpo-25942.o5lBhR.rst @@ -0,0 +1 @@ +Added new parameter, ``cleanup_timeout``, to ``subprocess.run()``