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

Jump to bottom

subprocess: check_output() doesn't close pipes on error #56703

Closed
vstinner opened this issue Jul 4, 2011 · 9 comments
Closed

subprocess: check_output() doesn't close pipes on error #56703

vstinner opened this issue Jul 4, 2011 · 9 comments
Labels
stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

vstinner commented Jul 4, 2011

BPO 12494
Nosy @vstinner, @vadmium
Files
  • subprocess_check_output.patch
  • subprocess_check_output-2.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 2011-09-01.21:57:18.718>
created_at = <Date 2011-07-04.22:13:45.149>
labels = ['library']
title = "subprocess: check_output() doesn't close pipes on error"
updated_at = <Date 2016-04-04.20:51:44.618>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-04-04.20:51:44.618>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2011-09-01.21:57:18.718>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2011-07-04.22:13:45.149>
creator = 'vstinner'
dependencies = []
files = ['22572', '22580']
hgrepos = []
issue_num = 12494
keywords = ['patch']
message_count = 9.0
messages = ['139812', '139848', '139903', '143297', '143312', '143313', '143359', '143360', '262868']
nosy_count = 4.0
nosy_names = ['vstinner', 'rosslagerwall', 'python-dev', 'martin.panter']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue12494'
versions = ['Python 3.3']
    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 4, 2011

    subprocess.check_output() doesn't close explicitly pipes if an error occurs. See for example issue bpo-12493 for an example of an error on .communicate().

    Attached patch uses a context manager to ensure that all pipes are always closed and that the status is read to avoid zombies.

    Other subprocess functions should be fixed:

    • call() (will fix check_call)
    • getstatusoutput() (will fix getoutput): see patch attached to the issue bpo-10197 to replace os.popen() by subprocess.Popen

    @vstinner vstinner added the stdlib Python modules in the Lib dir label Jul 4, 2011
    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 5, 2011

    subprocess_check_output-2.patch is a more complete patch: "fix" (?) call(), check_output() and getstatusoutput(). These functions kill the process if an exception occurs to not hang on wait() in Popen.__exit__().

    Because of the kill, I don't know if the fix should be applied to 2.7 and 3.2. In case of an exception, is it better to keep the subprocess alive, or to kill it? If we keep it alive, the caller of the function cannot interact with the process, and we don't know exactly when it will finish.

    By "exception", I mean unexpected exceptions: check_output() handles explicitly the TimeoutExpired exception.

    @vstinner
    Copy link
    Member Author

    vstinner commented Jul 5, 2011

    See also issue bpo-12044 which changed the context manager to call the wait() method.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Sep 1, 2011

    The second patch looks good. Tests?

    I think it would be better to kill the process than to let it carry on.

    But, it *probably* shouldn't be applied to 2.7 & 3.2 given that it is a behaviour change.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 1, 2011

    The second patch looks good. Tests?

    Ok, I will try to write a new patch with tests.

    But, it *probably* shouldn't be applied to 2.7& 3.2 given that it is a behaviour change.

    I consider that this issue is a bug, so it should be fixed in 2.7 and
    3.2. I agree that *killing* the process is a behaviour change, but we
    can just close pipes on error for 2.7 and 3.2.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Sep 1, 2011

    I consider that this issue is a bug, so it should be fixed in 2.7 and
    3.2. I agree that *killing* the process is a behaviour change, but we
    can just close pipes on error for 2.7 and 3.2.

    Yeah, that seems right.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 1, 2011

    New changeset 86b7f14485c9 by Victor Stinner in branch 'default':
    Issue bpo-12494: Close pipes and kill process on error in subprocess functions
    http://hg.python.org/cpython/rev/86b7f14485c9

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 1, 2011

    The second patch looks good. Tests?

    I don't see how to test that the pipes are closed, because the Popen object (and its stdout and stderr attributes) are not visible outside the patched functions.

    I consider that this issue is a bug, so it should be fixed in 2.7
    and 3.2.

    I tried to write a patch, but I chose to keep the code unchanged. I prefer to keep this little tricky bug, instead of introducing another more important regression.

    Reopen the issue if you have a script to test the fix, or if you really want a backport.

    @vstinner vstinner closed this as completed Sep 1, 2011
    @vadmium
    Copy link
    Member

    vadmium commented Apr 4, 2016

    As I understand it, this change only made it to 3.3+

    @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
    Assignees
    No one assigned
    Labels
    stdlib Python modules in the Lib dir
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @vstinner @vadmium