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().

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?

> Did you forget to send the patch?

Right :-)

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

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().

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

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.

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

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.

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

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

Martin: why don't you use "with"?

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.

Oops, I forget to close the issue.

New changeset b14a1e81c34a by Victor Stinner in branch '3.6':
Fix subprocess.Popen.__del__() fox Python shutdown
https://hg.python.org/cpython/rev/b14a1e81c34a