Popen.communicate not ignoring BrokenPipeError

[memeplex]

BPO	26372
Nosy	@gvanrossum, @gpshead, @vstinner, @ambv, @vadmium, @serhiy-storchaka, @1st1
Files	broken_pipe_error.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 = 'https://github.com/gpshead'
closed_at = <Date 2016-06-05.04:30:52.838>
created_at = <Date 2016-02-17.01:28:32.540>
labels = ['type-bug', 'library', 'expert-asyncio']
title = 'Popen.communicate not ignoring BrokenPipeError'
updated_at = <Date 2016-06-05.04:30:52.837>
user = 'https://bugs.python.org/memeplex'

bugs.python.org fields:

activity = <Date 2016-06-05.04:30:52.837>
actor = 'gregory.p.smith'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2016-06-05.04:30:52.838>
closer = 'gregory.p.smith'
components = ['Library (Lib)', 'asyncio']
creation = <Date 2016-02-17.01:28:32.540>
creator = 'memeplex'
dependencies = []
files = ['41941']
hgrepos = []
issue_num = 26372
keywords = ['patch']
message_count = 17.0
messages = ['260373', '260387', '260390', '260391', '260392', '260405', '260414', '260415', '260418', '260763', '266499', '267204', '267206', '267212', '267245', '267292', '267367']
nosy_count = 9.0
nosy_names = ['gvanrossum', 'gregory.p.smith', 'vstinner', 'memeplex', 'lukasz.langa', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26372'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[memeplex]

When not using a timeout, communicate will raise a BrokenPipeError if the command returns an error code. For example:

sp = subprocess.Popen('cat --not-an-option', shell=True, stdin=subprocess.PIPE)
time.sleep(.2)
sp.communicate(b'hi\n')

This isn't consistent with the behavior of communicate with a timeout, which doesn't raise the exception. Moreover, there is even a comment near the point of the exception stating that communicate must ignore BrokenPipeError:

    def _stdin_write(self, input):
        if input:
            try:
                self.stdin.write(input)
            except BrokenPipeError:
                # communicate() must ignore broken pipe error
                pass
            ....
        self.stdin.close()

Obviously, the problem is that self.stdin.close() is outside the except clause.

[vadmium]

Seems a reasonable proposal to me. Originally the bufsize parameter defaulted to 0 so this wouldn’t have been such a problem. Explicitly setting bufsize=0 should be a decent workaround.

[vstinner]

This isn't consistent with the behavior of communicate with a timeout

Right, it looks like BrokenPipeError on writing into stdin is ignored in some cases, but not in all cases.

Attached patch for Python 3.6 adds two try/except BrokenPipeError with two unit tests.

It looks like Python 2.7 has a similar bug: stdin.close() is not surrounded by try/except BrokenPipeError.

[vstinner]

Explicitly setting bufsize=0 should be a decent workaround.

It kills performances, no?

[vstinner]

It looks like asyncio.subprocess has a similar issue in Process._feed_stdin, but I'm not sure that stdin.close() can trigger BrokenPipeError. Well, it wouldn't hurd to protect stdin.close() with a try/except BrokenPipeError too ;-)

[vstinner]

See also issue bpo-23570: Change "with subprocess.Popen():" (context manager) to ignore broken pipe error.

[vadmium]

Looking over the code for communicate(), I think setting bufsize=0 should not cause a performance problem in the original use case. Communicate() either calls stdin.write() once, or bypasses the file object and calls os.write(). Only if stdin, stdout, etc were used before communicate(), then there could be a problem (and subtly different behaviour).

I left some suggestions on the code review.

[vstinner]

bufsize impacts all streams, no only stdin, no?

[vadmium]

Yes I understand bufsize (and universal_newlines) affects any of stdin, stdout and stderr that is set to PIPE.

[serhiy-storchaka]

This is just the bug I reported in msg236951.

Text streams are buffered, thus setting bufsize=0 does not help if universal_newlines=True.

Added comments on Rietveld.

[serhiy-storchaka]

Ping.