Cancel stdout/stderr subscriptions once subprocesses are dead, add more debugging information in ca…

Cancel stdout/stderr subscriptions once subprocesses are dead, add more debugging information in case the debug timer expires

R=whesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31649

[kustermann, 2014-01-08 17:42:07]

@ajohnsen: We're relying on
  process.stdout.listen().cancel()
  process.stderr.listen().cancel()
to actually close the underlying file descriptor (even if the subprocess is
dead, subprocesses of the subprocess may still keep these fds live).

[Bill Hesse, 2014-01-08 17:43:53]

We have to give a process's stdout and stderr some time to send their data and
close after the process exit event happens.  We don't control the order of these
events.  So we really should just have a timeout on the state where exit has
happened but the streams are still open.  And this timeout will only come into
play in case the bug actually occurs.

[Bill Hesse, 2014-01-08 18:32:18]

LGTM, with a suggestion.

https://codereview.chromium.org/128703002/diff/90001/dart/tools/testing/dart/...
File dart/tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/128703002/diff/90001/dart/tools/testing/dart/...
dart/tools/testing/dart/browser_controller.dart:155: stdoutDone.complete();
Can we put a cancel for the timer in here:
 if (stderrIsDone) watchdogTimer.cancel();

?

And similarly for closeStderr, and keep the timer in a variable?

I would feel better about not keeping the timers around, keeping the objects
live.

https://codereview.chromium.org/128703002/diff/90001/dart/tools/testing/dart/...
dart/tools/testing/dart/browser_controller.dart:187: 
Check if both stdout and stderr are closed already, and don't setup the timer if
so?  Because we will never cancel it in that case.

[Anders Johnsen, 2014-01-09 06:22:28]

Yes, this should work. But I think Bill may be right that this timer will keep
the test-runner alive, even when all tests are 'done'.

[kustermann, 2014-01-09 08:25:35]

https://codereview.chromium.org/128703002/diff/90001/dart/tools/testing/dart/...
File dart/tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/128703002/diff/90001/dart/tools/testing/dart/...
dart/tools/testing/dart/browser_controller.dart:155: stdoutDone.complete();
On 2014/01/08 18:32:19, Bill Hesse wrote:
> Can we put a cancel for the timer in here:
>  if (stderrIsDone) watchdogTimer.cancel();
> 
> ?
> 
> And similarly for closeStderr, and keep the timer in a variable?
> 
> I would feel better about not keeping the timers around, keeping the objects
> live.

Good point.

https://codereview.chromium.org/128703002/diff/90001/dart/tools/testing/dart/...
dart/tools/testing/dart/browser_controller.dart:187: 
On 2014/01/08 18:32:19, Bill Hesse wrote:
> Check if both stdout and stderr are closed already, and don't setup the timer
if
> so?  Because we will never cancel it in that case.

Done

[kustermann, 2014-01-09 09:05:15]

Committed patchset #3 manually as r31649 (presubmit successful).