Return futures on Stream.cancel when possible.

Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56
Reverted: https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
Committed: https://github.com/dart-lang/sdk/commit/1905ddafaa242883891e2fb2d706e2dfa059a71b
Reverted: https://github.com/dart-lang/sdk/commit/46a8579c1eb58f6a36a7d094d9b7824c92873de4
Committed: https://github.com/dart-lang/sdk/commit/6255638cd0623b3aa41596b98f4584876f6b8822

[floitsch, 2016-08-01 15:07:59]

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

[Lasse Reichstein Nielsen, 2016-08-01 15:23:27]

lgtm

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

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream.da...
sdk/lib/async/stream.dart:235: return new Future.value(null);
We have `Future._nullFuture` as a reusable null-valued future. Avoids creating a
new one every time.

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

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:714: result = onCancel() ?? new
Future.value(null);
Use `Future._nullFuture`.

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_im...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:781: Future cancel() => new Future.value(null);
Future._nullFuture

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:919: return new Future.value(null);
Future._nullFuture
(I will stop saying it now, but do change the rest too :)

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
sdk/lib/async/stream_transformers.dart:37: Future _cancelFuture;
Why a field? I only see it used inside one method?

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
sdk/lib/async/stream_transformers.dart:115: _cancelFuture =
subscription.cancel();
That is, why is _cancelFuture not a local variable here.

[floitsch, 2016-08-01 15:55:56]

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
sdk/lib/async/stream_transformers.dart:115: _cancelFuture =
subscription.cancel();
On 2016/08/01 15:23:26, Lasse Reichstein Nielsen wrote:
> That is, why is _cancelFuture not a local variable here.

In case someone calls `cancel` multiple times.

[Lasse Reichstein Nielsen, 2016-08-01 16:49:38]

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
sdk/lib/async/stream_transformers.dart:115: _cancelFuture =
subscription.cancel();
Possibly a good point.
I'm not sure _onCancel will be called more than once, though.
In the default _BufferingStreamSubscription implementation (stream_impl.dart,
line 191) we check whether we are already canceled, and only call _cancel (which
calls _onCancel) if we arent'. We also call _cancel in _sendDone and _sendError
(if cancelOnError), which can also only happen once. The implementation buffers
the cancelFuture there, so you don't need to do it here.

[floitsch, 2016-08-01 21:00:38]

PTAL.

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

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream.da...
sdk/lib/async/stream.dart:235: return new Future.value(null);
On 2016/08/01 15:23:26, Lasse Reichstein Nielsen wrote:
> We have `Future._nullFuture` as a reusable null-valued future. Avoids creating
a
> new one every time.

Done.

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

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_co...
sdk/lib/async/stream_controller.dart:714: result = onCancel() ?? new
Future.value(null);
On 2016/08/01 15:23:26, Lasse Reichstein Nielsen wrote:
> Use `Future._nullFuture`.

Moved the 'Future._nullFuture' to the `cancel` function, but done.

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_im...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:781: Future cancel() => new Future.value(null);
On 2016/08/01 15:23:26, Lasse Reichstein Nielsen wrote:
> Future._nullFuture

Done.

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:919: return new Future.value(null);
On 2016/08/01 15:23:26, Lasse Reichstein Nielsen wrote:
> Future._nullFuture
> (I will stop saying it now, but do change the rest too :)

Done.

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
File sdk/lib/async/stream_transformers.dart (right):

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
sdk/lib/async/stream_transformers.dart:37: Future _cancelFuture;
On 2016/08/01 15:23:26, Lasse Reichstein Nielsen wrote:
> Why a field? I only see it used inside one method?

Acknowledged.

https://codereview.chromium.org/2202533003/diff/20001/sdk/lib/async/stream_tr...
sdk/lib/async/stream_transformers.dart:115: _cancelFuture =
subscription.cancel();
On 2016/08/01 16:49:38, Lasse Reichstein Nielsen wrote:
> Possibly a good point.
> I'm not sure _onCancel will be called more than once, though.
> In the default _BufferingStreamSubscription implementation (stream_impl.dart,
> line 191) we check whether we are already canceled, and only call _cancel
(which
> calls _onCancel) if we arent'. We also call _cancel in _sendDone and
_sendError
> (if cancelOnError), which can also only happen once. The implementation
buffers
> the cancelFuture there, so you don't need to do it here.
> 

You are right.
Changed a bit more...

https://codereview.chromium.org/2202533003/diff/40001/sdk/lib/async/stream_im...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/2202533003/diff/40001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:194: return _cancelFuture ?? Future._nullFuture;
This is probably the biggest change: almost all StreamSubscriptions will now
return a Future.

[Lasse Reichstein Nielsen, 2016-08-02 01:25:39]

lgtm

https://codereview.chromium.org/2202533003/diff/60001/sdk/lib/async/stream_im...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/2202533003/diff/60001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:204: if (cancelFuture is Future) {
cancel() can't return null any more, so there is no need to check for `is
Future`. (If it's a non-null non-Future object, it's a type error, and it's fine
to just assume it's a Future).

Should the check be `!identical(cancelFuture, Future._nullFuture)`?

https://codereview.chromium.org/2202533003/diff/60001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:205: cancelFuture.then((_) {
If the cancelFuture has an error, then the result future isn't completed. Maybe
use "whenComplete" instead of "then"?
The cancelFuture error will still be unhandled, but it's probably a good idea to
wait for it before completing the asFuture future.

[floitsch, 2016-08-02 12:03:09]

Also updated the Changelog. (Maybe have a short look?).
thanks.

https://codereview.chromium.org/2202533003/diff/60001/sdk/lib/async/stream_im...
File sdk/lib/async/stream_impl.dart (right):

https://codereview.chromium.org/2202533003/diff/60001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:204: if (cancelFuture is Future) {
On 2016/08/02 01:25:39, Lasse Reichstein Nielsen wrote:
> cancel() can't return null any more, so there is no need to check for `is
> Future`. (If it's a non-null non-Future object, it's a type error, and it's
fine
> to just assume it's a Future).
> 
> Should the check be `!identical(cancelFuture, Future._nullFuture)`?

done.

https://codereview.chromium.org/2202533003/diff/60001/sdk/lib/async/stream_im...
sdk/lib/async/stream_impl.dart:205: cancelFuture.then((_) {
On 2016/08/02 01:25:39, Lasse Reichstein Nielsen wrote:
> If the cancelFuture has an error, then the result future isn't completed.
Maybe
> use "whenComplete" instead of "then"?
> The cancelFuture error will still be unhandled, but it's probably a good idea
to
> wait for it before completing the asFuture future.

done.

[floitsch, 2016-08-05 11:58:51]

Description was changed from

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
==========

to

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56
==========

[floitsch, 2016-08-05 11:58:52]

Committed patchset #6 (id:100001) manually as
395e7aaa696126b82862a9f75e7ebf742c354b56 (presubmit successful).

[floitsch, 2016-08-08 09:15:16]

Description was changed from

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56
==========

to

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56

Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
==========

[floitsch, 2016-08-08 15:30:40]

Description was changed from

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56

Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
==========

to

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56

Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
Committed:
https://github.com/dart-lang/sdk/commit/1905ddafaa242883891e2fb2d706e2dfa059a71b
==========

[floitsch, 2016-08-08 15:30:42]

Committed patchset #9 (id:160001) manually as
1905ddafaa242883891e2fb2d706e2dfa059a71b (presubmit successful).

[floitsch, 2016-08-08 15:31:18]

Committed again. (TBR for the smaller changes).

[floitsch, 2016-09-05 14:54:33]

Description was changed from

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56

Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
Committed:
https://github.com/dart-lang/sdk/commit/1905ddafaa242883891e2fb2d706e2dfa059a71b
==========

to

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56
Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
Committed:
https://github.com/dart-lang/sdk/commit/1905ddafaa242883891e2fb2d706e2dfa059a71b
Reverted:
https://github.com/dart-lang/sdk/commit/46a8579c1eb58f6a36a7d094d9b7824c92873de4
==========

[floitsch, 2016-09-05 14:56:13]

Updated Pipe.cancel as well.
Trying again...

[floitsch, 2016-09-05 15:41:27]

Description was changed from

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56
Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
Committed:
https://github.com/dart-lang/sdk/commit/1905ddafaa242883891e2fb2d706e2dfa059a71b
Reverted:
https://github.com/dart-lang/sdk/commit/46a8579c1eb58f6a36a7d094d9b7824c92873de4
==========

to

==========
Return futures on Stream.cancel when possible.

Deprecate returning `null`.

Also, fixes cases where transformations on a stream didn't forward the cancel
future

Fixes #26777.

BUG= http://dartbug.com/26777.
R=lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/395e7aaa696126b82862a9f75e7ebf742c354b56
Reverted:
https://github.com/dart-lang/sdk/commit/99e5328eaca7f98641a5c6e523e80b7297792817
Committed:
https://github.com/dart-lang/sdk/commit/1905ddafaa242883891e2fb2d706e2dfa059a71b
Reverted:
https://github.com/dart-lang/sdk/commit/46a8579c1eb58f6a36a7d094d9b7824c92873de4
Committed:
https://github.com/dart-lang/sdk/commit/6255638cd0623b3aa41596b98f4584876f6b8822
==========

[floitsch, 2016-09-05 15:41:29]

Committed patchset #10 (id:180001) manually as
6255638cd0623b3aa41596b98f4584876f6b8822 (presubmit successful).