classification
Title: subprocess.Popen should emit a ResourceWarning in destructor if the process is still running
Type: resource usage Stage:
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, martin.panter, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2016-04-12 22:49 by haypo, last changed 2017-01-06 09:50 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_res_warn.patch haypo, 2016-04-13 00:20 review
Messages (15)
msg263281 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-04-12 22:49
A subprocess.Popen object contains many resources: pipes, a child process (its pid, and an handle on Windows), etc.

IMHO it's not safe to rely on the destructor to release all resources. I would prefer to release resources explicitly. For example, use proc.wait() or "with proc:".

Attached patch emits a ResourceWarning in Popen destructor if the status of the child process was not read yet.

The patch changes also _execute_child() to set the returncode on error, if the child process raised a Python exception. It avoids to emit a ResourceWarning on this case.

The patch fixes also unit tests to release explicitly resources. self.addCleanup(p.stdout.close) is not enough: use "with proc:" instead.

TODO: fix also the Windows implementation of _execute_child().
msg263285 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-12 23:54
Did you forget to send the patch?

One potential problem is how to provide for people who really want to let the child continue to run in the background or as a daemon without waiting for it, even if the parent exits. Perhaps a special method proc.detach() or whatever?
msg263287 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-04-13 00:20
> Did you forget to send the patch?

Right :-)
msg263288 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-04-13 00:29
> One potential problem is how to provide for people who really want to let the child continue to run in the background or as a daemon without waiting for it, even if the parent exits. Perhaps a special method proc.detach() or whatever?

Maybe my heuristic to decide if ResourceWarning must be emitted is wrong.

If stdout and/or stderr is redirected to a pipe and the process is still alive when the destructor is called, it sounds more likely like a bug, because it's better to explicitly close these pipes.

If no stream is redirected, yeah, it's ok to pass the pid to a different function which will handle the child process. The risk here is not never called waitpid() to read the child exit status and so create zombi processes.

For daemons, I disagree: the daemon must use double fork, so the parent will quickly see its direct child process to exit. Ignoring the status of the first child status is a bug (we must call waitpid().

I have to think about the detach() idea and check if some applications use it, or even some parts of the stdlib.

Note: The ResourceWarning idea comes from asyncio.subprocess transport which also raises a ResourceWarning. I also had the idea when I read the issue #25942 and the old issue #12494.
msg263296 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-13 02:36
I think the basic idea of adding the warning is good.

I think this might be a bit like open(closefd=True) and socket.detach(). Normally, it is a bug not to close a file or socket, but the API is flexible and has a way to bypass this.

Perhaps the term “daemon” means something very specific that is not relevant. But as another example, how would you implement the “bg” and “fg” commands of a Unix shell with subprocess.Popen? The Python parent may need to exit cleanly if the child is still running in the background.

I am not really familiar with it, but perhaps the webbrowser.BackgroundBrowser may be a use case for detach().
msg265930 - (view) Author: Roundup Robot (python-dev) Date: 2016-05-20 10:48
New changeset b122011b139e by Victor Stinner in branch 'default':
Use "with popen:" in test_subprocess
https://hg.python.org/cpython/rev/b122011b139e

New changeset 4c02b983bd5f by Victor Stinner in branch 'default':
Issue #26741: POSIX implementation of subprocess.Popen._execute_child() now
https://hg.python.org/cpython/rev/4c02b983bd5f

New changeset b7f3494deb2c by Victor Stinner in branch 'default':
subprocess now emits a ResourceWarning warning
https://hg.python.org/cpython/rev/b7f3494deb2c
msg265931 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-05-20 10:49
Martin Panter:
> I think the basic idea of adding the warning is good.

Cool.

I pushed an enhanced version of my patch:

* I fixed _execute_child() to set correctly the returncode attribute
* I splitted the patch into multiple commits and I documented the doc
* I fixed test_subprocess on Windows


Me:
> TODO: fix also the Windows implementation of _execute_child().

subprocess.py on Windows is correct, but I had to fix more unit tests specific to Windows in test_subproces.py.


Martin Panter:
> One potential problem is how to provide for people who really want to let the child continue to run in the background or as a daemon without waiting for it, even if the parent exits. Perhaps a special method proc.detach() or whatever?

While I'm not convinced that the use case exists nor that it's safe to delegate the management of the subprocess to a different object after the Popen object is destroyed, I opened the issue #27068 to discuss this feature enhancement.
msg265933 - (view) Author: Roundup Robot (python-dev) Date: 2016-05-20 11:09
New changeset 72946937536e by Victor Stinner in branch '3.5':
asyncio: fix ResourceWarning related to subprocesses
https://hg.python.org/cpython/rev/72946937536e

New changeset 911f398c6396 by Victor Stinner in branch 'default':
Merge 3.5 (issue #26741)
https://hg.python.org/cpython/rev/911f398c6396
msg265937 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-05-20 12:03
I tried out your code on the webbrowser module, and it does raise a warning:

>>> import webbrowser
>>> b = webbrowser.get("chromium")
>>> b.open("https://bugs.python.org/issue26741")
/home/proj/python/cpython/Lib/subprocess.py:1011: ResourceWarning: running subprocess <subprocess.Popen object at 0x7f1ef31c90c0>
  source=self)
True

At this point, the Python interpreter has a child process “chromium” which is left as a zombie when it exits. I guess the easiest solution (at least for Unix) would be to spawn an intermediate launcher process that exited after launching the web browser process.
msg265940 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-05-20 12:24
> At this point, the Python interpreter has a child process “chromium” which is left as a zombie when it exits.

The new ResourceWarning is the confirmation that there is an issue. Creating a zombi process is not a good idea :-)

I opened the issue #27069 to fix this bug.
msg272532 - (view) Author: Roundup Robot (python-dev) Date: 2016-08-12 12:18
New changeset f78a682a3515 by Martin Panter in branch '3.5':
Issue #26741: Clean up subprocess.Popen object in test_poll
https://hg.python.org/cpython/rev/f78a682a3515

New changeset 7ff3ce0dfd45 by Martin Panter in branch 'default':
Issue #26741: Merge ResourceWarning fixes from 3.5
https://hg.python.org/cpython/rev/7ff3ce0dfd45
msg272534 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-08-12 12:20
Martin: why don't you use "with"?
msg272535 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-12 12:25
No super important reason, just to avoid indenting the code. This is a medium-sized test function, with a few long lines already. And if you keep the indentation the same, it makes it easier to port other changes to Python 2, look through the repository history etc.
msg272931 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-08-17 12:46
Oops, I forget to close the issue.
msg284803 - (view) Author: Roundup Robot (python-dev) Date: 2017-01-06 09:50
New changeset b14a1e81c34a by Victor Stinner in branch '3.6':
Fix subprocess.Popen.__del__() fox Python shutdown
https://hg.python.org/cpython/rev/b14a1e81c34a
History
Date User Action Args
2017-01-06 09:50:28python-devsetmessages: + msg284803
2016-08-17 12:46:23hayposetstatus: open -> closed
resolution: fixed
messages: + msg272931
2016-08-12 12:25:18martin.pantersetmessages: + msg272535
2016-08-12 12:20:05hayposetmessages: + msg272534
2016-08-12 12:18:38python-devsetmessages: + msg272532
2016-05-20 12:24:37hayposetmessages: + msg265940
2016-05-20 12:03:50hayposetstatus: closed -> open
resolution: fixed -> (no value)
2016-05-20 12:03:10martin.pantersetmessages: + msg265937
2016-05-20 11:09:03python-devsetmessages: + msg265933
2016-05-20 10:49:12hayposetstatus: open -> closed
resolution: fixed
messages: + msg265931
2016-05-20 10:48:09python-devsetnosy: + python-dev
messages: + msg265930
2016-04-13 02:36:09martin.pantersetmessages: + msg263296
2016-04-13 00:29:44hayposetmessages: + msg263288
2016-04-13 00:20:36hayposetfiles: + subprocess_res_warn.patch
keywords: + patch
messages: + msg263287
2016-04-12 23:54:22martin.pantersetmessages: + msg263285
2016-04-12 22:49:07haypocreate