future and completer

future and completer


BUG=
TEST=

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

[Siggi Cherem (dart-lang), 2011-10-13 20:45:44]

http://codereview.chromium.org/8271014/diff/1/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/8271014/diff/1/corelib/src/future.dart#newcode73
corelib/src/future.dart:73: void handleException(bool onException(Object
exception));
Just to document here our discussion, even though none of us is learning towards
this direction, but we could also use the event listener API here:

interface FutureListener<T> {
  EventListenerList get exception();
}

interface Future<T> {
  T get value();
  Object get exception();

  bool get isComplete();
  bool get hasValue();

  void then(void onComplete(T value));
   
  FutureListener<T> get on();
}

So you can do:
Future<T> f = ...;
f.on.exception.add((e) { doSomething(); });

http://codereview.chromium.org/8271014/diff/1/corelib/src/future.dart#newcode102
corelib/src/future.dart:102: interface Completer<T> factory CompleterImpl<T> {
Here is the simplified interface we discussed this afternoon:
- rename setValue to complete, and setException to completeException so that
it's clear that the methods have the side effect of completing the future
- remove the complete(valueOrException)

interface Completer<T> {
  Completer();
  Future get future();
  void complete(T value);
  void completeException(Object exception);
}

[arv (Not doing code reviews), 2011-10-13 21:15:46]

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart#newc...
corelib/src/future.dart:112: void completeException(Object exception);
Can this be called fail or reject instead?

http://codereview.chromium.org/8271014/diff/4001/corelib/src/implementation/f...
File corelib/src/implementation/future_implementation.dart (right):

http://codereview.chromium.org/8271014/diff/4001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:82: onComplete(value);
I know Mark has argued that this must be called async.

[Siggi Cherem (dart-lang), 2011-10-13 21:51:48]

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart#newc...
corelib/src/future.dart:112: void completeException(Object exception);
On 2011/10/13 21:15:46, arv wrote:
> Can this be called fail or reject instead?

One reason we were leaning towards 'completeException' was to emphasize that you
can only complete a future one way or the other. So futures would simply have 2
states:
- not complete
- complete => either with a value or with an error

[mattsh, 2011-10-13 22:08:49]

http://codereview.chromium.org/8271014/diff/4001/corelib/src/implementation/f...
File corelib/src/implementation/future_implementation.dart (right):

http://codereview.chromium.org/8271014/diff/4001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:82: onComplete(value);
On 2011/10/13 21:15:46, arv wrote:
> I know Mark has argued that this must be called async.
Right, I agree.  So I think I should change this to use setTimeout here.

[MarkM, 2011-10-13 22:35:35]

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart#newc...
corelib/src/future.dart:65: * for the value), then the exception will be thrown
when it is set.
"...when it is set." 
When what is set?

http://codereview.chromium.org/8271014/diff/4001/corelib/src/future.dart#newc...
corelib/src/future.dart:68: * because the exception associated with this Future
will propagate naturally
If the future is being consumed only via "then", I don't understand how the
exception propagates.

[mattsh, 2011-10-14 21:28:50]

now added Future.dart to gypi and lib files.

[Jacob, 2011-10-14 21:31:28]

LGTM

Lets check this in now and support both future and promise for a little while so
that we can start experimenting with the new API.

[Siggi Cherem (dart-lang), 2011-10-14 22:00:55]

Can we port the a subset of promise tests before submitting?

At least half of the test here are still applicable:
tests/corelib/src/PromiseTest.dart

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
File corelib/src/implementation/future_implementation.dart (right):

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:39: bool
_exceptionHandled;
any need to make this a field, and not a local var inside _complete()?

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:100: }
could we fold together these 2 conditionals? for instance:

if (_exception != null) {
  for (Function handler in _exceptionHandlers) {
    if (handler(_exception)) {
      _exceptionHandled = true;
    }
  }
  if (!_eceptionHandled && _listeners.length > 0) {
    throw _exception;
  }
} else {
  for (var listener in  _listener) { listener(value); }
}

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:112: void _setValue(T
value) {
_setValue => _completeValue

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:120: void
_setException(var exception) {
=> _completeException

[mattsh, 2011-10-14 22:09:19]

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
File corelib/src/implementation/future_implementation.dart (right):

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:39: bool
_exceptionHandled;
On 2011/10/14 22:00:55, sigmund wrote:
> any need to make this a field, and not a local var inside _complete()?

Good point, yes, it should be.  Will do in followup CL.

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:100: }
On 2011/10/14 22:00:55, sigmund wrote:
> could we fold together these 2 conditionals? for instance:
> 
> if (_exception != null) {
>   for (Function handler in _exceptionHandlers) {
>     if (handler(_exception)) {
>       _exceptionHandled = true;
>     }
>   }
>   if (!_eceptionHandled && _listeners.length > 0) {
>     throw _exception;
>   }
> } else {
>   for (var listener in  _listener) { listener(value); }
> }
> 
(Private method, doesn't affect the inteface, but still good suggestion.)

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:112: void _setValue(T
value) {
On 2011/10/14 22:00:55, sigmund wrote:
> _setValue => _completeValue

right

http://codereview.chromium.org/8271014/diff/9001/corelib/src/implementation/f...
corelib/src/implementation/future_implementation.dart:120: void
_setException(var exception) {
On 2011/10/14 22:00:55, sigmund wrote:
> => _completeException
right

[Siggi Cherem (dart-lang), 2011-10-14 22:13:06]

Some additional comments...

http://codereview.chromium.org/8271014/diff/11003/corelib/src/future.dart
File corelib/src/future.dart (right):

http://codereview.chromium.org/8271014/diff/11003/corelib/src/future.dart#new...
corelib/src/future.dart:9: * A Future is used to obtain a value sometime in the
To match the dart doc syntax, we should use [Future], [Completer], [then], etc
in the text here and all the documentation further below

http://codereview.chromium.org/8271014/diff/11003/corelib/src/future.dart#new...
corelib/src/future.dart:16: *
for code snippets use:
 [: 
  ... code
 :]

http://codereview.chromium.org/8271014/diff/11003/corelib/src/implementation/...
File corelib/src/implementation/future_implementation.dart (right):

http://codereview.chromium.org/8271014/diff/11003/corelib/src/implementation/...
corelib/src/implementation/future_implementation.dart:8: class
FutureNotCompleteException implements Exception {
shouldn't this be declared in future.dart (this is part of the user API)?