Make exceptions propagate through multiple Future branches.

Make exceptions propagate through multiple Future branches.

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

[nweiz, 2012-11-21 00:46:43]

By way of background, this fixes an issue I ran into while writing tests for
Pub. Specifically, when one Future has an exception, only one of its "child"
Futures (those created via transform() or chain()) would ever see that
exception. In addition, the exception would occasionally be thrown to the top
level even when there were no calls to then() and it could otherwise be handled.

This is blocking "pub upload", and I'd really like to get it checked in before
the Thanksgiving holiday on Thursday, so a quick review would be very much
appreciated.

[Siggi Cherem (dart-lang), 2012-11-21 01:08:57]

thanks Nathan - overall looks good, just a couple comments

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dart
File tests/corelib/future_test.dart (right):

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dar...
tests/corelib/future_test.dart:610: Expect.listEquals([2, 1], results);
it's weird to see these in opposite order, maybe use Expect.setEquals so that we
test only the relevant part?

(same in all other places)

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dar...
tests/corelib/future_test.dart:639: var branch = completer.future.transform((_)
=> null);
maybe add another test to check what happens if you do the transform after
adding 'handleException'? That is,

var base = completer.future;
base.handleException((e) { results.add('base'); return true; });
var branch = base.transform((_) => null);
...

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dar...
tests/corelib/future_test.dart:695: testExceptionIsHandledInBaseAndBranch();
add the other 2 tests? (...AfterComplete)

[floitsch, 2012-11-21 10:16:41]

LGTM.
We will change the Future interface, but propagating errors to all subscribers
is part of the change we want to do.

[nweiz, 2012-11-21 20:39:09]

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dart
File tests/corelib/future_test.dart (right):

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dar...
tests/corelib/future_test.dart:610: Expect.listEquals([2, 1], results);
On 2012/11/21 01:08:57, Siggi Cherem (dart-lang) wrote:
> it's weird to see these in opposite order, maybe use Expect.setEquals so that
we
> test only the relevant part?
> 
> (same in all other places)

Done.

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dar...
tests/corelib/future_test.dart:639: var branch = completer.future.transform((_)
=> null);
On 2012/11/21 01:08:57, Siggi Cherem (dart-lang) wrote:
> maybe add another test to check what happens if you do the transform after
> adding 'handleException'? That is,
> 
> var base = completer.future;
> base.handleException((e) { results.add('base'); return true; });
> var branch = base.transform((_) => null);
> ...

That case isn't handled by this CL; it will actually stop the exception from
propagating. The correct behavior in that situation is a little less clear-cut.

https://codereview.chromium.org/11418104/diff/1/tests/corelib/future_test.dar...
tests/corelib/future_test.dart:695: testExceptionIsHandledInBaseAndBranch();
On 2012/11/21 01:08:57, Siggi Cherem (dart-lang) wrote:
> add the other 2 tests? (...AfterComplete)

Done.