Change Process.start to return a future that completes with a

Change Process.start to return a future that completes with a
Process object only when the process has been successfully started.

This is less error prone since you can use all the methods on a
Process object as soon as you can get your hands on it. Unfortunately,
this change leads to quite a few changes in test.dart. Some of those
changes will fix bugs as well. We used to have a problem with a 
timeout that could fire just in between two steps in a chain of
commands that constitute one test run. This change should address
that issue as well.

BUG=dartbug.com/5305

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

[Mads Ager (google), 2012-10-11 16:00:53]

Sorry for the long list of reviewers.

Søren and Rico, could you review the dart:io part of this.

Emily, Peter and Graham, could you have a look at the test.dart changes?

Thanks!

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1149: browserUsed =
test.configuration['browser'];
This is not right. browserUsed is supposed to be a String but
configuration['browser'] is a bool. We probably need to use
configuration['runtime'] here, but I don't want to make that change now. There
are enough moving pieces in this change already. Emily, since browserUsed
becomes a boolean here which is not equal to any string, can we just get rid of
this and the special cases we have based on browserUsed?

[gram, 2012-10-11 16:50:28]

The changes in tools/testing/dart and utils/testrunner look good to me. There is
a slight chance that this introduces a race in
utils/testrunner/run_pipeline.dart but that may have been present already, and
I'll fixed that separately.

[ricow1, 2012-10-11 17:27:34]

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/dart_std_i...
File tests/standalone/io/dart_std_io_pipe_test.dart (right):

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/dart_std_i...
tests/standalone/io/dart_std_io_pipe_test.dart:73:
Expect.fail(error.toString());
we should probably do a dir.deleteRecursivelySync() in here as well, otherwise
this will be leftover if this fails.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:694: if (timedOut) process.kill();
Why not just do p.kill() at the start of this closure and then return?

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:925: _startProcess(() { _reportResult(); });
why not just pass _reportResult in here instead of making an extra closure?

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:949: _startProcess(then) {
should we change "then" to another name here to not confuse this with the Future
then (e.g., setupDone or simply callback

http://codereview.chromium.org/11091070/diff/1/utils/testrunner/layout_test_c...
File utils/testrunner/layout_test_controller.dart (right):

http://codereview.chromium.org/11091070/diff/1/utils/testrunner/layout_test_c...
utils/testrunner/layout_test_controller.dart:165: .replaceAll(const
RegExp("[^A-Za-z0-9]"),"_")}.txt';
indentaion seems random

[Emily Fortuna, 2012-10-11 18:23:07]

lgtm after the comments are addressed

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:681: new StringInputStream(stdoutStream);
consider:
var stdoutStringStream = new StringInputStream(stdoutStream);

otherwise we're starting to look an awful lot like Java

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:949: _startProcess(then) {
On 2012/10/11 17:27:34, ricow1 wrote:
> should we change "then" to another name here to not confuse this with the
Future
> then (e.g., setupDone or simply callback

+1

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1149: browserUsed =
test.configuration['browser'];
On 2012/10/11 16:00:54, Mads Ager wrote:
> This is not right. browserUsed is supposed to be a String but
> configuration['browser'] is a bool. We probably need to use
> configuration['runtime'] here, but I don't want to make that change now. There
> are enough moving pieces in this change already. Emily, since browserUsed
> becomes a boolean here which is not equal to any string, can we just get rid
of
> this and the special cases we have based on browserUsed?

Right. I've put in this CL: https://codereview.chromium.org/11098075/ to fix
this.

http://codereview.chromium.org/11091070/diff/1/utils/testrunner/layout_test_c...
File utils/testrunner/layout_test_controller.dart (right):

http://codereview.chromium.org/11091070/diff/1/utils/testrunner/layout_test_c...
utils/testrunner/layout_test_controller.dart:165: .replaceAll(const
RegExp("[^A-Za-z0-9]"),"_")}.txt';
On 2012/10/11 17:27:34, ricow1 wrote:
> indentaion seems random

just -1 space

[ahe, 2012-10-11 18:39:50]

Changes to tools/testing/dart, LGTM!

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:673: Future processFuture =
Process.start(command.executable, command.arguments);
Consider using cascading here.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:696: processFuture.handleException((e) {
Shouldn't this API be the same as process.onError so you don't have to wrap the
exception handler.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:945: // exit.
+1000!

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:960: _processErrorHandler(e);
Aren't you getting tired of wrapping this? ;-)

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1121: processFuture.then((p) {
A type for p would be nice.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1140: processFuture.handleException((e) {
Boring! ;-)

[Søren Gjesse, 2012-10-12 07:47:21]

LGTM, looks much cleaner

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process.dart#newco...
runtime/bin/process.dart:83: abstract void set onExit(void callback(int
exitCode));
Shouldn't this return a future (with the exit code) as well?

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process_impl.dart#...
runtime/bin/process_impl.dart:282: void set onError(void callback(e)) {
Shouldn't we try to get rid of onError as well? As far as I can see besides
startup errors it is only used to report failures to kill the process.

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/process_wo...
File tests/standalone/io/process_working_directory_test.dart (right):

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/process_wo...
tests/standalone/io/process_working_directory_test.dart:47: var future =
Process.start(fullTestFilePath,
One could consider writing this with cascades and avoid the local variable.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_suite....
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_suite....
tools/testing/dart/test_suite.dart:99: });
This code is now 7 lines longer...

[Mads Ager (google), 2012-10-12 08:44:46]

Thanks for all the comments!

Rico and Søren, could you have another look. I removed onError on process as
well. This means that kill can throw an exception in rare cases. Something is
fishy if that happens, so I think this is the right thing to do.

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process.dart
File runtime/bin/process.dart (right):

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process.dart#newco...
runtime/bin/process.dart:83: abstract void set onExit(void callback(int
exitCode));
On 2012/10/12 07:47:22, Søren Gjesse wrote:
> Shouldn't this return a future (with the exit code) as well?

That could make sense, yes. Let's experiment with that as a separate change?
This is getting big already.

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/1/runtime/bin/process_impl.dart#...
runtime/bin/process_impl.dart:282: void set onError(void callback(e)) {
On 2012/10/12 07:47:22, Søren Gjesse wrote:
> Shouldn't we try to get rid of onError as well? As far as I can see besides
> startup errors it is only used to report failures to kill the process.

I would love to get rid of onError, that was one of my goals doing this. At
least we should clean up the case where the process is not started. It should
not be possible to get there anymore. There is one other case: we failed to send
a signal to the process. Maybe we should just throw an exception in that case?
Kill is synchronous in any case.  I'll go ahead and do that.

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/dart_std_i...
File tests/standalone/io/dart_std_io_pipe_test.dart (right):

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/dart_std_i...
tests/standalone/io/dart_std_io_pipe_test.dart:73:
Expect.fail(error.toString());
On 2012/10/11 17:27:34, ricow1 wrote:
> we should probably do a dir.deleteRecursivelySync() in here as well, otherwise
> this will be leftover if this fails.

Done for this particular test. I'm pretty sure that we do not clean up on
failures in most of the file/directory tests.

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/process_wo...
File tests/standalone/io/process_working_directory_test.dart (right):

http://codereview.chromium.org/11091070/diff/1/tests/standalone/io/process_wo...
tests/standalone/io/process_working_directory_test.dart:47: var future =
Process.start(fullTestFilePath,
On 2012/10/12 07:47:22, Søren Gjesse wrote:
> One could consider writing this with cascades and avoid the local variable.

I actually did a couple of places and removed it again because I found it less
readable.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:673: Future processFuture =
Process.start(command.executable, command.arguments);
On 2012/10/11 18:39:50, ahe wrote:
> Consider using cascading here.

I did and for this code I found it too confusing because the body of the 'then'
part is too big.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:681: new StringInputStream(stdoutStream);
On 2012/10/11 18:23:08, Emily Fortuna wrote:
> consider:
> var stdoutStringStream = new StringInputStream(stdoutStream);
> 
> otherwise we're starting to look an awful lot like Java

Agreed! I kept it as it was but I will be more than happy to go to 'var' here.
:-)

I collapsed this to two lines instead of four. No reason to declare stdoutStream
or stderrStream here either.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:694: if (timedOut) process.kill();
On 2012/10/11 17:27:34, ricow1 wrote:
> Why not just do p.kill() at the start of this closure and then return?

Because we need the exit handler set up so that we will get to the part of the
code that marks the test as completed and moves on to the next test.

Since we need that handler set up I thought that it would be cleanest if we
always set up everything and then in the rare timeout case kill the process
afterwards.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:696: processFuture.handleException((e) {
On 2012/10/11 18:39:50, ahe wrote:
> Shouldn't this API be the same as process.onError so you don't have to wrap
the
> exception handler.

That would be nice, yes. I'll eliminate onError on process instead which will
avoid this issue with double-up error handling as well.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:925: _startProcess(() { _reportResult(); });
On 2012/10/11 17:27:34, ricow1 wrote:
> why not just pass _reportResult in here instead of making an extra closure?

Done.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:949: _startProcess(then) {
On 2012/10/11 17:27:34, ricow1 wrote:
> should we change "then" to another name here to not confuse this with the
Future
> then (e.g., setupDone or simply callback

Done.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1121: processFuture.then((p) {
On 2012/10/11 18:39:50, ahe wrote:
> A type for p would be nice.

Done.

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1140: processFuture.handleException((e) {
On 2012/10/11 18:39:50, ahe wrote:
> Boring! ;-)

Yawn! ;-)

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_runner...
tools/testing/dart/test_runner.dart:1149: browserUsed =
test.configuration['browser'];
On 2012/10/11 18:23:08, Emily Fortuna wrote:
> On 2012/10/11 16:00:54, Mads Ager wrote:
> > This is not right. browserUsed is supposed to be a String but
> > configuration['browser'] is a bool. We probably need to use
> > configuration['runtime'] here, but I don't want to make that change now.
There
> > are enough moving pieces in this change already. Emily, since browserUsed
> > becomes a boolean here which is not equal to any string, can we just get rid
> of
> > this and the special cases we have based on browserUsed?
> 
> Right. I've put in this CL: https://codereview.chromium.org/11098075/ to fix
> this.

Thank you!

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_suite....
File tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/11091070/diff/1/tools/testing/dart/test_suite....
tools/testing/dart/test_suite.dart:99: });
On 2012/10/12 07:47:22, Søren Gjesse wrote:
> This code is now 7 lines longer...

I know. I removed onError on process. That means that killing a process can in
rare cases throw an exception. If it does, something funky is going on though,
so I think removing onError on process is the right thing to do.

http://codereview.chromium.org/11091070/diff/1/utils/testrunner/layout_test_c...
File utils/testrunner/layout_test_controller.dart (right):

http://codereview.chromium.org/11091070/diff/1/utils/testrunner/layout_test_c...
utils/testrunner/layout_test_controller.dart:165: .replaceAll(const
RegExp("[^A-Za-z0-9]"),"_")}.txt';
On 2012/10/11 17:27:34, ricow1 wrote:
> indentaion seems random

Done. It was off in the original too.

[Anders Johnsen, 2012-10-12 08:53:11]

DBC,

Thank you for doing this Mads, I'm sure it'll fix some issues for me as well!

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.d...
runtime/bin/process_impl.dart:247: throw new ProcessException("Could not kill
process");
Maybe change exception to "Could not send kill signal to process". The current
message is not very POSIX friendly.

[Mads Ager (google), 2012-10-12 08:56:25]

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.d...
runtime/bin/process_impl.dart:247: throw new ProcessException("Could not kill
process");
On 2012/10/12 08:53:11, ajohnsen wrote:
> Maybe change exception to "Could not send kill signal to process". The current
> message is not very POSIX friendly.

And an error message about signals is not very Windows friendly. :-)

[Anders Johnsen, 2012-10-12 08:58:10]

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.d...
runtime/bin/process_impl.dart:247: throw new ProcessException("Could not kill
process");
On 2012/10/12 08:56:25, Mads Ager wrote:
> On 2012/10/12 08:53:11, ajohnsen wrote:
> > Maybe change exception to "Could not send kill signal to process". The
current
> > message is not very POSIX friendly.
> 
> And an error message about signals is not very Windows friendly. :-)

Hehe agreed, but if we mention both 'kill' and 'signal' we would cover both? ;)

[Søren Gjesse, 2012-10-12 11:44:04]

LGTM!

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.d...
runtime/bin/process_impl.dart:273: void _reportError(e) {
This is not used any more - right?

[Mads Ager (google), 2012-10-12 11:50:26]

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.dart
File runtime/bin/process_impl.dart (right):

http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.d...
runtime/bin/process_impl.dart:273: void _reportError(e) {
On 2012/10/12 11:44:04, Søren Gjesse wrote:
> This is not used any more - right?

It is still used internally to report startup errors. I'll see if I can locally
restructure to get rid of that. Thanks!

[Mads Ager (google), 2012-10-12 12:40:45]

On 2012/10/12 11:50:26, Mads Ager wrote:
>
http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.dart
> File runtime/bin/process_impl.dart (right):
> 
>
http://codereview.chromium.org/11091070/diff/11002/runtime/bin/process_impl.d...
> runtime/bin/process_impl.dart:273: void _reportError(e) {
> On 2012/10/12 11:44:04, Søren Gjesse wrote:
> > This is not used any more - right?
> 
> It is still used internally to report startup errors. I'll see if I can
locally
> restructure to get rid of that. Thanks!

Restructured a bit so _onError and _onStart are gone from _Process as well.
Thanks!