[Python-checkins] bpo-37380: subprocess: don't use _active on win (GH-14360)
Victor Stinner
webhook-mailer at python.org
Fri Jun 28 12:12:21 EDT 2019
https://github.com/python/cpython/commit/042821ae3cf537e01963c9ec85d1a454d921e826
commit: 042821ae3cf537e01963c9ec85d1a454d921e826
branch: master
author: Ruslan Kuprieiev <kupruser at gmail.com>
committer: Victor Stinner <vstinner at redhat.com>
date: 2019-06-28T18:12:16+02:00
summary:
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 <ruslan at iterative.ai>
files:
A Misc/NEWS.d/next/Windows/2019-06-25-04-15-22.bpo-37380.tPxjuz.rst
M Lib/subprocess.py
M Lib/test/test_subprocess.py
diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index c0bda96cbc03..5bbeba47a374 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -218,22 +218,38 @@ def __repr__(self):
_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 97d21904b9ce..6b8acb258ee3 100644
--- a/Lib/test/test_subprocess.py
+++ b/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()
@@ -2679,8 +2683,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
@@ -2701,8 +2709,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
@@ -2714,7 +2726,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")
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 000000000000..facce27954a6
--- /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.
More information about the Python-checkins
mailing list