classification
Title: subprocess bufsize=1 docs are misleading
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, akira, gregory.p.smith, jwilk, martin.panter, pitrou, python-dev, raylu
Priority: normal Keywords: patch

Created on 2014-04-22 23:35 by raylu, last changed 2017-12-13 16:33 by izbyshev. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess-line-buffering-issue21332.patch akira, 2014-04-30 10:09 update docs, add tests, pass line_buffering review
subprocess-line-buffering-issue21332-ps2.patch akira, 2014-05-01 10:25 modify test_universal_newlines to work with Python io implementation review
subprocess-line-buffering-issue21332-ps3.patch akira, 2014-05-02 04:37 avoid big timeout in the tests review
subprocess-line-buffering-issue21332-ps4.patch akira, 2014-05-09 00:45 remove no longer necessary changes to test_universal_newlines review
subprocess-line-buffering-issue21332-ps5.patch akira, 2014-09-21 12:54 move assertEqual() to the caller
Pull Requests
URL Status Linked Edit
PR 4842 open izbyshev, 2017-12-13 16:33
Messages (17)
msg217044 - (view) Author: raylu (raylu) Date: 2014-04-22 23:35
https://docs.python.org/3.3/library/subprocess.html says

"bufsize will be supplied as the corresponding argument to the open() function when creating the stdin/stdout/stderr pipe file objects: ... 1 means line buffered"

but it calls io.open with 'wb' and 'rb': http://hg.python.org/cpython/file/c9cb931b20f4/Lib/subprocess.py#l828

This puts the file in binary mode, and https://docs.python.org/3.3/library/functions.html#open says

"1 to select line buffering (only usable in text mode)"

Even with universal_newlines=True, the TextIOWrapper isn't passed line_buffering=True. There's actually no way to get line buffering any more.
msg217596 - (view) Author: Akira Li (akira) * Date: 2014-04-30 10:09
It looks like a bug in the subprocess module e.g., if child process does:

  sys.stdout.write(sys.stdin.readline())
  sys.stdout.flush()

and the parent:

   p.stdin.write(line) #NOTE: no flush
   line = p.stdout.readline()

then a deadlock may happen with bufsize=1 (because it is equivalent to bufsize=-1 in the current code)

Surprisingly, it works if universal_newlines=True but only for C implementation of io i.e., if C extension is disabled:

  import io, _pyio 
  io.TextIOWrapper = _pyio.TextIOWrapper

then it blocks forever even if universal_newlines=True with bufsize=1 that is clearly wrong (<-- bug) e.g., `test_universal_newlines` deadlocks with _pyio.TextIOWrapper

C implementation works because it sets `needflush` flag even if only `write_through` is provided [1]:

    if (self->write_through)
        needflush = 1;
    else if (self->line_buffering &&
        (haslf ||
         PyUnicode_FindChar(text, '\r', 0, PyUnicode_GET_LENGTH(text), 1) != -1))
        needflush = 1;

[1]: http://hg.python.org/cpython/file/0b2e199ad088/Modules/_io/textio.c#l1333

Python io implementation doesn't flush with only `write_through` flag.

It doesn't deadlock if bufsize=0 whether universal_newlines=True or not.

Note: Python 2.7 doesn't deadlock with bufsize=0 and bufsize=1 in this case as expected.

What is not clear is whether it should work with universal_newline=False and bufsize=1: both current docs and Python 2.7 behaviour say that there should not be a deadlock.

I've updated the docs to mention that bufsize=1 works only with 
universal_newlines=True and added corresponding tests. I've also updated the subprocess' code to pass line_buffering explicitly.

Patch is uploaded.
msg217601 - (view) Author: Akira Li (akira) * Date: 2014-04-30 10:39
Related issue #21396
msg217651 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2014-04-30 20:56
Thanks for the report, diagnosis and patch!  Your change looks good to me.  I'll commit it soon.
msg217683 - (view) Author: Akira Li (akira) * Date: 2014-05-01 10:25
I've changed test_newlines to work also with Python io implementation.

I've updated the patch.

Note: tests use 10 seconds timeouts: I don't know how long it should take to read back a line from a subprocess so that the timeout would indicate a deadlock.
msg217739 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-05-02 01:41
Perhaps you can avoid the 10 s deadlock timeout and threading in your test by closing the underlying input pipe file descriptor (or raw file object), without flushing it.

Also, it seems to me that “line_buffering=True” is redundant with “write_through=True”. I’m not super familiar with the new write-through mode though, so I could be wrong. Perhaps the change in “subprocess.py” may not be needed at least once Issue 21396 is fixed.
msg217740 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-05-02 01:49
Sorry, it seems I was wrong on the second point. Looking closer, it seems write-through mode only flushes the TextIOWrapper layer, not the underlying binary file object, whereas line-buffering mode flushes everything to the OS, so the extra line_buffering=True flag would be needed.
msg217742 - (view) Author: Akira Li (akira) * Date: 2014-05-02 04:37
yes, line_buffering=(bufsize == 1) is necessary to support the current 
Python io implementation or if C implementation is fixed to avoid 
buffer.flush() on every write with write_through=True -- otherwise
bufsize is not respected in text mode (it would always mean bufsize=0). 

Note: the current patch for issue #21396 (making C and Python io do the 
same thing) may break subprocess code with universal_newlines=True that 
expects (incorrectly) bufsize=0 by default -- as test_universal_newlines
had (enabling universal_newlines shouldn't switch from bufsize=-1 to 
bufsize=0). <-- XXX backward compatibility issue!

> Perhaps you can avoid the 10 s deadlock timeout and threading in your 
> test by closing the underlying input pipe file descriptor (or raw file 
> object), without flushing it.

It is a good idea. There could be portability issues with the test: it 
relies on the fact that os.close doesn't flush already buffered data -- I 
don't know whether os.close causes flush on Windows (it doesn't on POSIX 
[1]: the data shall be discarded).

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html

I've uploaded a new patch with the updated tests. Please, review.
msg217744 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-05-02 06:14
On second thoughts maybe the idea of closing the input is not such a good idea in practice. Once you call os.close() on the file descriptor, that descriptor becomes unallocated, and I can’t see any way to prevent p.stdin.close(), Popen() context, destructors, etc from trying to close the file descriptor again. If the Python test suite is ever multithreaded (I’m not really familiar with it), that would be a real problem. In any case closing an unallocated file descriptor is a bad idea. Maybe the deadlocking version is more appropriate after all.
msg217745 - (view) Author: Akira Li (akira) * Date: 2014-05-02 07:14
to be clear: the test itself doesn't use threads, `python -mtest -j0`
runs tests using multiple *processes*, not threads. There is no issue.

If the test were to run in the presence of multiple threads then
the issue would be the *explicit* p.stdin.close() in the test (look
at the patch) that may close `p.stdin.fileno()` that might be opened
in another thread (unrelated to the current test) after it was
closed in the test the first time. I could call
`getattr(p.stdin, 'buffer', p.stdin).raw.fd = -1` to avoid trying to
close the fd the second time but I don't think it is necessary.

I don't think tests are expected to run in the presence of multiple
threads unless they start them.
msg218136 - (view) Author: Akira Li (akira) * Date: 2014-05-09 00:45
I've updated the patch to remove changes to test_universal_newlines 
test that was fixed in revision 37d0c41ed8ad that closes #21396 issue
msg218229 - (view) Author: Akira Li (akira) * Date: 2014-05-10 18:21
I've asked about thread-safety of tests on python-dev mailing list: 
https://mail.python.org/pipermail/python-dev/2014-May/134523.html
msg219058 - (view) Author: Akira Li (akira) * Date: 2014-05-24 22:01
> The short answer is: no, you don't have to make you thread thread
> safe, as long as it can reliably run even in the presence of
> background threads (like the tkinter threads Victor mentions).

https://mail.python.org/pipermail/python-dev/2014-May/134541.html

It seems the test may be left as is.

Please, review the patch.
msg220919 - (view) Author: raylu (raylu) Date: 2014-06-18 02:37
I'm fairly sure this hasn't been fixed in tip so I think we're still waiting on patch review. Is there an update here?
msg227216 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-09-21 13:21
The latest patch looks good to me.
msg227223 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-21 19:15
New changeset 38867f90f1d9 by Antoine Pitrou in branch '3.4':
Issue #21332: Ensure that ``bufsize=1`` in subprocess.Popen() selects line buffering, rather than block buffering.
https://hg.python.org/cpython/rev/38867f90f1d9

New changeset 763d565e5840 by Antoine Pitrou in branch 'default':
Issue #21332: Ensure that ``bufsize=1`` in subprocess.Popen() selects line buffering, rather than block buffering.
https://hg.python.org/cpython/rev/763d565e5840
msg227225 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-09-21 19:16
Pushed! Thank you!
History
Date User Action Args
2017-12-13 16:33:05izbyshevsetpull_requests: + pull_request4732
2014-09-21 19:16:25pitrousetstatus: open -> closed
resolution: fixed
messages: + msg227225

stage: commit review -> resolved
2014-09-21 19:15:52python-devsetnosy: + python-dev
messages: + msg227223
2014-09-21 13:21:46pitrousetnosy: + pitrou
messages: + msg227216
2014-09-21 13:21:31pitrousetassignee: gregory.p.smith ->
stage: commit review
versions: - Python 3.3
2014-09-21 12:54:09akirasetfiles: + subprocess-line-buffering-issue21332-ps5.patch
2014-06-18 02:37:11raylusetmessages: + msg220919
2014-05-24 22:01:23akirasetmessages: + msg219058
2014-05-11 12:23:30Arfreversetnosy: + Arfrever
2014-05-10 18:21:34akirasetmessages: + msg218229
2014-05-09 00:45:50akirasetfiles: + subprocess-line-buffering-issue21332-ps4.patch

messages: + msg218136
2014-05-02 07:14:29akirasetmessages: + msg217745
2014-05-02 06:14:11martin.pantersetmessages: + msg217744
2014-05-02 04:38:01akirasetfiles: + subprocess-line-buffering-issue21332-ps3.patch

messages: + msg217742
2014-05-02 01:49:13martin.pantersetmessages: + msg217740
2014-05-02 01:41:47martin.pantersetnosy: + martin.panter
messages: + msg217739
2014-05-01 10:25:30akirasetfiles: + subprocess-line-buffering-issue21332-ps2.patch

messages: + msg217683
2014-04-30 20:56:50gregory.p.smithsetassignee: gregory.p.smith
messages: + msg217651
2014-04-30 17:20:35ned.deilysetnosy: + gregory.p.smith
2014-04-30 10:39:57akirasetmessages: + msg217601
2014-04-30 10:09:15akirasetfiles: + subprocess-line-buffering-issue21332.patch
type: behavior
messages: + msg217596

keywords: + patch
2014-04-23 19:42:14akirasetnosy: + akira
2014-04-23 11:39:33jwilksetnosy: + jwilk
2014-04-22 23:35:45raylucreate