| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2016-02-18 00:08 |
| bufsize impacts all streams, no only stdin, no? |
|
|
| msg260418 - (view) |
Author: Martin Panter (martin.panter) * |
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) * |
Date: 2016-02-24 07:59 |
| This is just the bug I reported in . 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) * |
Date: 2016-05-27 14:38 |
| Ping. |
|
|
| msg267204 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
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) * |
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) * |
Date: 2016-06-04 00:51 |
| Thanks, I looked at this issue just today :) |
|
|
| msg267245 - (view) |
Author: Martin Panter (martin.panter) * |
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)  |
Date: 2016-06-04 19:05 |
| New changeset 3a560525ca50 by Gregory P. Smith in branch '3.5': - use os.devnull instead of /dev/null https://hg.python.org/cpython/rev/3a560525ca50 New changeset 52e331b86f2b by Gregory P. Smith in branch 'default': - use os.devnull instead of /dev/null https://hg.python.org/cpython/rev/52e331b86f2b |
|
|
| msg267367 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-06-05 02:58 |
| New changeset e89eb7935ca9 by Gregory P. Smith in branch 'default': merge from 3.5. (moves the tests to the proper class) https://hg.python.org/cpython/rev/e89eb7935ca9 |
|
|