classification
Title: Popen.communicate not ignoring BrokenPipeError
Type: behavior Stage: resolved
Components: asyncio, Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: gregory.p.smith, gvanrossum, lukasz.langa, martin.panter, memeplex, python-dev, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2016-02-17 01:28 by memeplex, last changed 2016-06-05 04:30 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
broken_pipe_error.patch vstinner, 2016-02-17 11:17 review
Messages (17)
msg260373 - (view) Author: Memeplex (memeplex) Date: 2016-02-17 01:28
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.
msg260387 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-17 10:51
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.
msg260390 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-17 11:14
> 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.
msg260391 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-17 11:14
> Explicitly setting bufsize=0 should be a decent workaround.

It kills performances, no?
msg260392 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-17 11:16
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 ;-)
msg260405 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-17 17:21
See also issue #23570: Change "with subprocess.Popen():" (context manager) to ignore broken pipe error.
msg260414 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-18 00:05
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.
msg260415 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-02-18 00:08
bufsize impacts all streams, no only stdin, no?
msg260418 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-18 00:33
Yes I understand bufsize (and universal_newlines) affects any of stdin, stdout and stderr that is set to PIPE.
msg260763 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-02-24 07:59
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.
msg266499 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-27 14:38
Ping.
msg267204 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-04 00:38
Lukas - cc'ing you just in case this bug is related to the asyncio subprocess race condition-ish problems you were talking about at pycon.
msg267206 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2016-06-04 00:42
the bot hasn't piped up with the changesets.  these are the 3.5 and default fixes:

remote: notified python-checkins@python.org of incoming changeset 883cfb3e28f9
remote: notified python-checkins@python.org of incoming changeset 78e81de6d447
msg267212 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2016-06-04 00:51
Thanks, I looked at this issue just today :)
msg267245 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-04 08:06
Maybe use os.devnull instead of "/dev/null"?

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/7728/steps/test/logs/stdio
======================================================================
ERROR: test_communicate_BrokenPipeError_stdin_flush (test.test_subprocess.ProcessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\test\test_subprocess.py", line 1267, in test_communicate_BrokenPipeError_stdin_flush
    open('/dev/null', 'wb') as dev_null:
FileNotFoundError: [Errno 2] No such file or directory: '/dev/null'
msg267292 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-04 19:05
New changeset 3a560525ca50 by Gregory P. Smith in branch '3.5':
issue26372 - use os.devnull instead of /dev/null
https://hg.python.org/cpython/rev/3a560525ca50

New changeset 52e331b86f2b by Gregory P. Smith in branch 'default':
issue26372 - use os.devnull instead of /dev/null
https://hg.python.org/cpython/rev/52e331b86f2b
msg267367 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-06-05 02:58
New changeset e89eb7935ca9 by Gregory P. Smith in branch 'default':
merge from 3.5. (moves the issue26372 tests to the proper class)
https://hg.python.org/cpython/rev/e89eb7935ca9
History
Date User Action Args
2016-06-05 04:30:52gregory.p.smithsetstatus: open -> closed
stage: commit review -> resolved
2016-06-05 02:58:54python-devsetmessages: + msg267367
2016-06-04 19:05:33python-devsetnosy: + python-dev
messages: + msg267292
2016-06-04 08:06:27martin.pantersetmessages: + msg267245
2016-06-04 00:51:54lukasz.langasetmessages: + msg267212
2016-06-04 00:42:07gregory.p.smithsetmessages: + msg267206
2016-06-04 00:38:55gregory.p.smithsetnosy: + lukasz.langa
messages: + msg267204
2016-06-04 00:36:59gregory.p.smithsetresolution: fixed
stage: patch review -> commit review
2016-05-28 21:53:27gregory.p.smithsetassignee: gregory.p.smith
2016-05-27 14:38:52serhiy.storchakasetmessages: + msg266499
2016-02-24 07:59:32serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg260763
2016-02-24 02:41:15ned.deilysetnosy: + gregory.p.smith
2016-02-18 00:33:29martin.pantersetmessages: + msg260418
2016-02-18 00:08:38vstinnersetmessages: + msg260415
2016-02-18 00:05:17martin.pantersetmessages: + msg260414
stage: needs patch -> patch review
2016-02-17 17:21:43vstinnersetmessages: + msg260405
2016-02-17 11:17:21vstinnersetfiles: + broken_pipe_error.patch
keywords: + patch
2016-02-17 11:17:13vstinnersetnosy: + yselivanov, gvanrossum
components: + asyncio
2016-02-17 11:16:48vstinnersetmessages: + msg260392
versions: + Python 2.7, Python 3.6
2016-02-17 11:14:53vstinnersetmessages: + msg260391
2016-02-17 11:14:25vstinnersetnosy: + vstinner
messages: + msg260390
2016-02-17 10:51:41martin.pantersetnosy: + martin.panter

messages: + msg260387
stage: needs patch
2016-02-17 01:28:32memeplexcreate