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.

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.

> 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.

> Explicitly setting bufsize=0 should be a decent workaround.

It kills performances, no?

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 ;-)

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

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.

bufsize impacts all streams, no only stdin, no?

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

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.

Ping.

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.

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

Thanks, I looked at this issue just today :)

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'

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

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