bpo-37380: subprocess: don't use _active on win (GH-14360) (GH-15706)

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/issue37380GH-msg346333
[2] https://bugs.python.org/issue36067GH-msg336262

Signed-off-by: Ruslan Kuprieiev <[email protected]>
(cherry picked from commit 042821a)

Co-authored-by: Ruslan Kuprieiev <[email protected]>

Author: 2 people

Lib/subprocess.py

@@ -217,22 +217,38 @@ def __repr__(self):
         __str__ = __repr__
 
 
-# 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

Lib/test/test_subprocess.py

@@ -59,10 +59,14 @@ def setUp(self):
         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()
 
@@ -2622,8 +2626,12 @@ def test_zombie_fast_process_del(self):
         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
@@ -2644,8 +2652,12 @@ def test_leak_fast_process_del_killed(self):
             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
@@ -2657,7 +2669,11 @@ def test_leak_fast_process_del_killed(self):
                 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")

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.