convert isolates to use Future

convert isolates to use Future


BUG=
TEST=

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

[mattsh, 2011-11-04 03:17:03]

This changes Isolate.spawn to return a Future rather than a Promise, and changes
the dartcombat sample to use Future and Completer rather than Promise.  (Also,
added a Futures class with a wait method to enable waiting on a list of Future
and collecting their values.)
(Also changed DOM APIs to use Future rather than Promise.)

[Jacob, 2011-11-04 03:19:57]

http://codereview.chromium.org/8457005/diff/2001/frog/lib/dom/html.dart
File frog/lib/dom/html.dart (left):

http://codereview.chromium.org/8457005/diff/2001/frog/lib/dom/html.dart#oldco...
frog/lib/dom/html.dart:18712: Element createElement([String tagName]);
actually, i believe this file is obsolete. go ahead and delete now that frog is
using
client/html/html.dart

[Ben Laurie (Google), 2011-11-04 11:48:12]

Two main concerns:

1. The stub generator needs to be updated, too.

2. [addCompleteHandler] has become [then] and [then] has disappeared. This does
not seem like a step forward - we need a replacement for [then] (or a different
name for the new [then]).

Wasn't there some proposal for [then] being as it is now on Promises and
[addCompleteHandler] becoming [when]?

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
File client/samples/dartcombat/state.dart (right):

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:147: res.then((int result) {
If then no longer returns a promise you are going to break a lot of code.

Did we really agree that interface?

[Siggi Cherem (dart-lang), 2011-11-04 16:57:33]

I'd like if we can actually split this change in 2 - changing Isolate.spawn is
scary enough to do it in its own proper CL. 

Then we can do individual CLs to change each piece that uses Promises
(dartcombat, mint maker, etc)

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
File client/samples/dartcombat/state.dart (right):

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:147: res.then((int result) {
On 2011/11/04 11:48:13, Ben Laurie (Google) wrote:
> If then no longer returns a promise you are going to break a lot of code.
> 
> Did we really agree that interface?

I don't think we are finalized on the interface. But, I'd like to see how much
we can simplify things. 

I think the behavior of the old [then] is very useful, so we definitively want
it somewhere. For the time being, how about we provide it as a helper function
in [Futures]? 

My thinking is that we should consider adding it back in [Future] once we see
how much this affects our code readability. Doing a quick grep, I only found 6
places where we clearly used the returned value of [then] as it used to be, but
I'm not convinced either way yet.

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:152: return true;
I wonder if we can do something here to avoid writing return true all the time.
I'd like essentially an optional return, and consider the exception handled by
default.

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

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:116: class Futures {
+ comment on this class

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:124: Completer completer = new Completer<Object>();
Compelter<Object> -> Completer<List>?

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:126: List<Object> values = new List(futures.length);
List<Object> -> List (let it be dynamic)

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:131: int pos = i;
add here a TODO to remove this once loop-var capture works also in this kind of
for-loops (as it does in for-in loops).

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:132: futures[pos].then((Object value) {
Object -> var, or remove the type annotation.

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:140: }
to address Ben's concern, let's add here a method that does what [then] used to
do, we can call it [chain] or [after]. This is what I have in mind:

static Future chain(Future future, var action(var value)) {
  Completer completer = new Completer()
  future.then((value) { completer.complete(action(value)); });
  future.handleException((ex) { completer.completeException(ex); });
  return completer.future;
}

http://codereview.chromium.org/8457005/diff/2001/corelib/src/isolate.dart
File corelib/src/isolate.dart (right):

http://codereview.chromium.org/8457005/diff/2001/corelib/src/isolate.dart#new...
corelib/src/isolate.dart:147: Future<SendPort> spawn() {
I'm surprised that so little changes had to be made to keep all tests green.

[mattsh, 2011-11-04 17:10:30]

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
File client/samples/dartcombat/state.dart (right):

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:147: res.then((int result) {
On 2011/11/04 16:57:33, sigmund wrote:
> On 2011/11/04 11:48:13, Ben Laurie (Google) wrote:
> > If then no longer returns a promise you are going to break a lot of code.
> > 
> > Did we really agree that interface?
> 
> I don't think we are finalized on the interface. But, I'd like to see how much
> we can simplify things. 
> 
> I think the behavior of the old [then] is very useful, so we definitively want
> it somewhere. For the time being, how about we provide it as a helper function
> in [Futures]? 
> 
> My thinking is that we should consider adding it back in [Future] once we see
> how much this affects our code readability. Doing a quick grep, I only found 6
> places where we clearly used the returned value of [then] as it used to be,
but
> I'm not convinced either way yet.
Yes, this facility should go on Futures, but that can be a separate CL.

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:147: res.then((int result) {
On 2011/11/04 11:48:13, Ben Laurie (Google) wrote:
> If then no longer returns a promise you are going to break a lot of code.
> 
> Did we really agree that interface?

Done.

http://codereview.chromium.org/8457005/diff/2001/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:152: return true;
On 2011/11/04 16:57:33, sigmund wrote:
> I wonder if we can do something here to avoid writing return true all the
time.
> I'd like essentially an optional return, and consider the exception handled by
> default.

I definitely prefer that you need to take explicit action if your intention is
to swallow an exception.  In general, people should write very few of these, and
when they do, asking them to return 'true' is not much of a burden, and gives
some more assurance the exception isn't being accidentally lost.  So I'd prefer
to keep the interface here as it is.

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

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:116: class Futures {
On 2011/11/04 16:57:33, sigmund wrote:
> + comment on this class

Done.

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:124: Completer completer = new Completer<Object>();
On 2011/11/04 16:57:33, sigmund wrote:
> Compelter<Object> -> Completer<List>?

Done.

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:126: List<Object> values = new List(futures.length);
On 2011/11/04 16:57:33, sigmund wrote:
> List<Object> -> List (let it be dynamic)
I think it should be List<Object> here because we don't care what the values are
and we don't need to call any methods on them, and we want to get a static error
here if we did try to call something.

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:131: int pos = i;
On 2011/11/04 16:57:33, sigmund wrote:
> add here a TODO to remove this once loop-var capture works also in this kind
of
> for-loops (as it does in for-in loops).

Done.

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:132: futures[pos].then((Object value) {
On 2011/11/04 16:57:33, sigmund wrote:
> Object -> var, or remove the type annotation.

See above.

http://codereview.chromium.org/8457005/diff/2001/corelib/src/future.dart#newc...
corelib/src/future.dart:140: }
On 2011/11/04 16:57:33, sigmund wrote:
> to address Ben's concern, let's add here a method that does what [then] used
to
> do, we can call it [chain] or [after]. This is what I have in mind:
> 
> static Future chain(Future future, var action(var value)) {
>   Completer completer = new Completer()
>   future.then((value) { completer.complete(action(value)); });
>   future.handleException((ex) { completer.completeException(ex); });
>   return completer.future;
> }

Can do in follow up CL.

[Siggi Cherem (dart-lang), 2011-11-04 17:35:28]

lgtm

Ben - I assume you are ok with progressively adding what we need (e.g. fixing
then as we see it's use cases)

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

http://codereview.chromium.org/8457005/diff/8003/corelib/src/future.dart#newc...
corelib/src/future.dart:118: * example, waiting for a collectin of Futures to
complete).
collectin -> collection 
Futures -> futures

http://codereview.chromium.org/8457005/diff/8003/corelib/src/future.dart#newc...
corelib/src/future.dart:123: * Returns a future which will complete once all the
futures in a
a future -> a [Future]

[Ben Laurie (Google), 2011-11-04 17:54:46]

Siggi: if by "use cases" you mean stuff that currently uses Promise, then sure.

http://codereview.chromium.org/8457005/diff/8003/client/samples/dartcombat/pl...
File client/samples/dartcombat/player.dart (right):

http://codereview.chromium.org/8457005/diff/8003/client/samples/dartcombat/pl...
client/samples/dartcombat/player.dart:92: res.completeException(message[1]);
is completeException really better than fail?

http://codereview.chromium.org/8457005/diff/8003/client/samples/dartcombat/st...
File client/samples/dartcombat/state.dart (right):

http://codereview.chromium.org/8457005/diff/8003/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:150: res.handleException((String error) {
Should this be Exception error? OK if that's a later improvement... :-)

http://codereview.chromium.org/8457005/diff/8003/client/samples/dartcombat/st...
client/samples/dartcombat/state.dart:152: return true;
What is the return for?

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

http://codereview.chromium.org/8457005/diff/8003/corelib/src/future.dart#newc...
corelib/src/future.dart:118: * example, waiting for a collectin of Futures to
complete).
On 2011/11/04 17:35:28, sigmund wrote:
> collectin -> collection 
> Futures -> futures

or [Future]s

[Ben Laurie (Google), 2011-11-04 19:04:29]

http://codereview.chromium.org/8457005/diff/8003/tests/isolate/src/TestFramew...
File tests/isolate/src/TestFramework.dart (right):

http://codereview.chromium.org/8457005/diff/8003/tests/isolate/src/TestFramew...
tests/isolate/src/TestFramework.dart:182: }
I should point out that this changes behaviour in unchecked mode. Seems OK to
me, tho :-)