[issue21332] subprocess bufsize=1 docs are misleading

[STINNER Victor]

STINNER Victor  added the comment:


New changeset a2670565d8f5c502388378aba1fe73023fd8c8d4 by Victor Stinner 
(Alexey Izbyshev) in branch 'master':
bpo-32236: open() emits RuntimeWarning if buffering=1 for binary mode (GH-4842)
https://github.com/python/cpython/commit/a2670565d8f5c502388378aba1fe73023fd8c8d4


--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Alexey Izbyshev]

Change by Alexey Izbyshev :


--
pull_requests: +4732

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Akira Li]

Changes by Akira Li 4kir4...@gmail.com:


Added file: 
http://bugs.python.org/file36679/subprocess-line-buffering-issue21332-ps5.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Antoine Pitrou]

Changes by Antoine Pitrou pit...@free.fr:


--
assignee: gregory.p.smith - 
stage:  - commit review
versions:  -Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Antoine Pitrou]

Antoine Pitrou added the comment:

The latest patch looks good to me.

--
nosy: +pitrou

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Antoine Pitrou]

Antoine Pitrou added the comment:

Pushed! Thank you!

--
resolution:  - fixed
stage: commit review - resolved
status: open - closed

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[raylu]

raylu added the comment:

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?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

 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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:


--
nosy: +Arfrever

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

I've asked about thread-safety of tests on python-dev mailing list: 
https://mail.python.org/pipermail/python-dev/2014-May/134523.html

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

I've updated the patch to remove changes to test_universal_newlines 
test that was fixed in revision 37d0c41ed8ad that closes #21396 issue

--
Added file: 
http://bugs.python.org/file35189/subprocess-line-buffering-issue21332-ps4.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

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.

--
Added file: 
http://bugs.python.org/file35124/subprocess-line-buffering-issue21332-ps2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Martin Panter]

Martin Panter added the comment:

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.

--
nosy: +vadmium

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Martin Panter]

Martin Panter added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

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.

--
Added file: 
http://bugs.python.org/file35129/subprocess-line-buffering-issue21332-ps3.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

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.

--
keywords: +patch
type:  - behavior
Added file: 
http://bugs.python.org/file35109/subprocess-line-buffering-issue21332.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

akira added the comment:

Related issue #21396

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Ned Deily]

Changes by Ned Deily n...@acm.org:


--
nosy: +gregory.p.smith

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Gregory P. Smith]

Gregory P. Smith added the comment:

Thanks for the report, diagnosis and patch!  Your change looks good to me.  
I'll commit it soon.

--
assignee:  - gregory.p.smith

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[Jakub Wilk]

Changes by Jakub Wilk jw...@jwilk.net:


--
nosy: +jwilk

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[akira]

Changes by akira 4kir4...@gmail.com:


--
nosy: +akira

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue21332] subprocess bufsize=1 docs are misleading

[raylu]

New submission from raylu:

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.

--
components: Library (Lib)
messages: 217044
nosy: raylu
priority: normal
severity: normal
status: open
title: subprocess bufsize=1 docs are misleading
versions: Python 3.3, Python 3.4, Python 3.5

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue21332
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com