Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subprocess.Popen should emit a ResourceWarning in destructor if the process is still running #70928

Closed
vstinner opened this issue Apr 12, 2016 · 15 comments
Labels
performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

vstinner commented Apr 12, 2016

BPO 26741
Nosy @pitrou, @vstinner, @vadmium, @serhiy-storchaka
Files
  • subprocess_res_warn.patch
  • 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 2016-08-17.12:46:23.083>
    created_at = <Date 2016-04-12.22:49:07.385>
    labels = ['library', 'performance']
    title = 'subprocess.Popen should emit a ResourceWarning in destructor if the process is still running'
    updated_at = <Date 2017-01-06.09:50:28.276>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2017-01-06.09:50:28.276>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-08-17.12:46:23.083>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2016-04-12.22:49:07.385>
    creator = 'vstinner'
    dependencies = []
    files = ['42448']
    hgrepos = []
    issue_num = 26741
    keywords = ['patch']
    message_count = 15.0
    messages = ['263281', '263285', '263287', '263288', '263296', '265930', '265931', '265933', '265937', '265940', '272532', '272534', '272535', '272931', '284803']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'vstinner', 'python-dev', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue26741'
    versions = ['Python 3.6']

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 12, 2016

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

    @vstinner vstinner added stdlib Python modules in the Lib dir performance Performance or resource usage labels Apr 12, 2016
    @vadmium
    Copy link
    Member

    vadmium commented Apr 12, 2016

    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?

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 13, 2016

    Did you forget to send the patch?

    Right :-)

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 13, 2016

    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 bpo-25942 and the old issue bpo-12494.

    @vadmium
    Copy link
    Member

    vadmium commented Apr 13, 2016

    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-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 20, 2016

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

    @vstinner
    Copy link
    Member Author

    vstinner commented May 20, 2016

    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 bpo-27068 to discuss this feature enhancement.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 20, 2016

    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 bpo-26741)
    https://hg.python.org/cpython/rev/911f398c6396

    @vadmium
    Copy link
    Member

    vadmium commented May 20, 2016

    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.

    @vstinner vstinner reopened this May 20, 2016
    @vstinner
    Copy link
    Member Author

    vstinner commented May 20, 2016

    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 bpo-27069 to fix this bug.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 12, 2016

    New changeset f78a682a3515 by Martin Panter in branch '3.5':
    Issue bpo-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 bpo-26741: Merge ResourceWarning fixes from 3.5
    https://hg.python.org/cpython/rev/7ff3ce0dfd45

    @vstinner
    Copy link
    Member Author

    vstinner commented Aug 12, 2016

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

    @vadmium
    Copy link
    Member

    vadmium commented Aug 12, 2016

    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.

    @vstinner
    Copy link
    Member Author

    vstinner commented Aug 17, 2016

    Oops, I forget to close the issue.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 6, 2017

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants