[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

STINNER Victor added the comment:

Oops, I forget to close the issue.

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

Changes by STINNER Victor :


--
resolution: fixed -> 
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

STINNER Victor added the comment:

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.

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Martin Panter]

Martin Panter added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

STINNER Victor added the comment:

> Did you forget to send the patch?

Right :-)

--
keywords: +patch
Added file: http://bugs.python.org/file42448/subprocess_res_warn.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[Martin Panter]

Martin Panter added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26741] subprocess.Popen should emit a ResourceWarning in destructor if the process is still running

[STINNER Victor]

New submission from STINNER Victor:

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

--
components: Library (Lib)
messages: 263281
nosy: haypo, martin.panter, pitrou, serhiy.storchaka
priority: normal
severity: normal
status: open
title: subprocess.Popen should emit a ResourceWarning in destructor if the 
process is still running
type: resource usage
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com