Add callback support for stdout and stderr.

Add callback support for stdout and stderr.

It's currently an inefficient thread implementation. Interestingly
enough, callback support is significantly faster on cygwin than on
native python.

Writing an efficient implementation is punted for a later change,
one per implementation.

Stops using a temporary file since it's not necessary anymore.

The goal is to reduce the number of places where a similar paradigm
is used by having a canonical generic implementation.

R=dpranke@chromium.org
BUG=
TEST=Tested manually on Windows, cygwin, linux, OSX 10.6


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109239

[M-A Ruel, 2011-11-03 13:13:40]

This CL took me a while to implement but this will permit me to remove all the
adhocs implementations. This one is well tested and guaranteed to work on all
platforms.

I have OS-specific versions pending (but not fully working either) but I wanted
to have a rock-solid base implementation first.

[Dirk Pranke, 2011-11-04 01:15:11]

I'm not sure I understand what you're attempting to do here. What is the problem
you're trying to solve? Maybe you can point me at examples of the things you
want to clean up?

Also, I'm not convinced that this code is easier or more clearly correct than
simply implementing the select/waitfornextevent versions would be, given the
issues you have around signalling threads when a timeout happens.

http://codereview.chromium.org/8374026/diff/8001/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/8374026/diff/8001/subprocess2.py#newcode215
subprocess2.py:215: dest.put((name, data))
This timeout doesn't really work if the pipes are blocking, right? I don't think
the thread will ever wake up if nothing is written.

[M-A Ruel, 2011-11-04 01:41:48]

On 2011/11/04 01:15:11, Dirk Pranke wrote:
> I'm not sure I understand what you're attempting to do here. What is the
problem
> you're trying to solve? Maybe you can point me at examples of the things you
> want to clean up?

I want to implement the accelerated version. There was at least 3 different
implementations and they are all buggy in a way or another.

> Also, I'm not convinced that this code is easier or more clearly correct than
> simply implementing the select/waitfornextevent versions would be, given the
> issues you have around signalling threads when a timeout happens.

select doesn't exist on windows. I'll implement the select based version, then
the windows native one.

http://codereview.chromium.org/8374026/diff/8001/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/8374026/diff/8001/subprocess2.py#newcode215
subprocess2.py:215: dest.put((name, data))
On 2011/11/04 01:15:11, Dirk Pranke wrote:
> This timeout doesn't really work if the pipes are blocking, right? I don't
think
> the thread will ever wake up if nothing is written.

If the process is killed, the pipes are closed, which unblocks the read() calls.

[Dirk Pranke, 2011-11-04 01:58:10]

Okay, I follow what you're doing a bit better here. I missed that you were
starting a separate timeout thread for each fd.

It looks like you're going through all of this work in order to support people
calling subprocess.communicate with StringIOs for stdout/stderr in order to
process output as it happens; is that right? Can you point me to places where
you would want to do that?

[M-A Ruel, 2011-11-04 12:32:37]

On 2011/11/04 01:58:10, Dirk Pranke wrote:
> Okay, I follow what you're doing a bit better here. I missed that you were
> starting a separate timeout thread for each fd.

Yep, having the timeout in a separate thread guarantees no dead lock.

> It looks like you're going through all of this work in order to support people
> calling subprocess.communicate with StringIOs for stdout/stderr in order to
> process output as it happens; is that right? Can you point me to places where
> you would want to do that?

Not StringIO but real callback functions. Examples are
gclient_utils.CheckCAllAndFilter(), gcl.RunShellWithReturnCode() and I'd like to
use it in some calls in scm.py, e.g. print the stdout as it goes but still have
access to it.

There's two things that bothers me with the current implementation,
universal_newlines and bufsize. I'll send another separate review for
universal_newlines, bufsize I'm undecided at the moment, I'll probably won't do
anything about it.

[M-A Ruel, 2011-11-08 19:18:06]

Ping.

I've implemented the posix accelerated version. The windows version is a bit
harder to implemented than expected. Handles created with CreatePipe(), used
internally by subprocess.py, cannot be used for asynchronous I/O so I need to do
a tad more hooking to get it to work to create the right kind of pipes depending
on the use case.

[Dirk Pranke, 2011-11-08 20:14:11]

Sorry for the delay; this review is non-trivial and I've been
distracted by other things so I haven't been able to give it the time
I wanted to, but I'll get to it this afternoon for sure.

-- Dirk

On Tue, Nov 8, 2011 at 11:18 AM,  <maruel@chromium.org> wrote:
> Ping.
>
> I've implemented the posix accelerated version. The windows version is a bit
> harder to implemented than expected. Handles created with CreatePipe(), used
> internally by subprocess.py, cannot be used for asynchronous I/O so I need
> to do
> a tad more hooking to get it to work to create the right kind of pipes
> depending
> on the use case.
>
> http://codereview.chromium.org/8374026/
>

[M-A Ruel, 2011-11-08 20:15:40]

On 2011/11/08 20:14:11, Dirk Pranke wrote:
> Sorry for the delay; this review is non-trivial and I've been
> distracted by other things so I haven't been able to give it the time
> I wanted to, but I'll get to it this afternoon for sure.

np, no hurry.

[Dirk Pranke, 2011-11-09 03:43:17]

lgtm

[commit-bot: I haz the power, 2011-11-09 13:52:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/8374026/8001

[commit-bot: I haz the power, 2011-11-09 13:57:59]

Change committed as 109239