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

Change "with subprocess.Popen():" (context manager) to ignore broken pipe error #67758

Closed
vstinner opened this issue Mar 3, 2015 · 13 comments
Closed

Comments

@vstinner
Copy link
Member

vstinner commented Mar 3, 2015

BPO 23570
Nosy @vstinner, @4kir4, @vadmium, @serhiy-storchaka
Files
  • popen_exit.patch
  • popen_exit-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 2015-04-06.21:31:28.982>
    created_at = <Date 2015-03-03.09:49:16.812>
    labels = []
    title = 'Change "with subprocess.Popen():" (context manager) to ignore broken pipe error'
    updated_at = <Date 2016-02-17.17:21:23.866>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2016-02-17.17:21:23.866>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-04-06.21:31:28.982>
    closer = 'vstinner'
    components = []
    creation = <Date 2015-03-03.09:49:16.812>
    creator = 'vstinner'
    dependencies = []
    files = ['38311', '38313']
    hgrepos = []
    issue_num = 23570
    keywords = ['patch']
    message_count = 13.0
    messages = ['237115', '237117', '237120', '237122', '237125', '237129', '237130', '237131', '237133', '237498', '240186', '260403', '260404']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'neologix', 'akira', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'rejected'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue23570'
    versions = ['Python 3.5']

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 3, 2015

    The Popen.communicate() method ignores broken pipe error when writing to stdin. I propose to modify Popen.__exit__() to do the same in Python 3.5.

    Attached patch implements this suggestion and document it. I added this paragraph to Popen doc:

    "The context manager ignores broken pipe errors when closing the process’s stdin: call explicitly proc.stdin.flush() and proc.stdin.close() to get broken pipe errors."

    So it's still possible to get broken pipe errors if you need them.

    Do you know applications or libraries which rely on broken pipe errors and do something different than just ignoring them?

    I prefer to leave Python 3.4 unchanged to avoid subtle behaviour changes in minor Python releases.

    See also:

    • issue bpo-21619 which modified Popen.__exit__() to call wait() even if stdin.close() raised an exception
    • issue bpo-19612 which modified communicate() to handle EINVAL on stdin.write() on Windows

    @vadmium
    Copy link
    Member

    vadmium commented Mar 3, 2015

    I left some minor comments on the documentation.

    As a side effect of your rearranging of _stdin_write(), I think it would fix the bug with communicate() leaking a BrokenPipeError and leaving a zombie when there is less than a buffer’s worth of data to send.

    @serhiy-storchaka
    Copy link
    Member

    Do you want to modify IOBase.__exit__ to ignore I/O errors in close()? I think such changes should be discussed in Python-Dev.

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 3, 2015

    Do you want to modify IOBase.__exit__ to ignore I/O errors in close()?

    Nope. On files, you want to want to know if your data has been fully written. For a subprocess, it's different. You only expect best effort.

    The BrokenPipeError exception is raised by Python when an OS function fails with EPIPE. This exception has the same purpose than the SIGPIPE signal: warn the application that it's now useless to send more data, the consumer will ignore it. By the way, since Python checks the result of *all* OS functions, SIGPIPE is simply ignored. SIGPIPE and EPIPE have the same purpose.

    In the subprocess module, if we get BrokenPipeError, it means that the child process stopped reading from stdin (closed it or the process already exited).

    Popen.communicate() must close stdin, so why would we pass BrokenPipeError to the caller? It's useless, we just stop writing and close the pipe.

    Since Popen.__exit__() also closes stdin, I use the same rationale: it useless to pass BrokenPipeError to the caller. The caller expects that the process exited.

    Anyway, we closed all pipes, it's not more possible to communicate with the child process. The following example displays "is proc stdin closed? True":
    ---

    import subprocess, sys
    
    args = [sys.executable, '-c', 'pass']
    proc = subprocess.Popen(args, stdin=subprocess.PIPE)
    try:
        with proc:
            proc.stdin.write(b'x' * (2**20))
    except BrokenPipeError:
        pass
    print("is proc stdin closed?", proc.stdin.closed)

    See also: "Why does SIGPIPE exist?"
    https://stackoverflow.com/questions/8369506/why-does-sigpipe-exist

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 3, 2015

    New patch to fix the bug seen by Serhiy.

    Anyway, we closed all pipes

    Oh, I forgot to explain that TextIOWrapper.close() closes the buffered file even if flush() raised an exception. BufferedWriter.close() does the same.

    So stdin.close() always closes the text (binary or text, buffered or not). It's not more possible to use stdin after stdin.close(), even if stdin.close() raised an exception.

    @serhiy-storchaka
    Copy link
    Member

    I don't see a difference between buffered file and Popen object. Both are useless
    after closing, both can raise an exception when flush a buffer on closing. Why
    suppress an exception in one case but not in other? I think this question
    needs wider discussion in Python-Dev.

    @serhiy-storchaka
    Copy link
    Member

    New patch to fix the bug seen by Serhiy.

    I thought about different solution:

    try:
        if input:
            self.stdin.write(input)
    finally:
        self.stdin.close()
    

    But your approach looks working too,

    @vstinner
    Copy link
    Member Author

    vstinner commented Mar 3, 2015

    I thought about different solution:

    Your solution is different: I would prefer to also ignore broken pipe errors on close(). I'm not sure that close() can raise a BrokenPipeError in practice.

    @serhiy-storchaka
    Copy link
    Member

    Your solution is different: I would prefer to also ignore broken pipe errors
    on close(). I'm not sure that close() can raise a BrokenPipeError in
    practice.

    Of course all this code should be inside try/except block that ignores a
    BrokenPipeError.

    @vadmium
    Copy link
    Member

    vadmium commented Mar 8, 2015

    After understanding the Windows test failure in bpo-21619, I am starting to believe that code relying on a BrokenPipeError or EPIPE is flawed. It is an inherent unavoidable race condition with the receiving end of the pipe, as long as another thread or process is involved, which is always the case for the subprocess module. Another way of looking at it is that there is no guarantee that your data will have been (or will be) received, even if stdin.close() succeeds and does not raise EPIPE or similar. This is because piped data is buffered by the OS.

    So the proposed change wouldn’t be a significant disadvantage, except for code that is already flawed. It is analogous to the argument used for ignoring EINTR, because depending on it for handling signals is inherently racy.

    @vstinner
    Copy link
    Member Author

    vstinner commented Apr 6, 2015

    No consensus was found, I close the issue.

    @vstinner vstinner closed this as completed Apr 6, 2015
    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Feb 17, 2016

    Should this issue be reopened in light of
    http://bugs.python.org/issue26372 (Popen.communicate not ignoring
    BrokenPipeError)?

    If .close() shouldn't raise BrokenPipeError in .communicate() (and it
    shouldn't) then it seems logical that .close() shouldn't raise
    BrokenPipeError in .__exit__() too (and in other subprocess.Popen
    methods that do not return until the child process is dead such as
    subprocess.run())

    @vstinner
    Copy link
    Member Author

    "Should this issue be reopened in light of http://bugs.python.org/issue26372 (Popen.communicate not ignoring BrokenPipeError)?"

    I don't like reopen old issues. IMHO the two issues are different enough to justify two entries in the bug tracker.

    @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
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants