bpo-37380: subprocess: skip _cleanup and __del__ on win by efiop · Pull Request #14360 · python/cpython

efiop · 2019-06-25T01:09:40Z

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

https://bugs.python.org/issue37380

the-knights-who-say-ni · 2019-06-25T01:09:43Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

efiop · 2019-06-25T02:37:00Z

Thanks for the review @eryksun ! Addressed your comments in the patch.

vstinner · 2019-06-25T10:25:17Z

That's wrong: you must also wait for process completion on Windows.

Hi @vstinner ! Thanks for the review! On Windows we just need to close the handle when we no longer need it, so that the kernel can free it. CloseHandle is called as a __del__ in class Handle defined above, so there are no additional actions required here, and so we can just skip the whole __del__ logic here. From [1]

When you no longer need these handles, close them by using the CloseHandle function.

[1] https://docs.microsoft.com/en-us/windows/desktop/ProcThread/creating-processes

@vstinner If we're in __del__, presumably that means that nobody has a reference to the Popen object, right? In that case, we can let it be cleaned up. Closing the handle does not terminate the process - it'll just keep running and we can't get back to it through that particular handle (but we could use OpenProcess with the PID to get a new handle later).

bedevere-bot · 2019-06-25T10:26:23Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

zooba · 2019-06-25T15:39:44Z

@vstinner If we're in __del__, presumably that means that nobody has a reference to the Popen object, right? In that case, we can let it be cleaned up. Closing the handle does not terminate the process - it'll just keep running and we can't get back to it through that particular handle (but we could use OpenProcess with the PID to get a new handle later).

vstinner · 2019-06-25T20:41:43Z

Steve:

@vstinner If we're in del, presumably that means that nobody has a reference to the Popen object, right? In that case, we can let it be cleaned up. Closing the handle does not terminate the process - it'll just keep running and we can't get back to it through that particular handle (but we could use OpenProcess with the PID to get a new handle later).

My worry is that a badly written program may "leak" programs which continue to run in the background which can cause different issues. For example, you may not be able to remove the directory used by these background tasks since they have a handle open on these directories. It can cause various different issues.

The idea of adding a detatch() method to Popen was proposed once but didn't convince enough people to be implemented: https://bugs.python.org/issue27068

The purpose of the ResourceWarning is to help the developer to write more reliable softwares. It's just a matter of a handle which remains open longer than expected.

I'm talking about this warning:

$ cat x.py 
import subprocess, sys, gc
proc = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)'])
proc = None
gc.collect()

$ python3 -Wd x.py 
/usr/lib64/python3.7/subprocess.py:858: ResourceWarning: subprocess 12920 is still running
  ResourceWarning, source=self)
ResourceWarning: Enable tracemalloc to get the object allocation traceback

I would like to also get this warning on Windows.

efiop · 2019-06-25T21:12:45Z

Guys, big thanks for the reviews! I've adjusted my patch, so that it only re-defines _active and _cleanup for windows. I didn't notice right away that __del__ is actually already able to deal with _active being None, so that we don't need to introduce any changes to __del__ at all 🎉 So we will still get the ResourceWarning for Windows.

vstinner · 2019-06-25T22:50:18Z

I don't see where this PR explicitly closes the handle: https://bugs.python.org/issue37380#msg346568

eryksun · 2019-06-25T23:52:18Z

As to Popen.__del__, I don't understand why we force scripts to always have to poll() in order to avoid a resource warning when warnings are enabled. Why can't we instead check whether self._internal_poll() is None instead of self.returncode is None, in order to quietly handle cases where the program has exited already? For example:

>>> p = subprocess.Popen('python -c ""')
>>> del p
C:\Program Files\Python37\lib\subprocess.py:858: ResourceWarning: subprocess 3180 is still running
  ResourceWarning, source=self)
ResourceWarning: Enable tracemalloc to get the object allocation traceback

The process exited. It is not running. The warning is wrong.

eryksun · 2019-06-26T00:00:33Z

In a related issue to waiting on the child process, if someone wants to open an easy issue for this, we should not be creating self._waitpid_lock = threading.Lock() in Windows. It isn't used.

vstinner · 2019-06-26T00:56:21Z

As to Popen.del, I don't understand why we force scripts to always have to poll() in order to avoid a resource warning when warnings are enabled. Why can't we instead check whether self._internal_poll() is None instead of self.returncode is None, in order to quietly handle cases where the program has exited already?

The purpose is to promote to handle explicitly resources, rather than relying on Python garbage collector to magically handle them: the garbage collection behaves differently on PyPy, and may not collect objects "immediately" depending if they are part of a reference cycle or not.

subprocess.Popen should "release" its resources when the process exit: close pipes, "close" the child processes, etc. I suggest to use "with popen: ..." since exit() closes pipes and waits until the child process completes.

vstinner · 2019-06-26T00:57:20Z

In the case of Windows, I would expect that the handle can be closed as soon as the process completes. But it seems to be the case now: https://bugs.python.org/issue37380#msg346568

@eryksun

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>

efiop · 2019-06-26T18:08:31Z

Adjusted the _cleanup comment. Please take a look.

eryksun · 2019-06-26T21:54:36Z

+    # 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.
+    #


LGTM.

FYI, if bpo37410 is resolved, we'll also be explicitly closing _handle when returncode is set. This would be a significant change. Currently we can assume _handle is valid, but with the proposed change we'll have to acquire a lock and check whether self.returncode is None before code that sets it in _internal_poll, _wait, and terminate.

miss-islington · 2019-09-05T16:00:40Z

Thanks @efiop for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2019-09-05T16:00:40Z

Thanks @efiop for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2019-09-05T16:00:51Z

GH-15706 is a backport of this pull request to the 3.7 branch.

@eryksun

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 <ruslan@iterative.ai> (cherry picked from commit 042821a) Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

bedevere-bot · 2019-09-05T16:00:57Z

GH-15707 is a backport of this pull request to the 3.8 branch.

@eryksun

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 <ruslan@iterative.ai> (cherry picked from commit 042821a) Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

@eryksun

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 <ruslan@iterative.ai> (cherry picked from commit 042821a) Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

@eryksun

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 <ruslan@iterative.ai> (cherry picked from commit 042821a) Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>

@eryksun

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>

@eryksun

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>

To better reflect python/cpython#14360

efiop requested a review from gpshead as a code owner June 25, 2019 01:09

the-knights-who-say-ni added the CLA not signed label Jun 25, 2019

bedevere-bot added the awaiting review label Jun 25, 2019

efiop force-pushed the fix-bpo-37380 branch from ee060b4 to b489caa Compare June 25, 2019 01:16

eryksun reviewed Jun 25, 2019

View reviewed changes

Comment thread Misc/NEWS.d/next/Windows/2019-06-25-04-15-22.bpo-37380.tPxjuz.rst Outdated

eryksun reviewed Jun 25, 2019

View reviewed changes

Comment thread Lib/subprocess.py Outdated

efiop force-pushed the fix-bpo-37380 branch 2 times, most recently from 636dfcf to f59cad6 Compare June 25, 2019 02:18

vstinner requested changes Jun 25, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jun 25, 2019

efiop force-pushed the fix-bpo-37380 branch 2 times, most recently from eb04016 to 80d8568 Compare June 25, 2019 12:50

zooba reviewed Jun 25, 2019

View reviewed changes

efiop force-pushed the fix-bpo-37380 branch from 80d8568 to 66b7f97 Compare June 25, 2019 21:12

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jun 25, 2019

vstinner requested changes Jun 25, 2019

View reviewed changes

Comment thread Lib/test/test_subprocess.py Outdated

Comment thread Lib/test/test_subprocess.py Outdated

Comment thread Lib/test/test_subprocess.py Outdated

Comment thread Lib/subprocess.py Outdated

efiop force-pushed the fix-bpo-37380 branch from 66b7f97 to fc92f9a Compare June 25, 2019 23:13

eryksun reviewed Jun 25, 2019

View reviewed changes

Comment thread Lib/subprocess.py Outdated

eryksun reviewed Jun 26, 2019

View reviewed changes

Comment thread Lib/test/test_subprocess.py Outdated

efiop force-pushed the fix-bpo-37380 branch from fc92f9a to c44c9c9 Compare June 26, 2019 18:07

eryksun reviewed Jun 26, 2019

View reviewed changes

vstinner merged commit 042821a into python:master Jun 28, 2019

bedevere-bot removed the awaiting changes label Jun 28, 2019

efiop deleted the fix-bpo-37380 branch June 28, 2019 16:12

efiop mentioned this pull request Jun 28, 2019

bpo-37410: subprocess closes the process handle when done #14391

Closed

zooba added needs backport to 3.7 labels Sep 5, 2019

bedevere-bot removed the needs backport to 3.7 label Sep 5, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 5, 2019

chyunsu3 mentioned this pull request Oct 6, 2019

[CI] Run tests on Windows dmlc/treelite#101

Merged

efiop mentioned this pull request Oct 31, 2020

tests: set subprocess._active to None treeverse/dvc#4818

Merged

2 tasks

efiop added a commit to treeverse/dvc that referenced this pull request Nov 1, 2020

tests: set subprocess._active to None

df19ef3

To better reflect python/cpython#14360

efiop added a commit to treeverse/dvc that referenced this pull request Nov 3, 2020

tests: set subprocess._active to None (#4818)

2225764

To better reflect python/cpython#14360

Uh oh!

Uh oh!

Conversation

efiop commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jun 25, 2019

Uh oh!

Uh oh!

Uh oh!

efiop commented Jun 25, 2019

Uh oh!

vstinner Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

efiop Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zooba Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-bot commented Jun 25, 2019

Uh oh!

Uh oh!

Uh oh!

zooba Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jun 25, 2019

Uh oh!

efiop commented Jun 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Jun 25, 2019

Uh oh!

Uh oh!

eryksun commented Jun 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eryksun commented Jun 26, 2019

Uh oh!

Uh oh!

vstinner commented Jun 26, 2019

Uh oh!

vstinner commented Jun 26, 2019

Uh oh!

efiop commented Jun 26, 2019

Uh oh!

eryksun Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Sep 5, 2019

Uh oh!

miss-islington commented Sep 5, 2019

Uh oh!

bedevere-bot commented Sep 5, 2019

Uh oh!

bedevere-bot commented Sep 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

efiop commented Jun 25, 2019 •

edited

Loading

efiop Jun 25, 2019 •

edited

Loading

eryksun commented Jun 25, 2019 •

edited

Loading