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" can raise OSError (EPIPE) when communicating with short-lived processes #55172

Closed
davidmalcolm opened this issue Jan 20, 2011 · 11 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@davidmalcolm
Copy link
Member

BPO 10963
Nosy @gpshead, @amauryfa, @pitrou, @giampaolo, @davidmalcolm, @vadmium, @serhiy-storchaka
Files
  • py3k-handle-EPIPE-for-short-lived-subprocesses-2011-01-20-001.patch
  • 10963.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-04-05.14:14:29.064>
    created_at = <Date 2011-01-20.20:23:29.583>
    labels = ['type-bug', 'library']
    title = '"subprocess" can raise OSError (EPIPE) when communicating with short-lived processes'
    updated_at = <Date 2016-02-18.00:14:03.709>
    user = 'https://github.com/davidmalcolm'

    bugs.python.org fields:

    activity = <Date 2016-02-18.00:14:03.709>
    actor = 'martin.panter'
    assignee = 'rosslagerwall'
    closed = True
    closed_date = <Date 2011-04-05.14:14:29.064>
    closer = 'rosslagerwall'
    components = ['Library (Lib)']
    creation = <Date 2011-01-20.20:23:29.583>
    creator = 'dmalcolm'
    dependencies = []
    files = ['20469', '21491']
    hgrepos = []
    issue_num = 10963
    keywords = ['patch']
    message_count = 11.0
    messages = ['126643', '126644', '126934', '126935', '132662', '132730', '132806', '133031', '133033', '236951', '260416']
    nosy_count = 13.0
    nosy_names = ['gregory.p.smith', 'amaury.forgeotdarc', 'pitrou', 'giampaolo.rodola', 'dwalczak', 'dmalcolm', 'mcrute', 'Yaniv.Aknin', 'rosslagerwall', 'simon3z', 'python-dev', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue10963'
    versions = ['Python 3.3']

    @davidmalcolm
    Copy link
    Member Author

    If we start a short-lived process which finishes before we begin communicating with it (e.g. by crashing), we can receive a SIGPIPE due to the receiving process no longer existing. This becomes an EPIPE, which becomes an:
    OSError: [Errno 32] Broken pipe

    Arguably this is a bug; if the subprocess could crash, the user currently has to check for it by both monitoring the returncode _and_ catching OSError (then examining for this specific errno), which seems ugly to me.

    I'm attaching a patch for subprocess which handles this case, masking the OSError within the Popen implementation, so that you have to test for it within the returncode, in one place, rather than wrap every call with a try/except.

    It could be argued that this is incorrect, as it masks under-reads of stdin by the subprocess. However I believe a sanely-written subprocess ought to indicate success/failure back with its return code.

    This was originally filed downstream within the Red Hat bugzilla instance as:
    https://bugzilla.redhat.com/show_bug.cgi?id=667431

    The handler part of the patch is based on a patch attached there by Federico Simoncelli; I don't know if he has signed a PSF contributor agreement, however the size of the patch is sufficiently small that I suspect it's not copyrightable, and I've somewhat rewritten it since; I wrote the unit test.

    @davidmalcolm davidmalcolm added the stdlib Python modules in the Lib dir label Jan 20, 2011
    @pitrou
    Copy link
    Member

    pitrou commented Jan 20, 2011

    It could be argued that this is incorrect, as it masks under-reads of
    stdin by the subprocess. However I believe a sanely-written subprocess
    ought to indicate success/failure back with its return code.

    It seems quite orthogonal. The subprocess could have terminated successfully while not all of the stdin bytes were consumed; EPIPE informs you of the latter.

    @simon3z
    Copy link
    Mannequin

    simon3z mannequin commented Jan 24, 2011

    I agree they are orthogonal. The problem is that Popen.communicate doesn't support a way to ignore the exception and keep reading from stdout and sterr until the process dies.
    When the child closes the stdin its (error/debug/info) messages on stout/sterr will be lost.
    I am not just concerned about checking both the return-code and this exception, I am also worried about losing important information.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 24, 2011

    You are right, this is suboptimal. It would also be a behaviour change from currently, but it seems beneficial.

    @pitrou pitrou added the type-feature A feature request or enhancement label Jan 24, 2011
    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Mar 31, 2011

    I'd argue that this is not a feature request but a bug.

    I did some testing of this issue and the problem is that EPIPE is only generated sometimes depending on the time the process takes to finish, the size of the data sent, the underlying mechanism used (select vs poll) and the whether anything happens between the starting of the process and the communicate() call.

    Here are some results (on my PC, I think some of these will vary on others):
    With poll:
    [sys.executable, 'c', 'pass']- no error
    ['dd', 'option=bad'] - varies between EPIPE and no error
    ['dd', 'option=bad'], sleep(1) - EPIPE

    With select:
    [sys.executable, 'c', 'pass']- EPIPE
    ['dd', 'option=bad'] - EPIPE
    ['dd', 'option=bad'], sleep(1) - EPIPE

    Only stdin (neither select or poll):
    [sys.executable, 'c', 'pass']- no error (error in 2.7)
    ['dd', 'option=bad'] - no error (error in 2.7)
    ['dd', 'option=bad'], sleep(1) - EPIPE

    (all of my tests appear to fail on Windows, they also generate EINVAL sometimes)

    I think it's best to remove all this inconsistency and fix it so that EPIPE is never generated and then backport it to 2.7, 3.1, 3.2.

    Attached is a patch which fixes it for poll, select, windows and adds two tests.

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Apr 1, 2011

    Marked bpo-6457 as a duplicate. See bpo-6457 for more discussion.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 2, 2011

    I think it's best to remove all this inconsistency and fix it so that
    EPIPE is never generated and then backport it to 2.7, 3.1, 3.2.

    Attached is a patch which fixes it for poll, select, windows and adds
    two tests.

    The patch looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 5, 2011

    New changeset c10d55c51d81 by Ross Lagerwall in branch '2.7':
    Issue bpo-10963: Ensure that subprocess.communicate() never raises EPIPE.
    http://hg.python.org/cpython/rev/c10d55c51d81

    New changeset 158495d49f58 by Ross Lagerwall in branch '3.1':
    Issue bpo-10963: Ensure that subprocess.communicate() never raises EPIPE.
    http://hg.python.org/cpython/rev/158495d49f58

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Apr 5, 2011

    Committed, thanks.

    @rosslagerwall rosslagerwall mannequin closed this as completed Apr 5, 2011
    @rosslagerwall rosslagerwall mannequin self-assigned this Apr 5, 2011
    @rosslagerwall rosslagerwall mannequin added type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Apr 5, 2011
    @serhiy-storchaka
    Copy link
    Member

    self.stdout.close() also can fail with EPIPE or EINVAL if the stream is buffered (text streams are buffered). I.e. communicate() in text mode can loss the data. See also bpo-21619.

    @vadmium
    Copy link
    Member

    vadmium commented Feb 18, 2016

    FTR bpo-26372 has been opened about Serhiy’s bug with stdin.close() raising EPIPE.

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants