From d16a2e6643f7de043845fc9d24336cb18fef6e70 Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Sun, 5 Nov 2017 09:19:33 -0500 Subject: [PATCH 01/11] subprocess.call() merely calls subprocess.run() --- Lib/subprocess.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index f6d03f89a85788..0b49f6f1c28331 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -263,14 +263,7 @@ def call(*popenargs, timeout=None, **kwargs): retcode = call(["ls", "-l"]) """ - with Popen(*popenargs, **kwargs) as p: - try: - return p.wait(timeout=timeout) - except: - p.kill() - p.wait() - raise - + return run(*popenargs, timeout=timeout, **kwargs).returncode def check_call(*popenargs, **kwargs): """Run command with arguments. Wait for command to complete. If From bebd8d349a8e64dbb01172a76c0c71c3521845f8 Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Sat, 4 Nov 2017 17:16:32 -0400 Subject: [PATCH 02/11] bpo-25942: Added new parameter to run(): cleanup_timeout In the event of a KeyboardInterrupt, the child process is given time to clean up before it is killed by the parent. --- Doc/library/subprocess.rst | 17 +++++++- Lib/subprocess.py | 41 ++++++++++++++++--- Lib/test/test_subprocess.py | 38 +++++++++++++++++ .../2017-11-05-11-42-56.bpo-25942.o5lBhR.rst | 1 + 4 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2017-11-05-11-42-56.bpo-25942.o5lBhR.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 693355cce7f93d..f6c56e375804ca 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 recevies a :exc:`KeyboardInterrupt` 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 :exc:`KeyboardInterrupt` + 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 0b49f6f1c28331..0a3fe75ad09ac5 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -255,15 +255,22 @@ 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: + + run(...).returncode + + (except that the input and check parameters are not supported). - retcode = call(["ls", "-l"]) + See run() for argument details. + + Example: + retcode = call(["ls", "-l"]) """ - return run(*popenargs, timeout=timeout, **kwargs).returncode + return run(*popenargs, timeout=timeout, cleanup_timeout=None, **kwargs).returncode def check_call(*popenargs, **kwargs): """Run command with arguments. Wait for command to complete. If @@ -364,7 +371,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 @@ -379,6 +386,14 @@ 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 recevies a 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 KeyboardInterrupt + will kill the child immediately. If cleanup_timeout is not specified, + then any remaining time from the original timeout is used, or 1.0 if no + original timeout was specified. + 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 @@ -397,6 +412,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) @@ -405,6 +422,20 @@ def run(*popenargs, input=None, timeout=None, check=False, **kwargs): stdout, stderr = process.communicate() raise TimeoutExpired(process.args, timeout, output=stdout, stderr=stderr) + except KeyboardInterrupt: + if cleanup_timeout is None: + cleanup_timeout = 1.0 + if timeout: + remaining_timeout = timeout - (_time() - start_time) + cleanup_timeout = max(0.0, remaining_timeout) + + if cleanup_timeout == 0.0: + raise + try: + process.wait(timeout=cleanup_timeout) + raise + except TimeoutExpired: + raise KeyboardInterrupt except: process.kill() process.wait() diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index fff1b0db1051b6..ce4201af924a47 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1476,6 +1476,44 @@ def test_run_kwargs(self): env=newenv) self.assertEqual(cp.returncode, 33) + @unittest.skipIf(mswindows, "cannot use setpgid, killpg, etc. on windows") + @unittest.skipIf(threading is None, "threading required") + def test_run_interrupted(self): + # In the event of SIGINT, run() should allow the + # child time to cleanup (if cleanup_timeout was given). + + # Change the current process group to avoid + # interrupting our parent process. + original_pgid = os.getpgrp() + os.setpgid(0, 0) + + def interrupt_this_process_group(): + # Send SIGINT to the current process group after a short delay + time.sleep(0.5) + os.killpg(os.getpgrp(), signal.SIGINT) + + tf = tempfile.NamedTemporaryFile(delete=False) + tf.close() + + try: + with self.assertRaises(KeyboardInterrupt) as ex: + threading.Thread(target=interrupt_this_process_group).start() + rc = subprocess.run([sys.executable, "-c", + "import time\n" + "try:\n" + " time.sleep(3.0)\n" + "except KeyboardInterrupt:\n" + " time.sleep(0.1)\n" + f" open('{tf.name}', 'w').write('cleaned up')\n"], + timeout=None, cleanup_timeout=1.0, + check=True) + + with open(tf.name, 'r') as tf: + self.assertEqual(tf.read(), 'cleaned up') + finally: + # Restore original process group + os.setpgid(0, original_pgid) + os.unlink(tf.name) @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()`` From ff62b7b5878eadd3f3f5879197fcb180e0951b87 Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 05:41:45 -0500 Subject: [PATCH 03/11] subprocess.run(): Actually kill the child process if KeyboardInterrupt cleanup takes too long. --- Lib/subprocess.py | 5 +++++ Lib/test/test_subprocess.py | 41 ++++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 0a3fe75ad09ac5..3817184fda06ea 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -430,13 +430,18 @@ def run(*popenargs, input=None, timeout=None, check=False, cleanup_timeout=None, cleanup_timeout = max(0.0, remaining_timeout) if cleanup_timeout == 0.0: + process.kill() + process.wait() raise try: process.wait(timeout=cleanup_timeout) raise except TimeoutExpired: + process.kill() + process.wait() raise KeyboardInterrupt except: + print("Killing!") process.kill() process.wait() raise diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index ce4201af924a47..017ad9ac860994 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1476,10 +1476,23 @@ 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 SIGINT, 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 setpgid, killpg, etc. on windows") @unittest.skipIf(threading is None, "threading required") - def test_run_interrupted(self): - # In the event of SIGINT, run() should allow the + 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). # Change the current process group to avoid @@ -1498,18 +1511,22 @@ def interrupt_this_process_group(): try: with self.assertRaises(KeyboardInterrupt) as ex: threading.Thread(target=interrupt_this_process_group).start() - rc = subprocess.run([sys.executable, "-c", - "import time\n" - "try:\n" - " time.sleep(3.0)\n" - "except KeyboardInterrupt:\n" - " time.sleep(0.1)\n" - f" open('{tf.name}', 'w').write('cleaned up')\n"], - timeout=None, cleanup_timeout=1.0, - check=True) + subprocess.run([sys.executable, "-c", + "import time\n" + "try:\n" + " time.sleep(3.0)\n" + "except KeyboardInterrupt:\n" + f" time.sleep({cleanup_time})\n" + f" open('{tf.name}', 'w').write('cleaned up')\n"], + timeout=None, cleanup_timeout=cleanup_timeout, + check=True) with open(tf.name, 'r') as tf: - self.assertEqual(tf.read(), 'cleaned up') + 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: # Restore original process group os.setpgid(0, original_pgid) From f7280e00427008ec17ca5d3816b5341fabe95de9 Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 05:45:37 -0500 Subject: [PATCH 04/11] subprocess.call(): Fix copy/paste error --- Lib/subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 3817184fda06ea..301c4b22c82534 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -270,7 +270,7 @@ def call(*popenargs, timeout=None, cleanup_timeout=None, **kwargs): Example: retcode = call(["ls", "-l"]) """ - return run(*popenargs, timeout=timeout, cleanup_timeout=None, **kwargs).returncode + 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 From e0d3cbe2af28bb24ebc065a3039258fb0167116e Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 05:47:46 -0500 Subject: [PATCH 05/11] test_subprocess: Removed unused variable --- Lib/test/test_subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 017ad9ac860994..1b7249e27f5d55 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1509,7 +1509,7 @@ def interrupt_this_process_group(): tf.close() try: - with self.assertRaises(KeyboardInterrupt) as ex: + with self.assertRaises(KeyboardInterrupt): threading.Thread(target=interrupt_this_process_group).start() subprocess.run([sys.executable, "-c", "import time\n" From d1a4dac7cfd2fe990dc2410ff4980cb34979d55b Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 05:50:03 -0500 Subject: [PATCH 06/11] test_subprocess: Call join() on helper thread --- Lib/test/test_subprocess.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 1b7249e27f5d55..c7f2a430e09984 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1504,13 +1504,14 @@ def interrupt_this_process_group(): # Send SIGINT to the current process group after a short delay time.sleep(0.5) os.killpg(os.getpgrp(), signal.SIGINT) + interrupter_thread = threading.Thread(target=interrupt_this_process_group) tf = tempfile.NamedTemporaryFile(delete=False) tf.close() - + try: with self.assertRaises(KeyboardInterrupt): - threading.Thread(target=interrupt_this_process_group).start() + interrupter_thread.start() subprocess.run([sys.executable, "-c", "import time\n" "try:\n" @@ -1531,6 +1532,7 @@ def interrupt_this_process_group(): # Restore original process group os.setpgid(0, original_pgid) os.unlink(tf.name) + interrupter_thread.join() @unittest.skipIf(mswindows, "POSIX specific tests") class POSIXProcessTestCase(BaseTestCase): From b83180150f5d42e2e108bb348b3ceaa21f845b7b Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 05:54:39 -0500 Subject: [PATCH 07/11] test_subprocess: Explicitly close file during cleanup test. --- Lib/test/test_subprocess.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index c7f2a430e09984..4366f90f2d6fda 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1518,7 +1518,8 @@ def interrupt_this_process_group(): " time.sleep(3.0)\n" "except KeyboardInterrupt:\n" f" time.sleep({cleanup_time})\n" - f" open('{tf.name}', 'w').write('cleaned up')\n"], + f" with open('{tf.name}', 'w') as f:\n" + " f.write('cleaned up')\n"], timeout=None, cleanup_timeout=cleanup_timeout, check=True) From 22b030acd6ef0f367c1f7a552b9c6f1cdfc829de Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 05:58:35 -0500 Subject: [PATCH 08/11] subprocess: Remove print() --- Lib/subprocess.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 301c4b22c82534..f290df8d3128a4 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -441,7 +441,6 @@ def run(*popenargs, input=None, timeout=None, check=False, cleanup_timeout=None, process.wait() raise KeyboardInterrupt except: - print("Killing!") process.kill() process.wait() raise From 2e990ea03b3395e02dc8c9d295f4efeb320922af Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 06:07:25 -0500 Subject: [PATCH 09/11] Fix whitespace formatting --- Lib/test/test_subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 4366f90f2d6fda..2a4a7b59dac5d7 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1508,7 +1508,7 @@ def interrupt_this_process_group(): tf = tempfile.NamedTemporaryFile(delete=False) tf.close() - + try: with self.assertRaises(KeyboardInterrupt): interrupter_thread.start() From b7971615b3f1e382b0c6d0f6a22b73485020c3cc Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 15:42:49 -0500 Subject: [PATCH 10/11] test_subprocess: Simulate Ctrl+C via separate SIGINT signals to parent and child, rather than playing with process groups. (As requested in review.) --- Lib/test/test_subprocess.py | 60 ++++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 2a4a7b59dac5d7..fa9ce5b694a68d 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1476,7 +1476,6 @@ 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). @@ -1484,27 +1483,28 @@ def test_run_interrupted_cleanup_succeeds(self): self._test_run_interrupted(0.2, 1.0) def test_run_interrupted_cleanup_timed_out(self): - # In the event of SIGINT, run() should allow the + # 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 setpgid, killpg, etc. on windows") + @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). - # Change the current process group to avoid - # interrupting our parent process. - original_pgid = os.getpgrp() - os.setpgid(0, 0) - - def interrupt_this_process_group(): - # Send SIGINT to the current process group after a short delay + parent_pid = os.getpid() + def generate_sigint(): + # Send SIGINT to the current process and its child after a short delay time.sleep(0.5) - os.killpg(os.getpgrp(), signal.SIGINT) - interrupter_thread = threading.Thread(target=interrupt_this_process_group) + 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() @@ -1513,13 +1513,15 @@ def interrupt_this_process_group(): with self.assertRaises(KeyboardInterrupt): interrupter_thread.start() subprocess.run([sys.executable, "-c", - "import time\n" - "try:\n" - " time.sleep(3.0)\n" - "except KeyboardInterrupt:\n" - f" time.sleep({cleanup_time})\n" - f" with open('{tf.name}', 'w') as f:\n" - " f.write('cleaned up')\n"], + 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) @@ -1530,11 +1532,27 @@ def interrupt_this_process_group(): # child should have been killed before the cleanup executed self.assertEqual(tf.read(), '') finally: - # Restore original process group - os.setpgid(0, original_pgid) 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): From 220b40f5613377a908e9bfe863def6179f86a3ef Mon Sep 17 00:00:00 2001 From: Stuart Berg Date: Mon, 6 Nov 2017 15:34:23 -0500 Subject: [PATCH 11/11] subprocess.run(): Changes to exception handling as requested in review: - Cleanup child in response to all exceptions, not just KeyboardInterrupt - Close child stdin, stdout, stderr during cleanup - Slightly simplify control flow - Reduce default cleanup timeout to 0.0 for all exceptions except KeyboardInterrupt (for review: more discussion needed) --- Doc/library/subprocess.rst | 14 +++---- Lib/subprocess.py | 79 ++++++++++++++++++++++++++------------ 2 files changed, 61 insertions(+), 32 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index f6c56e375804ca..2451993dd65994 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -76,13 +76,13 @@ 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 recevies a :exc:`KeyboardInterrupt` 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 :exc:`KeyboardInterrupt` - 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. + 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:: diff --git a/Lib/subprocess.py b/Lib/subprocess.py index f290df8d3128a4..e51c98da7260e2 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -270,7 +270,8 @@ def call(*popenargs, timeout=None, cleanup_timeout=None, **kwargs): Example: retcode = call(["ls", "-l"]) """ - return run(*popenargs, timeout=timeout, cleanup_timeout=cleanup_timeout, **kwargs).returncode + 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 @@ -386,13 +387,16 @@ def run(*popenargs, input=None, timeout=None, check=False, cleanup_timeout=None, If timeout is given, and the process takes too long, a TimeoutExpired exception will be raised. - If the current process recevies a KeyboardInterrupt and + 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 KeyboardInterrupt - will kill the child immediately. If cleanup_timeout is not specified, - then any remaining time from the original timeout is used, or 1.0 if no - original timeout was specified. + 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 @@ -422,28 +426,53 @@ def run(*popenargs, input=None, timeout=None, check=False, cleanup_timeout=None, stdout, stderr = process.communicate() raise TimeoutExpired(process.args, timeout, output=stdout, stderr=stderr) - except KeyboardInterrupt: - if cleanup_timeout is None: - cleanup_timeout = 1.0 - if timeout: - remaining_timeout = timeout - (_time() - start_time) - cleanup_timeout = max(0.0, remaining_timeout) - - if cleanup_timeout == 0.0: - process.kill() - process.wait() - raise + except BaseException as ex: try: - process.wait(timeout=cleanup_timeout) - raise - except TimeoutExpired: + 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 KeyboardInterrupt - except: - process.kill() - process.wait() - raise + raise ex + retcode = process.poll() if check and retcode: raise CalledProcessError(retcode, process.args,