Change dart:io to use Future for one-shot operations.

Change dart:io to use Future for one-shot operations.

This appears to be what a lot of our users are doing and it is the
style used in our other libraries.

Noteworthy changes:

- File and Directory objects are now immutable.

- Directory.list returns an active lister object.

- Write operations on files return futures and the
  onNoPendingWrites handler is gone. The same
  functionality is there by using 'then' on the
  future returned from the last write operation.

- Process starting is now done with static methods:
  -- Process.start yields an InteractiveProcess.
  -- Process.run yields a Future<ProcessResult>

BUG=
TEST=

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

[Mads Ager (google), 2012-05-10 10:23:05]

Søren and Rico, could you guys perform the review of the actual dart:io changes?

Bob, could you have a look at the pub io.dart changes?

The rest of you: this is just FYI since you have registered an interest in this.
:)

[Søren Gjesse, 2012-05-10 11:24:45]

LGTM!

- with a few comments.

I like this change after all. The API looks more in sync with the rest of the
Dart APIs and the code is just as readable.

Making Directory and File immutable is also a win.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
File runtime/bin/directory.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
runtime/bin/directory.dart:24: * a [:Future<bool>:] the completes with the
result.
the completes -> that completes

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
runtime/bin/directory.dart:117: * called if a subdirectory cannot be opened for
listing.
Maybe add comment saying that directory listing is terminated if an error
occurs.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
File runtime/bin/directory_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
runtime/bin/directory_impl.dart:91: "Creation of temporary directory failed");
Long line.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/file_im...
runtime/bin/file_impl.dart:155: _onNoPendingWrites();
Call this on the event loop using a Timer.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/file_im...
runtime/bin/file_impl.dart:718: // then handler.
So we do always want to throw exceptions through the future - even if the
exception happens before the async part of the operation has been started?

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/process...
File runtime/bin/process_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/process...
runtime/bin/process_impl.dart:394: _ProcessResult(int this._exitCode, String
this._stdout, String this._stderr);
Can't this be a const constructor? Then the fields can be final and without the
_ prefix. Then the getters are not required.

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
File tests/standalone/io/directory_error_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
tests/standalone/io/directory_error_test.dart:41:
inNonExistent.create().handleException((e) {
Shouldn't there be a then handler here as well? If create completes without
exception then the test will still pass.

The same for more tests below.

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
File tests/standalone/io/file_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
tests/standalone/io/file_test.dart:225:
file3.open(FileMode.READ).then((RandomAccessFile openedFile3) {
Long line.

[ricow1, 2012-05-10 11:48:01]

LGTM

https://chromiumcodereview.appspot.com/10392023/diff/4001/runtime/bin/process...
File runtime/bin/process_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/runtime/bin/process...
runtime/bin/process_impl.dart:160: _reportError(new
ProcessException(status._errorMessage, status._errorCode));
Long line

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
File tests/standalone/io/directory_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/directory_test.dart:67: lister.onDone = (done) {
I would probably rename done to success, makes more sense

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/directory_test.dart:355: temp.path +
"/nested_temp_dir_${createdDirectories.length}_");
"${temp.path/nest..." instead?

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
File tests/standalone/io/file_invalid_arguments_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_invalid_arguments_test.dart:103: Expect.fail('write
list invalid argument');
this is write byte, not write list

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_invalid_arguments_test.dart:125:
writeListFuture.then((ignore) => Expect.fail('write string invalid argument'));
long line and this is not write string, it is write list (in the error message

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
File tests/standalone/io/file_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_test.dart:225:
file3.open(FileMode.READ).then((RandomAccessFile openedFile3) {
long line

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_test.dart:400: Expect.isTrue(e is FileIOException);
Don't we need to return true from this closure

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_test.dart:467: dirFuture.handleException((e) {
this is really hard to read, could we call dirFuture something like
dirFuture{1,2,3} instead of reusing it

[Mads Ager (google), 2012-05-10 12:42:37]

Thanks for the comments!

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
File runtime/bin/directory.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
runtime/bin/directory.dart:24: * a [:Future<bool>:] the completes with the
result.
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> the completes -> that completes

Done.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
runtime/bin/directory.dart:117: * called if a subdirectory cannot be opened for
listing.
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> Maybe add comment saying that directory listing is terminated if an error
> occurs.

It isn't really. If a directory cannot be opened, that part of the traversal
cannot continue, however, other directories  are still listed after that.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
File runtime/bin/directory_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/directo...
runtime/bin/directory_impl.dart:91: "Creation of temporary directory failed");
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/file_im...
runtime/bin/file_impl.dart:155: _onNoPendingWrites();
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> Call this on the event loop using a Timer.

Done.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/file_im...
runtime/bin/file_impl.dart:718: // then handler.
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> So we do always want to throw exceptions through the future - even if the
> exception happens before the async part of the operation has been started?

Yes, at least I did that consistently for the file operations. The reason for
doing that was that we called onError before if it was registered. If there is
no handleException on the future this will throw the exception in any case.

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/process...
File runtime/bin/process_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/runtime/bin/process...
runtime/bin/process_impl.dart:394: _ProcessResult(int this._exitCode, String
this._stdout, String this._stderr);
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> Can't this be a const constructor? Then the fields can be final and without
the
> _ prefix. Then the getters are not required.

Good call. Yes it can.

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
File tests/standalone/io/directory_error_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
tests/standalone/io/directory_error_test.dart:41:
inNonExistent.create().handleException((e) {
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> Shouldn't there be a then handler here as well? If create completes without
> exception then the test will still pass.
> 
> The same for more tests below.

No, not needed. If handleException is not called, done() will not be called and
the test will hang.

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
File tests/standalone/io/file_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2001/tests/standalone/io...
tests/standalone/io/file_test.dart:225:
file3.open(FileMode.READ).then((RandomAccessFile openedFile3) {
On 2012/05/10 11:24:45, Søren Gjesse wrote:
> Long line.

Done.

https://chromiumcodereview.appspot.com/10392023/diff/4001/runtime/bin/process...
File runtime/bin/process_impl.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/runtime/bin/process...
runtime/bin/process_impl.dart:160: _reportError(new
ProcessException(status._errorMessage, status._errorCode));
On 2012/05/10 11:48:01, ricow1 wrote:
> Long line

Done.

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
File tests/standalone/io/directory_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/directory_test.dart:67: lister.onDone = (done) {
On 2012/05/10 11:48:01, ricow1 wrote:
> I would probably rename done to success, makes more sense

Done here and above.

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/directory_test.dart:355: temp.path +
"/nested_temp_dir_${createdDirectories.length}_");
On 2012/05/10 11:48:01, ricow1 wrote:
> "${temp.path/nest..." instead?

'${temp.path}/nest...'

Done.

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
File tests/standalone/io/file_invalid_arguments_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_invalid_arguments_test.dart:103: Expect.fail('write
list invalid argument');
On 2012/05/10 11:48:01, ricow1 wrote:
> this is write byte, not write list

Good catch! Done.

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_invalid_arguments_test.dart:125:
writeListFuture.then((ignore) => Expect.fail('write string invalid argument'));
On 2012/05/10 11:48:01, ricow1 wrote:
> long line and this is not write string, it is write list (in the error message

Another good catch. Thanks!

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
File tests/standalone/io/file_test.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_test.dart:225:
file3.open(FileMode.READ).then((RandomAccessFile openedFile3) {
On 2012/05/10 11:48:01, ricow1 wrote:
> long line

Done.

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_test.dart:400: Expect.isTrue(e is FileIOException);
On 2012/05/10 11:48:01, ricow1 wrote:
> Don't we need to return true from this closure

Yes, thank you!!!

This test is completely wrong. The expected behavior is that readByte will
return -1 as end-of-file has been reached. So we don't need a handleException
here at all. Having the exception thrown and the test fail is the right thing.

This revealed another issue: the test did not have an equal number of
asyncTestStarted and asyncTestDone calls. I have fixed that and I have added a
receive port that is only closed if all async tests complete. That way the test
will hang if we get it wrong instead of just leaving temporary directories all
over the place.

https://chromiumcodereview.appspot.com/10392023/diff/4001/tests/standalone/io...
tests/standalone/io/file_test.dart:467: dirFuture.handleException((e) {
On 2012/05/10 11:48:01, ricow1 wrote:
> this is really hard to read, could we call dirFuture something like
> dirFuture{1,2,3} instead of reusing it

Done.

[Bob Nystrom, 2012-05-10 17:14:21]

Pub stuff LGTM. Nathan and I will likely get rid of much of that over time now
that it isn't needed.

This looks fantastic. I think people will be really excited to see it.

https://chromiumcodereview.appspot.com/10392023/diff/2003/runtime/bin/process...
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2003/runtime/bin/process...
runtime/bin/process.dart:51: interface InteractiveProcess {
The split between this and Process is unclear. Does this class exist simply
because you can't have static methods on interfaces?

Have you considered doing:

class Process {
  static Process start(...) {
    return _Process.start(executable, arguments, options);
  }

  static Future<ProcessResult> run(...);
    return _Process.run(executable, arguments, options);
  }

  InputStream get stdout();
  void close();
  // etc...
}

class _Process implements Process { ... }

This way, users still see a single simple concrete Process class, but the actual
implementation is still abstracted away like you want.

[Mads Ager (google), 2012-05-10 17:33:18]

https://chromiumcodereview.appspot.com/10392023/diff/2003/runtime/bin/process...
File runtime/bin/process.dart (right):

https://chromiumcodereview.appspot.com/10392023/diff/2003/runtime/bin/process...
runtime/bin/process.dart:51: interface InteractiveProcess {
On 2012/05/10 17:14:21, Bob Nystrom wrote:
> The split between this and Process is unclear. Does this class exist simply
> because you can't have static methods on interfaces?
> 
> Have you considered doing:
> 
> class Process {
>   static Process start(...) {
>     return _Process.start(executable, arguments, options);
>   }
> 
>   static Future<ProcessResult> run(...);
>     return _Process.run(executable, arguments, options);
>   }
> 
>   InputStream get stdout();
>   void close();
>   // etc...
> }
> 
> class _Process implements Process { ... }
> 
> This way, users still see a single simple concrete Process class, but the
actual
> implementation is still abstracted away like you want.

Thanks Bob! That makes a lot of sense to me. I have attempted to avoid
introducing other classes than Process itself here before so I can't really
explain why I went for InteractiveProcess now. :-)

In any case I think you are right that having just the Process class with
instances representing processes with which you can interact using streams is
better. I'll update the code with that tomorrow morning and then land.

[Mads Ager (google), 2012-05-10 19:04:04]

On 2012/05/10 17:33:18, Mads Ager wrote:
>
https://chromiumcodereview.appspot.com/10392023/diff/2003/runtime/bin/process...
> File runtime/bin/process.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10392023/diff/2003/runtime/bin/process...
> runtime/bin/process.dart:51: interface InteractiveProcess {
> On 2012/05/10 17:14:21, Bob Nystrom wrote:
> > The split between this and Process is unclear. Does this class exist simply
> > because you can't have static methods on interfaces?
> > 
> > Have you considered doing:
> > 
> > class Process {
> >   static Process start(...) {
> >     return _Process.start(executable, arguments, options);
> >   }
> > 
> >   static Future<ProcessResult> run(...);
> >     return _Process.run(executable, arguments, options);
> >   }
> > 
> >   InputStream get stdout();
> >   void close();
> >   // etc...
> > }
> > 
> > class _Process implements Process { ... }
> > 
> > This way, users still see a single simple concrete Process class, but the
> actual
> > implementation is still abstracted away like you want.
> 
> Thanks Bob! That makes a lot of sense to me. I have attempted to avoid
> introducing other classes than Process itself here before so I can't really
> explain why I went for InteractiveProcess now. :-)
> 
> In any case I think you are right that having just the Process class with
> instances representing processes with which you can interact using streams is
> better. I'll update the code with that tomorrow morning and then land.

OK, got rid of InteractiveProcess. This is a little funky because the stdout,
close, etc. methods are abstract in the public Process class. However, start
returns an instance of a subclass with actual implementations.

The way I would really like to do this is to have support for statics in
interfaces that have default implementations. I know there has been  talk about
adding that. If that happens we can easily transition this to a nice interface.
User code will look the same either way.

[Siggi Cherem (dart-lang), 2012-05-12 00:49:41]

lgtm! very very cool!