From 042821ae3cf537e01963c9ec85d1a454d921e826 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 28 Jun 2019 19:12:16 +0300 Subject: [PATCH] bpo-37380: subprocess: don't use _active on win (GH-14360) As noted by @eryksun in [1] and [2], using _cleanup and _active(in __del__) is not necessary on Windows, since: > Unlike Unix, a process in Windows doesn't have to be waited on by > its parent to avoid a zombie. Keeping the handle open will actually > create a zombie until the next _cleanup() call, which may be never > if Popen() isn't called again. This patch simply defines `subprocess._active` as `None`, for which we already have the proper logic in place in `subprocess.Popen.__del__`, that prevents it from trying to append the process to the `_active`. This patch also defines `subprocess._cleanup` as a noop for Windows. [1] https://bugs.python.org/issue37380#msg346333 [2] https://bugs.python.org/issue36067#msg336262 Signed-off-by: Ruslan Kuprieiev --- Lib/subprocess.py | 48 ++++++++++++------- Lib/test/test_subprocess.py | 34 +++++++++---- .../2019-06-25-04-15-22.bpo-37380.tPxjuz.rst | 2 + 3 files changed, 59 insertions(+), 25 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2019-06-25-04-15-22.bpo-37380.tPxjuz.rst diff --git a/Lib/subprocess.py b/Lib/subprocess.py index c0bda96cbc..5bbeba47a3 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -218,22 +218,38 @@ else: _PopenSelector = selectors.SelectSelector -# This lists holds Popen instances for which the underlying process had not -# exited at the time its __del__ method got called: those processes are wait()ed -# for synchronously from _cleanup() when a new Popen object is created, to avoid -# zombie processes. -_active = [] - -def _cleanup(): - for inst in _active[:]: - res = inst._internal_poll(_deadstate=sys.maxsize) - if res is not None: - try: - _active.remove(inst) - except ValueError: - # This can happen if two threads create a new Popen instance. - # It's harmless that it was already removed, so ignore. - pass +if _mswindows: + # On Windows we just need to close `Popen._handle` when we no longer need + # it, so that the kernel can free it. `Popen._handle` gets closed + # implicitly when the `Popen` instance is finalized (see `Handle.__del__`, + # which is calling `CloseHandle` as requested in [1]), so there is nothing + # for `_cleanup` to do. + # + # [1] https://docs.microsoft.com/en-us/windows/desktop/ProcThread/ + # creating-processes + _active = None + + def _cleanup(): + pass +else: + # This lists holds Popen instances for which the underlying process had not + # exited at the time its __del__ method got called: those processes are + # wait()ed for synchronously from _cleanup() when a new Popen object is + # created, to avoid zombie processes. + _active = [] + + def _cleanup(): + if _active is None: + return + for inst in _active[:]: + res = inst._internal_poll(_deadstate=sys.maxsize) + if res is not None: + try: + _active.remove(inst) + except ValueError: + # This can happen if two threads create a new Popen instance. + # It's harmless that it was already removed, so ignore. + pass PIPE = -1 STDOUT = -2 diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 97d21904b9..6b8acb258e 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -59,10 +59,14 @@ class BaseTestCase(unittest.TestCase): support.reap_children() def tearDown(self): - for inst in subprocess._active: - inst.wait() - subprocess._cleanup() - self.assertFalse(subprocess._active, "subprocess._active not empty") + if not mswindows: + # subprocess._active is not used on Windows and is set to None. + for inst in subprocess._active: + inst.wait() + subprocess._cleanup() + self.assertFalse( + subprocess._active, "subprocess._active not empty" + ) self.doCleanups() support.reap_children() @@ -2679,8 +2683,12 @@ class POSIXProcessTestCase(BaseTestCase): with support.check_warnings(('', ResourceWarning)): p = None - # check that p is in the active processes list - self.assertIn(ident, [id(o) for o in subprocess._active]) + if mswindows: + # subprocess._active is not used on Windows and is set to None. + self.assertIsNone(subprocess._active) + else: + # check that p is in the active processes list + self.assertIn(ident, [id(o) for o in subprocess._active]) def test_leak_fast_process_del_killed(self): # Issue #12650: on Unix, if Popen.__del__() was called before the @@ -2701,8 +2709,12 @@ class POSIXProcessTestCase(BaseTestCase): p = None os.kill(pid, signal.SIGKILL) - # check that p is in the active processes list - self.assertIn(ident, [id(o) for o in subprocess._active]) + if mswindows: + # subprocess._active is not used on Windows and is set to None. + self.assertIsNone(subprocess._active) + else: + # check that p is in the active processes list + self.assertIn(ident, [id(o) for o in subprocess._active]) # let some time for the process to exit, and create a new Popen: this # should trigger the wait() of p @@ -2714,7 +2726,11 @@ class POSIXProcessTestCase(BaseTestCase): pass # p should have been wait()ed on, and removed from the _active list self.assertRaises(OSError, os.waitpid, pid, 0) - self.assertNotIn(ident, [id(o) for o in subprocess._active]) + if mswindows: + # subprocess._active is not used on Windows and is set to None. + self.assertIsNone(subprocess._active) + else: + self.assertNotIn(ident, [id(o) for o in subprocess._active]) def test_close_fds_after_preexec(self): fd_status = support.findfile("fd_status.py", subdir="subprocessdata") diff --git a/Misc/NEWS.d/next/Windows/2019-06-25-04-15-22.bpo-37380.tPxjuz.rst b/Misc/NEWS.d/next/Windows/2019-06-25-04-15-22.bpo-37380.tPxjuz.rst new file mode 100644 index 0000000000..facce27954 --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2019-06-25-04-15-22.bpo-37380.tPxjuz.rst @@ -0,0 +1,2 @@ +Don't collect unfinished processes with ``subprocess._active`` on Windows to +cleanup later. Patch by Ruslan Kuprieiev. -- 2.40.0