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@iterative.ai>
This commit is contained in:
Ruslan Kuprieiev 2019-06-28 19:12:16 +03:00 committed by Victor Stinner
parent 64580da331
commit 042821ae3c
3 changed files with 58 additions and 24 deletions

View File

@ -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 = []
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():
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
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

View File

@ -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")

View File

@ -0,0 +1,2 @@
Don't collect unfinished processes with ``subprocess._active`` on Windows to
cleanup later. Patch by Ruslan Kuprieiev.