Let StreamSubscription.cancel return a Future.

Let StreamSubscription.cancel return a Future.

This gives the ability mark a subscription cancel as asynchrouns. Thiw
now means that StreamController's onCancel takes either a value (null in
    most cases) and a Future, just like 'new Future(() => value)'.


BUG=
R=floitsch@google.com, lrn@google.com

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

[Anders Johnsen, 2013-07-12 13:16:22]

This is limited changed that shows how the Stream methods would be implemented,
and how the base case (StreamController and StreamSubscription) should handle
it.

Missing:
- Tests
- cancelOnError
- Broadcast and other Streams

Please take a look and let me know what you think, at this early stage. (the IO
change is only for example - will be moved to another CL).

[floitsch, 2013-07-12 16:42:34]

FYI.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:350: subscription.cancel().whenComplete(() {
Why `whenComplete` if `cancel` is guaranteed not to throw an error?
`whenComplete` might make the closure nicer to write, but it also costs more
(since it needs to register itself for both callbacks).

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:878: * In case the cancel is asynchronous, the
returned [Future] can be used to
Returns a future that is completed when the cancel is done. The returned future
is guaranteed not to complete with an error.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
sdk/lib/async/stream_controller.dart:480: var future =
_runGuardedFutureResult(_onCancel);
_runGuardedFutureResult may return `null`. Document that this is ok (if it is).

I would still call _runGuarded and do the FutureImpl test here.
(unless _runGuardedFutureResult is supposed to be invoked at other locations).

If _runGuardedFutureResult receives a future, you don't verify that it doesn't
complete with an error. However we guarantee that the future never completes
with an error.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
sdk/lib/async/stream_impl.dart:88: _FutureImpl _cancelFuture;
Add TODO that this should be in some field that isn't used anymore.

[Lasse Reichstein Nielsen, 2013-07-16 12:03:31]

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:350: subscription.cancel().whenComplete(() {
Why would that cost more?

[floitsch, 2013-07-16 12:47:17]

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:350: subscription.cancel().whenComplete(() {
On 2013/07/16 12:03:31, Lasse Reichstein Nielsen wrote:
> Why would that cost more?
My mistake.

[Lasse Reichstein Nielsen, 2013-07-17 07:28:46]

lgtm

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream.dart#...
sdk/lib/async/stream.dart:878: * In case the cancel is asynchronous, the
returned [Future] can be used to
Agree, if it always returns a future, then there is no need to say "in case",
because it's impossible to say if it is the case and you should always act as if
it was async.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
sdk/lib/async/stream_controller.dart:69: * its subscription.
The [onCancel] callback may return a value or [Future]. The
[StreamSubscription.cancel] call returns a [Future] that will be completed with
this value, or with the value of the [Future].
A [Future] returned by [onCancel] should not complete with an error - the error
will most likely be unhandled. Also, any value is likely to be ignored, so just
returning or completing with a null value is sufficient.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
sdk/lib/async/stream_controller.dart:472: _FutureImpl
_recordCancel(StreamSubscription<T> subscription) {
Why _FutureImpl if the user can provide a future through the StreamController?
If this was just Future, then we could forward the user's Future directly, and
only wrap it if it is not a future.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
sdk/lib/async/stream_controller.dart:480: var future =
_runGuardedFutureResult(_onCancel);
I think we should not guarantee that there is no errors when the user is allowed
to provide the error - which they are through the StreamController.

We can recommend that there is no error, and say that if there is, it won't be
handled, but in the end, it's the user that decides how to complete.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
sdk/lib/async/stream_controller.dart:593: } else {
else if (value is Future) {  // You can't assume that all futures are
_FutureImpl.
  Future future = value;
  _FutureImpl result = new _FutureImpl();
  future.then(result._setValue, onError: result._setError);
  return future;
} else {

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_contr...
sdk/lib/async/stream_controller.dart:594: return new Future(() => value);
return new _FutureImpl.immediate(value);

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
sdk/lib/async/stream_impl.dart:88: _FutureImpl _cancelFuture;
And add documentation saying what it is, and where/when it is being useed.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
sdk/lib/async/stream_impl.dart:182: void _chainCancelFuture(_FutureImpl future)
{
_cancelFuture._setOrChainValue(future);

It handles any value (all Futures are chained, non-futures are set as values).

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
sdk/lib/async/stream_impl.dart:196: _chainCancelFuture(_onCancel());
So this line could just be:
 _cancelFuture.setOrChainValue(onCancel());
and you would also handle non-_FutureImpl futures correctly.

https://codereview.chromium.org/18915008/diff/2001/sdk/lib/async/stream_impl....
sdk/lib/async/stream_impl.dart:409: _chainCancelFuture(_onCancel());
_cancelFuture._setOrChainValue(_onCancel());

[Anders Johnsen, 2013-10-11 12:32:39]

PTAL, and ignore old comments as this is pretty much a rewrite due to changes
and new semantics.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:370: // TODO(ajohnsen): Run it before _onDone and
wait for future?
Please comment on this one - I'm not sure what the right thing is to do (but I'm
leaning toward waiting for it).

[floitsch, 2013-10-12 18:53:56]

Mostly LGTM, but I think we will have to discuss this again in person...

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:350: _cancelAndError(subscription,
result)(_asyncError(e, s));
It seems wrong to create a closure and then invoke it immediately...

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:874: * wait for the operation to complete. If in
doubt, wait for it.
Note that it can be `null`.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1272: * is properly closed.
Add comment (even if it is just pointing to the StreamSubscription).
Preferably without link.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:77: onCancel(),
Needs comment.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:480: var future = _runGuarded(_onCancel);
Use type.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:578: _runGuarded(_NotificationHandler
notificationHandler) {
"Future" as return type ?

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:74: static const int _STATE_IN_ERROR_CANCEL = 16;
Add comment that _STATE_IN_ERROR_CANCEL implies that the _STATE_CANCEL bit is
set, too.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:89: _Future _cancelFuture;
Add "TODO(floitsch): reuse another field"

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:186: _state &= ~_STATE_IN_ERROR_CANCEL;
Add comment.
// The user doesn't want to receive any events anymore. If there is an
// error event pending (waiting for the cancel to be done) discard that
// error.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:227: _cancelFuture = _onCancel();
I'm not sure we are allowed to call "_onCancel" while there is a callback
executed.
The original code guarded against this. Please talk to Lasse.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:337: void sendError() {
one line before and one line after the function.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:338: if (!_isCanceled || _inErrorCancel) {
Comment: If the subscription has been canceled while waiting for the cancel
future to finish we must not report the error.
Rewrite it for an early exit:
if (isCanceled && !_inErrorCancel) return;

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:370: // TODO(ajohnsen): Run it before _onDone and
wait for future?
On 2013/10/11 12:32:40, Anders Johnsen wrote:
> Please comment on this one - I'm not sure what the right thing is to do (but
I'm
> leaning toward waiting for it).

Neither.
The done event should *not* call cancel. Done implies that nothing comes
anymore.
The "_onCancel" is a misnomer. It should be "_onFinish" (or something similar).

It's the StreamController that should call its onCancel (which again is a
misnomer there). We should discuss this again in person.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
File sdk/lib/async/stream_pipe.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:29: (error) {
You will need to update this for the stack-trace argument.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:31: if (cancelFuture != null) {
if (cancelFuture is Future)

Users are used to write: onCancel: () => foo()
If this thing returns something else than a future it should not be an error.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:42: if (cancelFuture != null) {
ditto.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
File tests/lib/async/stream_state_test.dart (right):

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:79: ..expectBroadcastListenOpt()
You should not need to use "Opt".
This is in the "asBroadcast" branch, so this listen should happen
unconditionally. Just remove the "Opt".

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:82: ..expectBroadcastCancelOpt()
ditto.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:86: ..expectBroadcastListenOpt()
Remove, since we are in the non-broadcast branch.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:89: ..expectBroadcastCancelOpt()
ditto.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_su...
File tests/lib/async/stream_subscription_cancel_test.dart (right):

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_su...
tests/lib/async/stream_subscription_cancel_test.dart:79: });
Add test that a cancel after a cancelOnError will discard the error.

[Lasse Reichstein Nielsen, 2013-10-14 11:32:33]

LGTM with more tests. In my experience, only exhaustive tests can convince me
that something this complex actually works.

I think it works, but can be convinced otherwise. :)

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:350: _cancelAndError(subscription,
result)(_asyncError(e, s));
Agree.
If it is because _cancelAndError(subscription, result) is used somewhere else as
an error-callback, I'd rather have three arguments and use
   (x) => _cancelAndError(subscription, result, x) 
at that point. Which is already happening (check the (T value) => ...) change
below).

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:77: onCancel(),
Can the return type be Future? 
It's always ok to return null, but it may not be type-correct to use a void
function :(

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:580: try {
Is this not just
 Zone.current.runGuarded(notificationHandler)
?

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:74: static const int _STATE_IN_ERROR_CANCEL = 16;
And document what it means. In painful detail, because this is so hard to
understand anyway.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:186: _state &= ~_STATE_IN_ERROR_CANCEL;
any events anymore -> any further events.

This could possibly be generalized to handle the pending done event as well. If
the user cancels before an event, which has been delayed because it waits for
cancelling to complete, that event is no longer needed.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:227: _cancelFuture = _onCancel();
I accepted that we had to call _onCancel during another callback.
The alternative was to return a new _Future and only later, during the _onCancel
callback, link the cancel future to the new future. But that would mean always
returning a future, even if _onCancel returned null.

The callback is likely be in an event handler, not in, e.g., an "onPause"
callback.

I'm not absolutely convinced that it's sound, but it looks like it.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
File sdk/lib/async/stream_pipe.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:29: (error) {
And not make it curried.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
File tests/lib/async/stream_state_test.dart (right):

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:97: });
I want to see tests where the close and the cancel are interleaved in every
possible way.

I want to see a cancel call inside all of a data event, an error event
(with/without cancelOnError), a done event.
And a close inside all of onListen, onPause, onResume (and, heck, onCancel).
Consider whether it makes sense to do a sub.cancel inside onListen, or a close
inside onData.

[Anders Johnsen, 2013-10-16 11:52:20]

PTAL

Added some more tests, but I'm not 100% sure it's the right approach in
stream_state_test. Lasse, please comment :)

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:350: _cancelAndError(subscription,
result)(_asyncError(e, s));
On 2013/10/12 18:53:57, floitsch wrote:
> It seems wrong to create a closure and then invoke it immediately...

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:350: _cancelAndError(subscription,
result)(_asyncError(e, s));
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> Agree.
> If it is because _cancelAndError(subscription, result) is used somewhere else
as
> an error-callback, I'd rather have three arguments and use
>    (x) => _cancelAndError(subscription, result, x) 
> at that point. Which is already happening (check the (T value) => ...) change
> below).

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:874: * wait for the operation to complete. If in
doubt, wait for it.
On 2013/10/12 18:53:57, floitsch wrote:
> Note that it can be `null`.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1272: * is properly closed.
On 2013/10/12 18:53:57, floitsch wrote:
> Add comment (even if it is just pointing to the StreamSubscription).
> Preferably without link.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:77: onCancel(),
On 2013/10/12 18:53:57, floitsch wrote:
> Needs comment.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:77: onCancel(),
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> Can the return type be Future? 
> It's always ok to return null, but it may not be type-correct to use a void
> function :(

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:480: var future = _runGuarded(_onCancel);
On 2013/10/12 18:53:57, floitsch wrote:
> Use type.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:578: _runGuarded(_NotificationHandler
notificationHandler) {
On 2013/10/12 18:53:57, floitsch wrote:
> "Future" as return type ?

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:580: try {
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> Is this not just
>  Zone.current.runGuarded(notificationHandler)
> ?

Not sure, Florian?

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:74: static const int _STATE_IN_ERROR_CANCEL = 16;
On 2013/10/12 18:53:57, floitsch wrote:
> Add comment that _STATE_IN_ERROR_CANCEL implies that the _STATE_CANCEL bit is
> set, too.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:89: _Future _cancelFuture;
On 2013/10/12 18:53:57, floitsch wrote:
> Add "TODO(floitsch): reuse another field"

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:186: _state &= ~_STATE_IN_ERROR_CANCEL;
On 2013/10/12 18:53:57, floitsch wrote:
> Add comment.
> // The user doesn't want to receive any events anymore. If there is an
> // error event pending (waiting for the cancel to be done) discard that
> // error.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:186: _state &= ~_STATE_IN_ERROR_CANCEL;
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> any events anymore -> any further events.
> 
> This could possibly be generalized to handle the pending done event as well.
If
> the user cancels before an event, which has been delayed because it waits for
> cancelling to complete, that event is no longer needed.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:227: _cancelFuture = _onCancel();
On 2013/10/12 18:53:57, floitsch wrote:
> I'm not sure we are allowed to call "_onCancel" while there is a callback
> executed.
> The original code guarded against this. Please talk to Lasse.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:227: _cancelFuture = _onCancel();
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> I accepted that we had to call _onCancel during another callback.
> The alternative was to return a new _Future and only later, during the
_onCancel
> callback, link the cancel future to the new future. But that would mean always
> returning a future, even if _onCancel returned null.
> 
> The callback is likely be in an event handler, not in, e.g., an "onPause"
> callback.
> 
> I'm not absolutely convinced that it's sound, but it looks like it.
> 

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:337: void sendError() {
On 2013/10/12 18:53:57, floitsch wrote:
> one line before and one line after the function.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:338: if (!_isCanceled || _inErrorCancel) {
On 2013/10/12 18:53:57, floitsch wrote:
> Comment: If the subscription has been canceled while waiting for the cancel
> future to finish we must not report the error.
> Rewrite it for an early exit:
> if (isCanceled && !_inErrorCancel) return;

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:370: // TODO(ajohnsen): Run it before _onDone and
wait for future?
On 2013/10/11 12:32:40, Anders Johnsen wrote:
> Please comment on this one - I'm not sure what the right thing is to do (but
I'm
> leaning toward waiting for it).

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:370: // TODO(ajohnsen): Run it before _onDone and
wait for future?
On 2013/10/12 18:53:57, floitsch wrote:
> On 2013/10/11 12:32:40, Anders Johnsen wrote:
> > Please comment on this one - I'm not sure what the right thing is to do (but
> I'm
> > leaning toward waiting for it).
> 
> Neither.
> The done event should *not* call cancel. Done implies that nothing comes
> anymore.
> The "_onCancel" is a misnomer. It should be "_onFinish" (or something
similar).
> 
> It's the StreamController that should call its onCancel (which again is a
> misnomer there). We should discuss this again in person.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
File sdk/lib/async/stream_pipe.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:29: (error) {
On 2013/10/12 18:53:57, floitsch wrote:
> You will need to update this for the stack-trace argument.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:29: (error) {
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> And not make it curried.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:31: if (cancelFuture != null) {
On 2013/10/12 18:53:57, floitsch wrote:
> if (cancelFuture is Future)
> 
> Users are used to write: onCancel: () => foo()
> If this thing returns something else than a future it should not be an error.

Done.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_pipe...
sdk/lib/async/stream_pipe.dart:42: if (cancelFuture != null) {
On 2013/10/12 18:53:57, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
File tests/lib/async/stream_state_test.dart (right):

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:79: ..expectBroadcastListenOpt()
On 2013/10/12 18:53:57, floitsch wrote:
> You should not need to use "Opt".
> This is in the "asBroadcast" branch, so this listen should happen
> unconditionally. Just remove the "Opt".

Done.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:82: ..expectBroadcastCancelOpt()
On 2013/10/12 18:53:57, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:86: ..expectBroadcastListenOpt()
On 2013/10/12 18:53:57, floitsch wrote:
> Remove, since we are in the non-broadcast branch.

Done.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:89: ..expectBroadcastCancelOpt()
On 2013/10/12 18:53:57, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_st...
tests/lib/async/stream_state_test.dart:97: });
On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> I want to see tests where the close and the cancel are interleaved in every
> possible way.
> 
> I want to see a cancel call inside all of a data event, an error event
> (with/without cancelOnError), a done event.
> And a close inside all of onListen, onPause, onResume (and, heck, onCancel).
> Consider whether it makes sense to do a sub.cancel inside onListen, or a close
> inside onData.
>  

Done.

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_su...
File tests/lib/async/stream_subscription_cancel_test.dart (right):

https://codereview.chromium.org/18915008/diff/15001/tests/lib/async/stream_su...
tests/lib/async/stream_subscription_cancel_test.dart:79: });
On 2013/10/12 18:53:57, floitsch wrote:
> Add test that a cancel after a cancelOnError will discard the error.

Done.

[floitsch, 2013-10-16 14:43:44]

LGTM.
if possible wait for Lasse's comments on the tests.

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/15001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:580: try {
On 2013/10/16 11:52:21, Anders Johnsen wrote:
> On 2013/10/14 11:32:33, Lasse Reichstein Nielsen wrote:
> > Is this not just
> >  Zone.current.runGuarded(notificationHandler)
> > ?
> 
> Not sure, Florian?

Zone.current.runGuarded(notificationHandler) sounds right.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:932: * wait for the operation to complete. If in
doubt, wait for it. Note that
Start sentence with "Returns".
===
Returns a future if the cancel-operation is not completed synchronously.
Otherwise returns `null`.
===

I don't think it buys to add "if in doubt...".

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1219: *
ditto.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_cont...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:70: * [onCancel] can return a [Future]. The
[StreamSubscription] will then get
should return a future that completes when the cancel operation is done.

Remove the remainder of the paragraph.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:476: Future future =
_runGuarded(_onCancel);
If the doneFuture and the cancel are not linked (shouldn't they?)
move the _runGuarded below the completion of the _doneFuture.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:78: *   * An error is sent, and [cancelOnError]
is true.
Nit (you also need the new line as otherwise dartdoc will not itemize).

Set when either:

  * an error is sent, and [cancelOnError] is true, or
  * a done event is sent.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:82: * state is unset, and no furher events should
be delivered.
"should" is not helpful.
I guess "is" is more accurate (or "must").

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:215: bool get _waitForCancel => (_state &
_STATE_WAIT_FOR_CANCEL) != 0;
_waitsForCancel

[Anders Johnsen, 2013-10-21 08:01:45]

Thanks for the comments on the comments!

PTAL, Lasse, do you have further comments for tests?

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream.dart
File sdk/lib/async/stream.dart (right):

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:932: * wait for the operation to complete. If in
doubt, wait for it. Note that
On 2013/10/16 14:43:44, floitsch wrote:
> Start sentence with "Returns".
> ===
> Returns a future if the cancel-operation is not completed synchronously.
> Otherwise returns `null`.
> ===
> 
> I don't think it buys to add "if in doubt...".

Done.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream.dart...
sdk/lib/async/stream.dart:1219: *
On 2013/10/16 14:43:44, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_cont...
File sdk/lib/async/stream_controller.dart (right):

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:70: * [onCancel] can return a [Future]. The
[StreamSubscription] will then get
On 2013/10/16 14:43:44, floitsch wrote:
> should return a future that completes when the cancel operation is done.
> 
> Remove the remainder of the paragraph.

Done.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_cont...
sdk/lib/async/stream_controller.dart:476: Future future =
_runGuarded(_onCancel);
On 2013/10/16 14:43:44, floitsch wrote:
> If the doneFuture and the cancel are not linked (shouldn't they?)
> move the _runGuarded below the completion of the _doneFuture.

Done.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:78: *   * An error is sent, and [cancelOnError]
is true.
On 2013/10/16 14:43:44, floitsch wrote:
> Nit (you also need the new line as otherwise dartdoc will not itemize).
> 
> Set when either:
> 
>   * an error is sent, and [cancelOnError] is true, or
>   * a done event is sent.

Done.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:82: * state is unset, and no furher events should
be delivered.
On 2013/10/16 14:43:44, floitsch wrote:
> "should" is not helpful.
> I guess "is" is more accurate (or "must").

Using 'must', as the comment is dictating how the implementation should act.

https://codereview.chromium.org/18915008/diff/29001/sdk/lib/async/stream_impl...
sdk/lib/async/stream_impl.dart:215: bool get _waitForCancel => (_state &
_STATE_WAIT_FOR_CANCEL) != 0;
On 2013/10/16 14:43:44, floitsch wrote:
> _waitsForCancel

Done.

[Anders Johnsen, 2013-10-21 12:09:51]

Committed patchset #8 manually as r28925 (presubmit successful).