Make errors from StreamController onCancel calls end up in the returned future.

Make errors from StreamController onCancel calls end up in the returned future.

This changes errors happening in an onCancel callback from being uncaught
async errors (captured by zone.runGuarded) to being put into the result
future every time.
It already happens if the onCancel returns a future containing an error,
but if onCancel throws itself, it was different. This makes onCancel
act the same whether it fails immediately or asynchronously.

It should not change the behavior of current working programs:
an uncaught error would likely terminate the program, and the returned
futures are not checked for errors.
After the change, the error will reach the future and become uncaught there
instead, still terminating the program in the same way.

R=ajohnsen@google.com, floitsch@google.com

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

[Lasse Reichstein Nielsen, 2014-05-07 14:31:01]

Please check. 
Will add tests.

[Lasse Reichstein Nielsen, 2014-05-07 14:33:32]

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
sdk/lib/async/stream_controller.dart:501: try {
This entire try-catch could just be `cancelFuture = new Future.sync(_onCancel);`
- if we don't mind always returning a future, even if _onCancel doesn't.

[floitsch, 2014-05-07 15:16:25]

Mostly LGTM.

missing test.

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
sdk/lib/async/stream_controller.dart:512: result = result.whenComplete(() =>
cancelFuture);
I think we should wait for the source-stream's cancel before invoking our
cancel.

That is (something similar to):

Future addingStreamCancelFuture;
if (_isAddingStream) {
  ...
  addingStreamCancelFuture = addState.cancel();
}

_varData = null;
_state = ...;

localCancel() {
  ...
}

if (addingStreamCancelFuture != null) {
  return addingStreamCancelFuture.whenComplete(localCancel);
}
return localCancel();

[Lasse Reichstein Nielsen, 2014-05-07 17:40:26]

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
sdk/lib/async/stream_controller.dart:512: result = result.whenComplete(() =>
cancelFuture);
Any reason for wanting to wait?
We know we are going to do a cleanup, so why not start as soon as possible,
instead of waiting for something else (and probably unrelated) to stop first?
We will still get the errors in the same order, it's just that we allow our
async operations to interleave, which is what async is good at.

[floitsch, 2014-05-07 17:50:50]

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/269283007/diff/1/sdk/lib/async/stream_control...
sdk/lib/async/stream_controller.dart:512: result = result.whenComplete(() =>
cancelFuture);
On 2014/05/07 17:40:26, Lasse Reichstein Nielsen wrote:
> Any reason for wanting to wait?
> We know we are going to do a cleanup, so why not start as soon as possible,
> instead of waiting for something else (and probably unrelated) to stop first?
> We will still get the errors in the same order, it's just that we allow our
> async operations to interleave, which is what async is good at.

Let's discuss this tomorrow.
I don't have a good argument right now, but it feels safer if we wait.

[Lasse Reichstein Nielsen, 2014-05-08 13:52:41]

We'll wait and do the _onCancel after waiting for the addStream cancel to
complete.

[Lasse Reichstein Nielsen, 2014-05-09 14:16:39]

Updated error propagation, PTAL.

Errors during a cancel, including both the cancel call on an addStream stream
and errors from onCancel, now only end up in the cancel future.
The never end up in any of:
- the done future (returned by "close")
- the add-done future (returned by "addStream").

This only handles single-subscription streams.
Broadcast-streams likely also needs work.

[floitsch, 2014-05-09 15:39:21]

LGTM.

https://codereview.chromium.org/269283007/diff/20001/sdk/lib/async/stream_con...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/269283007/diff/20001/sdk/lib/async/stream_con...
sdk/lib/async/stream_controller.dart:499: if (_onCancel != null) {
Give general overview of how we want to handle errors.

https://codereview.chromium.org/269283007/diff/20001/sdk/lib/async/stream_con...
sdk/lib/async/stream_controller.dart:504: result = new
_Future().._asyncCompleteError(e, s);
add comment why this is necessary.

https://codereview.chromium.org/269283007/diff/20001/tests/lib/async/stream_c...
File tests/lib/async/stream_controller_test.dart (right):

https://codereview.chromium.org/269283007/diff/20001/tests/lib/async/stream_c...
tests/lib/async/stream_controller_test.dart:461: done.whenComplete(asyncEnd); 
// Must complete without error.
Then add a 'catchError' first (with a 'fail').
Otherwise the error is passed through and not caught.

You could put the asyncEnd into a 'then' too, but then you would get a timeout
instead of a fail.

https://codereview.chromium.org/269283007/diff/20001/tests/lib/async/stream_c...
tests/lib/async/stream_controller_test.dart:491: addDone.whenComplete(asyncEnd);
 // Should not get error.
ditto.

https://codereview.chromium.org/269283007/diff/20001/tests/lib/async/stream_c...
tests/lib/async/stream_controller_test.dart:525: addDone.whenComplete(asyncEnd);
 // Error must not go here.
ditto.

[Anders Johnsen, 2014-05-12 06:25:58]

LGTM, I'm glad we're getting this cleaned up.

[Lasse Reichstein Nielsen, 2014-05-12 09:48:41]

Committed patchset #3 manually as r36021 (presubmit successful).