Add cleanUp function to Future.wait.

Add cleanUp function to Future.wait.

The cleanUp function is called on the values of successful futures,
so it can release any resources that were successfully allocated.

R=sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2014-12-18 15:38:18]

lrn@google.com changed reviewers:
+ sgjesse@google.com

[kevmoo, 2014-12-18 23:17:50]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2014-12-18 23:17:51]

DBC

Could we have tests for this?

Also: is this strictly needed? The fact that `cleanUp` swallows errors is
bothersome. It's also likely that a cleanup method for an async result may need
to operate async.

This seems like a great fit for the async package, perhaps? Seems it would be
easy to add this functionality on top of the existing wait method.

https://codereview.chromium.org/815773002/diff/1/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/815773002/diff/1/sdk/lib/async/future.dart#ne...
sdk/lib/async/future.dart:259: * If [cleanUp] is provided, in the case of an
error, any non-null result of
This could be a bit more clear.

...in case of an error, all non-null results generated before the error are
passed to `cleanUp`, which can release any...

[Lasse Reichstein Nielsen, 2014-12-19 06:54:11]

Definitely needs tests. I did a quick upload of the base idea just before
shutting down yesterday to get comments on that.

It is quite possible to have this functionality in package:async instead. I
added it here because I kept needing it when writing isolate code, so I think
it's useful enough, and usable often enough, that it should be a core feature.

https://codereview.chromium.org/815773002/diff/1/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/815773002/diff/1/sdk/lib/async/future.dart#ne...
sdk/lib/async/future.dart:259: * If [cleanUp] is provided, in the case of an
error, any non-null result of
Any futures completing successfully after the first error will also be passed to
cleanUp. It could do with a rewrite for clarity.

We also swallow any error after the first - we consider the n futures to be one
operation, and it fails or completes a one.

I'll see if I can make the clean-up errors become uncaught errors instead of
being swallowed. There shouldn't be any errors, so having them uncaught is
reasonable too.

[Søren Gjesse, 2015-01-09 10:04:11]

lgtm

https://codereview.chromium.org/815773002/diff/40001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/815773002/diff/40001/sdk/lib/async/future.dar...
sdk/lib/async/future.dart:253: * The call to `cleanUp` must not throw.
Please explain what happens if cleanUp actually throws.

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
File tests/lib/async/future_test.dart (right):

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
tests/lib/async/future_test.dart:775: void cleanUp(index) {
Check that cleanup[index] is false.

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
tests/lib/async/future_test.dart:778: Future.wait(futures, cleanUp: cleanUp)
Maybe also test with eager error.

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
tests/lib/async/future_test.dart:780: onError: (e, s) {
Wouldnt it read better if you use catchError here?

[Lasse Reichstein Nielsen, 2015-01-09 10:52:10]

https://codereview.chromium.org/815773002/diff/40001/sdk/lib/async/future.dart
File sdk/lib/async/future.dart (right):

https://codereview.chromium.org/815773002/diff/40001/sdk/lib/async/future.dar...
sdk/lib/async/future.dart:253: * The call to `cleanUp` must not throw.
On 2015/01/09 10:04:11, Søren Gjesse wrote:
> Please explain what happens if cleanUp actually throws.

Done.

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
File tests/lib/async/future_test.dart (right):

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
tests/lib/async/future_test.dart:775: void cleanUp(index) {
On 2015/01/09 10:04:11, Søren Gjesse wrote:
> Check that cleanup[index] is false.

Done.

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
tests/lib/async/future_test.dart:778: Future.wait(futures, cleanUp: cleanUp)
On 2015/01/09 10:04:11, Søren Gjesse wrote:
> Maybe also test with eager error.

Done.

https://codereview.chromium.org/815773002/diff/40001/tests/lib/async/future_t...
tests/lib/async/future_test.dart:780: onError: (e, s) {
It would be wrong, even if it might read better.
The Expect.fail test throws if it fails.
A following catchError would then catch that error, which isn't correct.

[Lasse Reichstein Nielsen, 2015-01-09 10:52:45]

Committed patchset #4 (id:60001) manually as 42730 (presubmit successful).