This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: "universal newlines" subprocess support broken with select- and poll-based communicate()
Type: behavior Stage: patch review
Components: Library (Lib), Tests Versions: Python 3.4, Python 3.5
process
Status: open Resolution: out of date
Dependencies: Superseder: subprocess.Popen.communicate with universal_newlines=True doesn't accept strings on 3.2
View: 16903
Assigned To: Nosy List: asvetlov, chris.jerdonek, georg.brandl, gregory.p.smith, martin.panter, pitrou, r.david.murray
Priority: normal Keywords: patch

Created on 2011-07-23 20:11 by pitrou, last changed 2022-04-11 14:57 by admin.

Files
File name Uploaded Description Edit
issue12623_failing_test.diff asvetlov, 2012-08-15 20:26 review
Messages (17)
msg141014 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-23 20:11
The select() and poll() loop implementations of Popen.communicate() call os.write() instead of the write() method on the stdin pipe, meaning any newline translation *and* unicode-to-bytes encoding step is skipped.

To use the write() method on the stdin pipe, we may have to set the file descriptor in non-blocking mode, especially given that _PIPE_BUF worth of characters can amount to more than _PIPE_BUF bytes on the underlying raw fd.

See issue12591 for a simpler issue that was fixed.
msg168317 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-15 19:19
Antoine, can you explain why subprocess support for universal_newlines is broken?

As I can see tests for universal_newlines passed and these looks correct.

In general I like your idea to get rid of os.write, but maybe that patch should be landed in 3.4?

If not — I will prepare fix ASAP.
msg168320 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-15 19:38
Andrew, I'm not sure if this is the issue, but it seems like the only tests in which input is passed to communicate() with universal newlines is when stdin is the only PIPE, i.e.:

    def test_universal_newlines_communicate_stdin(self):
        # universal newlines through communicate(), with only stdin

IIRC, the select() and poll() implementations are only called when at least two of the streams are PIPE (like in the new input_none test I added recently).  Have you tried passing input to communicate() with at least two pipes (e.g. stdin and stdout)?
msg168322 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-15 19:51
As Chris said. Look at the POSIX version of _communicate(), nowhere is input given the newlines/encoding treatment.
msg168325 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-15 20:18
I've pushed test for piping stdin, stdout and and stderr: 4af2c0687970
What other test we should to add?
msg168328 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-15 20:26
Оор. I see. Pushing to communicate input with "\r" (see attached patch) produces the error.

Will work on fixing.

Thanks.
msg168329 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-15 20:30
I like to set status for the issue to "Release blocker" if Georg Brandl agree with that.
msg168330 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-15 20:32
> Оор. I see. Pushing to communicate input with "\r" (see attached patch) 
> produces the error.

Hmm, no, it's the reverse. Pushing input with "\n" should produce b"\r\n" on the other side, under Windows.
msg168331 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-08-15 20:33
Well, it's not a regression and it may be slightly delicate, so I don't think it should be a release blocker.
msg168333 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-15 20:36
The main question: can be fix applied to 3.3 or it can wait for 3.4?
3.2 has the same problem BTW.
msg168335 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-15 20:38
> Pushing to communicate input with "\r" (see attached patch) produces the error.

Is this a supported use case?  In universal newlines, stdin line endings are supposed to be "\n".  From the subprocess documentation: "For stdin, line ending characters '\n' in the input will be converted to the default line separator os.linesep."
msg168336 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-15 20:43
The real test should to put '\n' and subprocess should get '\r\n', right?
Looking the code this test will fail on Windows. 
I cannot check it just now but looks like this is the problem.
msg168340 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-08-15 21:34
> Looking the code this test will fail on Windows. I cannot check it just now but looks like this is the problem.

Would it be possible to do something like the following to check this on a non-Windows machine (since the Python implementation of io respects the mutability of os.linesep but perhaps not the C version)?  It seems so.

>>> import _pyio, io, os
>>> io.TextIOWrapper = _pyio.TextIOWrapper
>>> os.linesep = "\r\n"
etc.
msg168557 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-08-19 10:59
> The main question: can be fix applied to 3.3 or it can wait for 3.4?
> 3.2 has the same problem BTW.

I don't see any fix attached :)

If it is a bug, it can be fixed before 3.3rc1, no need for release blocker status.
msg169046 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2012-08-24 16:47
I like to leave fixes to 3.4.
Any change can produce side-effects, which can be nightmare for upcoming release candidate.
Sure, Georg will share my opinion.
Though absence '\n' -> '\r\n' for input if OS is Windows and universal_newlines=True is not good.
msg220743 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-16 18:19
Can we have an update on this please, I'm assuming that the priority should now be marked as normal?
msg268346 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-06-12 07:54
.
Summary: There was originally a bug, but it has been fixed. At best, we leave this open to work on including Andrew’s patch.

Andrew’s patch adds a modified copy of test_universal_newlines_communicate_stdin(). But it does not look correct, and would fail on Windows. It needs to avoid decoding newlines in the child, like in revision 150fa296f5b9. IMO the existing test_universal_newlines_communicate_stdin() test should also be adjusted to verify that os.linesep was sent to the child process.

Also, I don’t understand the SETBINARY business. Aren’t stdin etc set to binary mode by default in Python 3? Yes, it would be required for Python 2 compatibility, but if this patch is only for Python 3, why do we need it?

Anyway, Antoine opened this bug specifically about the “select- and poll-based” implementation (now based on the new selectors module). That implementation is only used with multiple pipes. So I don’t see how the patch is relevant to the original issue (although it may be worthwhile updating and adding anyway).

Regarding Antoine’s original report, we now do encode text strings to bytes. This was fixed as a side effect of revision c4a0fa6e687c in 3.3 (added timeout=... parameter), and directly in 3.2 by Issue 16903.

As for newline translation, I’m not sure if that is really relevant. The selectors implementation is only used if sys.platform != "win32", while os.linesep translation only needs to happen when 'posix' not in sys.builtin_module_names. I suspect in all cases where the selectors implementation is used, and os.linesep is "\n", so it is not actually a bug.
History
Date User Action Args
2022-04-11 14:57:20adminsetgithub: 56832
2016-06-12 20:34:36BreamoreBoysetnosy: - BreamoreBoy
2016-06-12 07:54:21martin.pantersetpriority: critical -> normal

superseder: subprocess.Popen.communicate with universal_newlines=True doesn't accept strings on 3.2
components: + Tests

nosy: + martin.panter
messages: + msg268346
resolution: out of date
stage: needs patch -> patch review
2014-06-16 18:19:06BreamoreBoysetnosy: + BreamoreBoy

messages: + msg220743
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2012-08-24 16:47:43asvetlovsetmessages: + msg169046
2012-08-19 10:59:49georg.brandlsetpriority: release blocker -> critical

messages: + msg168557
2012-08-15 21:34:43chris.jerdoneksetmessages: + msg168340
2012-08-15 20:43:30asvetlovsetmessages: + msg168336
2012-08-15 20:38:28chris.jerdoneksetmessages: + msg168335
2012-08-15 20:36:08asvetlovsetmessages: + msg168333
2012-08-15 20:33:16pitrousetmessages: + msg168331
2012-08-15 20:32:43pitrousetmessages: + msg168330
2012-08-15 20:30:23asvetlovsetpriority: normal -> release blocker
nosy: + georg.brandl
messages: + msg168329

2012-08-15 20:26:03asvetlovsetfiles: + issue12623_failing_test.diff
keywords: + patch
messages: + msg168328
2012-08-15 20:18:50asvetlovsetmessages: + msg168325
2012-08-15 19:51:46pitrousetmessages: + msg168322
2012-08-15 19:38:32chris.jerdoneksetmessages: + msg168320
2012-08-15 19:19:24asvetlovsetnosy: + r.david.murray
messages: + msg168317
2012-08-14 01:13:07chris.jerdoneksetnosy: + chris.jerdonek
2012-08-13 12:51:51asvetlovsetnosy: + asvetlov
2011-07-23 20:11:32pitroucreate