Repository navigation
Add a detach() method to subprocess.Popen #71255
Description
| BPO | 27068 |
|---|---|
| Nosy | @vstinner, @vadmium |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2017-11-24.01:34:39.161>
created_at = <Date 2016-05-20.10:19:44.154>
labels = ['type-feature', 'library']
title = 'Add a detach() method to subprocess.Popen'
updated_at = <Date 2017-11-24.01:34:39.159>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2017-11-24.01:34:39.159>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-11-24.01:34:39.161>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2016-05-20.10:19:44.154>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 27068
keywords = []
message_count = 9.0
messages = ['265929', '265934', '265941', '265942', '265943', '284933', '297101', '297972', '306874']
nosy_count = 2.0
nosy_names = ['vstinner', 'martin.panter']
pr_nums = []
priority = 'normal'
resolution = 'out of date'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue27068'
versions = ['Python 3.6']
Activity
vstinner commentedon May 20, 2016
The issue bpo-26741 modified the subprocess.Popen destructor to emit a ResourceWarning if the child process is still running.
According to Martin Panter, it can be deliberate to let the subprocess running if the management of the subprocess is delegated to a different object. I'm not convinced that it's safe to do that, but I chose to open this issue to discuss the feature.
My fear is that subprocess.Popen handles many private objects required by the subprocess, not only its pid. For example, on Windows Popen stores the handle of the subprocess. Popen also contains pipe objects when stdin, stdout and/or stderr is redirected.
I guess that detach() makes sense when the developer knows what he/she is doing and knows how to handle all resources attached to a subprocess.
vstinner commentedon May 20, 2016
Hum, I had to patch asyncio to avoid a ResourceWarning because asyncio polls the status of the child process using a child watcher, whereas the Popen object is not aware of the child watcher:
New changeset 72946937536e by Victor Stinner in branch '3.5':
asyncio: fix ResourceWarning related to subprocesses
https://hg.python.org/cpython/rev/72946937536e
The asyncio child watcher uses os.waitpid() on a specific pid or wait for the exit of any child process depending on the chosen implemetation. The fast watcher implementations uses a signal handler on the signal SIGCHLD.
vadmium commentedon May 20, 2016
One potential way to fix bpo-27069 (leave a webbrowser running in the background) might be to use a detach() method. Pseudocode:
vstinner commentedon May 20, 2016
Martin Panter:
Your code is a workaround to hide the warning, it doesn't fix the bug
(creating a zombi process).
vadmium commentedon May 20, 2016
In this example, I think we want to drop the zombie so the “init” process will call wait() on it. My pseudocode is similar to your fork() suggestion in bpo-27069. Fork returns the child’s PID, but we would ignore the PID and exit the intermediate parent process instead.
vadmium commentedon Jan 7, 2017
The user can access pipes and close them directly, or keep their own reference. I don’t think detach() should touch pipes, and __exit__() should probably continue to close them. Maybe call the method detach_pid() if that makes it clearer that pipes are unaffected.
For the Windows process handle, I suggest detach() should close it. I believe this would make Windows work like Unix when you set SIGCHLD to automatically reap children.
vstinner commentedon Jun 28, 2017
@martin Panter: Do you still want this feature? Any progress on bpo-27069?
vadmium commentedon Jul 8, 2017
Personally, I haven’t needed this feature. But I still think it may be a decent solution for the “webbrowser” module, potentially your “asyncio” problem, and other cases that could now trigger an unwanted ResourceWarning.
vstinner commentedon Nov 24, 2017
Sorry, but I'm not sure that it's a good idea to add a detach() method. It's hard to follow who owns all resources attached to a process, not only it's pid, but also streams which may be buffered. I propose the close the issue.
The webbrowser issue can be fixed differently without detach().