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
Popen.communicate not ignoring BrokenPipeError #70560
Comments
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. |
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. |
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 bpo-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 |
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 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': New changeset 52e331b86f2b by Gregory P. Smith in branch 'default': |
New changeset e89eb7935ca9 by Gregory P. Smith in branch 'default': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: