Reimplement r109239 but using Popen.communicate() instead.

Reimplement r109239 but using Popen.communicate() instead.

Enables threaded callback handler for subprocess.communicate().

R=dpranke@chromium.org
BUG=
TEST=


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

[M-A Ruel, 2011-11-30 20:44:51]

Tested manually on linux, mac, windows and cygwin.

http://codereview.chromium.org/8749015/diff/1/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/8749015/diff/1/subprocess2.py#newcode277
subprocess2.py:277: 'wait': threading.Thread(target=wait_fn),
By putting wait in its own thread, I don't need to do a double-loop anymore and
the try/finally clause makes much more sense now.

[Dirk Pranke, 2011-11-30 21:06:44]

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode220
subprocess2.py:220: """Does I/O for a process's pipes using thread.
Nit: "threads".

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode279
subprocess2.py:279: 'wait': threading.Thread(target=wait_fn),
Why do you need this one? Shouldn't the other threads all abort when the process
exits, anyway? It seems like the write_stdin() thread might be blocked in
stdin_io.read(), but I don't see how that gets interrupted regardless of what
else is going on?

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode292
subprocess2.py:292: t.daemon = True
Are  you sure you want these to be daemons? I suppose the plus side is that
lingering threads won't keep the process from exiting, but the downside is that
this might paper over bugs ...

[Dirk Pranke, 2011-11-30 21:07:56]

Oh, for the record (i.e., me, when I review the next version ...):

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

http://codereview.chromium.org/8374026

[M-A Ruel, 2011-11-30 21:26:03]

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode279
subprocess2.py:279: 'wait': threading.Thread(target=wait_fn),
On 2011/11/30 21:06:44, Dirk Pranke wrote:
> Why do you need this one? Shouldn't the other threads all abort when the
process
> exits, anyway? It seems like the write_stdin() thread might be blocked in
> stdin_io.read(), but I don't see how that gets interrupted regardless of what
> else is going on?

(Argh, rietveld ate my comment)

In the previous code, there was a risk that it could hang in this condition:
- Child process sends a lot of data to pipes
- Exits really fast
- A timeout is used
- It loops with self.returncode is still None even though the child process just
exited (race condition)

It get stuck at queue.get() because the only remaining thread is 'timeout'.

Having a worker wait thread fixes that.

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode292
subprocess2.py:292: t.daemon = True
On 2011/11/30 21:06:44, Dirk Pranke wrote:
> Are  you sure you want these to be daemons? I suppose the plus side is that
> lingering threads won't keep the process from exiting, but the downside is
that
> this might paper over bugs ...

Right, it's not needed anymore. Removed.

[Dirk Pranke, 2011-11-30 22:15:54]

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py
File subprocess2.py (right):

http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode279
subprocess2.py:279: 'wait': threading.Thread(target=wait_fn),
On 2011/11/30 21:26:04, Marc-Antoine Ruel wrote:
> On 2011/11/30 21:06:44, Dirk Pranke wrote:
> > Why do you need this one? Shouldn't the other threads all abort when the
> process
> > exits, anyway? It seems like the write_stdin() thread might be blocked in
> > stdin_io.read(), but I don't see how that gets interrupted regardless of
what
> > else is going on?
> 
> (Argh, rietveld ate my comment)
> 
> In the previous code, there was a risk that it could hang in this condition:
> - Child process sends a lot of data to pipes
> - Exits really fast
> - A timeout is used
> - It loops with self.returncode is still None even though the child process
just
> exited (race condition)
> 
> It get stuck at queue.get() because the only remaining thread is 'timeout'.
> 
> Having a worker wait thread fixes that.

Hm. Okay, I can see that. I think you could get rid of the wait thread by
ensuring that the other threads post a 'done' event before exiting and handing
self.poll() is not None in the else block. 

But I'm not sure if that's much better or worse than this; I don't know what the
cost of the additional thread is.

lgtm.

[Dirk Pranke, 2011-11-30 22:20:50]

un-lgtm! I think you didn't answer this question ...

> On 2011/11/30 21:06:44, Dirk Pranke wrote:
> It seems like the write_stdin() thread might be blocked in
> stdin_io.read(), but I don't see how that gets interrupted regardless of what
> else is going on?

[M-A Ruel, 2011-12-01 00:45:22]

On 2011/11/30 22:15:54, Dirk Pranke wrote:
> http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode279
> subprocess2.py:279: 'wait': threading.Thread(target=wait_fn),
> On 2011/11/30 21:26:04, Marc-Antoine Ruel wrote:
> > 
> > It get stuck at queue.get() because the only remaining thread is 'timeout'.
> > 
> > Having a worker wait thread fixes that.
> 
> Hm. Okay, I can see that. I think you could get rid of the wait thread by
> ensuring that the other threads post a 'done' event before exiting and handing
> self.poll() is not None in the else block. 
> 
> But I'm not sure if that's much better or worse than this; I don't know what
the
> cost of the additional thread is.

Then the problem is when no pipe is listened to, so only the timeout would fire.


On 2011/11/30 22:20:50, Dirk Pranke wrote:
> un-lgtm! I think you didn't answer this question ...
> 
> > On 2011/11/30 21:06:44, Dirk Pranke wrote:
> > It seems like the write_stdin() thread might be blocked in
> > stdin_io.read(), but I don't see how that gets interrupted regardless of
what
> > else is going on?

stdin_io is a cStringIO is it's not 'blocking'.
If the process terminates early, the self.stdin.write(data) call will return
since the pipe will not have a listener anymore.

[Dirk Pranke, 2011-12-01 00:59:09]

On 2011/12/01 00:45:22, Marc-Antoine Ruel wrote:
> On 2011/11/30 22:15:54, Dirk Pranke wrote:
> > http://codereview.chromium.org/8749015/diff/4001/subprocess2.py#newcode279
> > subprocess2.py:279: 'wait': threading.Thread(target=wait_fn),
> > On 2011/11/30 21:26:04, Marc-Antoine Ruel wrote:
> > > 
> > > It get stuck at queue.get() because the only remaining thread is
'timeout'.
> > > 
> > > Having a worker wait thread fixes that.
> > 
> > Hm. Okay, I can see that. I think you could get rid of the wait thread by
> > ensuring that the other threads post a 'done' event before exiting and
handing
> > self.poll() is not None in the else block. 
> > 
> > But I'm not sure if that's much better or worse than this; I don't know what
> the
> > cost of the additional thread is.
> 
> Then the problem is when no pipe is listened to, so only the timeout would
fire.
> 

Good point.

> 
> On 2011/11/30 22:20:50, Dirk Pranke wrote:
> > un-lgtm! I think you didn't answer this question ...
> > 
> > > On 2011/11/30 21:06:44, Dirk Pranke wrote:
> > > It seems like the write_stdin() thread might be blocked in
> > > stdin_io.read(), but I don't see how that gets interrupted regardless of
> what
> > > else is going on?
> 
> stdin_io is a cStringIO is it's not 'blocking'.
> If the process terminates early, the self.stdin.write(data) call will return
> since the pipe will not have a listener anymore.

Ah, right.

LGTM.

[commit-bot: I haz the power, 2011-12-01 14:58:40]

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

[commit-bot: I haz the power, 2011-12-01 15:04:59]

Change committed as 112465