classification
Title: Python select.select does not correctly report read readyness
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amicitas, giampaolo.rodola, gregory.p.smith, neologix, pitrou, python-dev, rosslagerwall
Priority: normal Keywords: patch

Created on 2011-03-10 05:04 by amicitas, last changed 2011-03-19 16:12 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess_buffer.diff neologix, 2011-03-10 08:12 Allow unbuffered mode.
11459.patch rosslagerwall, 2011-03-15 15:02
11459_v2.patch rosslagerwall, 2011-03-19 12:51
11459_v3.patch pitrou, 2011-03-19 15:54
Messages (14)
msg130488 - (view) Author: Novimir Pablant (amicitas) Date: 2011-03-10 05:04
I am trying to get the output from an external program into python using `subprocess.Popen` and `select.select`.   For some reason though select.select is at times telling me that stdout is not ready to read, even when it is (reading from it works).   

This problem exists in python 3.1 & 3.2 but not in python 2.7.



For my particular application I am connecting to external program via ssh.  I would not expect that the ssh connection would be an issue.

The program that I am calling produces some output and eventually asks for user input.  In the example below I only get a portion of the output, then at some point select.select tells me that read is not available.  If I try to read anyway I can keep getting more output.  As soon as I press a key (passing something to stdin),  select.select tells me that I can read again and I get the rest of the output.   

Any ideas?
    

    def wrapExternal(host=None):

       command = ["ssh", "-t", host, "some_program"]

       child = subprocess.Popen(command
                                ,bufsize=0
                                ,stdout=subprocess.PIPE
                                ,stderr=subprocess.STDOUT)
        
       out = ''
       while True:
          r, w, x = select.select([child.stdout], [], [], 1.0)
    
          if child.poll() is not None:
             break
                        
          if r:
             out = child.stdout.read(1)
             print(out.decode(), end='')
             sys.stdout.flush()
    
       return child.returncode
msg130489 - (view) Author: Novimir Pablant (amicitas) Date: 2011-03-10 05:12
I forgot to mention, I am running on OS X 10.6.6.
msg130493 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2011-03-10 08:12
Could you try with the attached patch ?
The problem is that subprocess silently replaces bufsize=0, so child.stdout is actually buffered, and when you read just one byte, everything that's available for reading is read into the python's object buffer. Then, select/poll doesn't see the pipe as ready for reading since everything as already been read.
Mixing buffered I/O and select leads to trouble, you're right to pass bufsize=0, but I don't know why subprocess goes out of its way and buffers it anyway:
        if bufsize == 0:
            bufsize = 1  # Nearly unbuffered (XXX for now)
msg130509 - (view) Author: Novimir Pablant (amicitas) Date: 2011-03-10 15:35
Applying the patch appears to fix this problem. Thanks!


I am definitely confused about why the buffer was changed to line buffered in the first place, especially since the default is bufsize=0 and the comment ("# Nearly unbuffered (XXX for now)") does not appear to match the actual behavior.
msg130979 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-03-15 15:02
The Charles' patch fixes the problem but breaks [test_os test_poll test_popen test_select test_uuid] when running make test.

Those two lines were introduced by Guido in [1f7891d84d93] but that was back in 2007 when subprocess used os.fdopen and the new IO hadn't been invented (I think). They should probably be removed now.

I think it breaks those tests because despite the fact that the docs claim subprocess defaults to unbuffered, it has been set at bufsize=1 which actually causes line buffering mode. os.popen wraps the returned stream with a TextIOWrapper which doesn't work with an unbuffered stream.

The attached patch causes os.popen (and platform.popen) to default to line buffered mode and to throw an error if buffering=0 is passed (unbuffered).

I don't think this should be backported since there is some behavior change.
msg131089 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-03-16 06:03
ross's patch looks good to me.

Isn't the behavior just plain broken in 3.1 and 3.2?  The docs say that the default bufsize=0 is unbuffered in Popen but the implementation has that nasty XXX to make it line buffered instead of unbuffered in it.

This appears to have hid the problems with the textiowrapper being used in os.popen.

I think fixing it in 3.1 and 3.2 is the right thing to do.

It is desirable for os.popen()'s default buffering to match that of open() itself which is that they're buffered a buffer size of io.DEFAULT_BUFFER_SIZE.
msg131091 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-03-16 06:21
to get that behavior, change the =1 default to =io.DEFAULT_BUFFER_SIZE in ross's patch.
msg131121 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-03-16 14:33
> to get that behavior, change the =1 default to =io.DEFAULT_BUFFER_SIZE in ross's patch.

The problem is, it doesn't seem like you can import and use io.DEFAULT_BUFFER_SIZE from inside os.py. Python fails to start.

You can import _io.DEFAULT_BUFFER_SIZE though.
msg131209 - (view) Author: Novimir Pablant (amicitas) Date: 2011-03-17 01:54
I agree with Gregory that fixing this in 3.1 and 3.2 is the way to go. 

Otherwise I would suggest changing the documentation to match the behavior.
msg131226 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-03-17 06:41
yeah i figured importing io from os at the top level might be a problem. it is not important for the default to be that exact value, even something safely on the small side like 512 will work.  but we could just have the default be set in the code by doing the 'import io' at os.popen() runtime.
msg131410 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-03-19 12:51
Attached is a patch which uses -1 for the buffer size of popen(). This gets translated by the io.open() to the default io buffer size.
msg131423 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-19 15:54
Here is the adapted patch for 3.1, with a test case.
msg131425 - (view) Author: Roundup Robot (python-dev) Date: 2011-03-19 16:10
New changeset 1dc52ecb8949 by Antoine Pitrou in branch '3.1':
Issue #11459: A `bufsize` value of 0 in subprocess.Popen() really creates
http://hg.python.org/cpython/rev/1dc52ecb8949

New changeset 7451da272111 by Antoine Pitrou in branch '3.2':
Issue #11459: A `bufsize` value of 0 in subprocess.Popen() really creates
http://hg.python.org/cpython/rev/7451da272111

New changeset cb148da52c47 by Antoine Pitrou in branch 'default':
Issue #11459: A `bufsize` value of 0 in subprocess.Popen() really creates
http://hg.python.org/cpython/rev/cb148da52c47
msg131426 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-03-19 16:12
Fixed now, thank you!
History
Date User Action Args
2012-05-06 12:04:42pitroulinkissue14737 superseder
2011-03-19 16:12:04pitrousetstatus: open -> closed
versions: + Python 3.3
nosy: gregory.p.smith, pitrou, giampaolo.rodola, neologix, rosslagerwall, python-dev, amicitas
messages: + msg131426

resolution: fixed
stage: patch review -> resolved
2011-03-19 16:10:02python-devsetnosy: + python-dev
messages: + msg131425
2011-03-19 15:54:26pitrousetfiles: + 11459_v3.patch
nosy: + pitrou
messages: + msg131423

2011-03-19 12:51:19rosslagerwallsetfiles: + 11459_v2.patch
nosy: gregory.p.smith, giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg131410
2011-03-17 06:41:52gregory.p.smithsetnosy: gregory.p.smith, giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg131226
2011-03-17 01:54:57amicitassetnosy: gregory.p.smith, giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg131209
2011-03-16 14:33:37rosslagerwallsetnosy: gregory.p.smith, giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg131121
2011-03-16 06:21:09gregory.p.smithsetnosy: gregory.p.smith, giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg131091
2011-03-16 06:03:39gregory.p.smithsetnosy: gregory.p.smith, giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg131089
2011-03-15 16:54:00pitrousetnosy: + gregory.p.smith
2011-03-15 15:02:56rosslagerwallsetfiles: + 11459.patch

messages: + msg130979
nosy: giampaolo.rodola, neologix, rosslagerwall, amicitas
stage: patch review
2011-03-10 15:35:29amicitassetnosy: giampaolo.rodola, neologix, rosslagerwall, amicitas
messages: + msg130509
2011-03-10 13:32:15rosslagerwallsetnosy: + rosslagerwall
2011-03-10 09:07:01giampaolo.rodolasetnosy: + giampaolo.rodola
2011-03-10 08:12:26neologixsetfiles: + subprocess_buffer.diff

nosy: + neologix
messages: + msg130493

keywords: + patch
2011-03-10 05:12:48amicitassetmessages: + msg130489
2011-03-10 05:04:27amicitascreate