Add ClosedStreamSink, and *Completer.setError, and StreamSinkCompleter.fromFuture.

Add ClosedStreamSink, and *Completer.setError, and StreamSinkCompleter.fromFuture.

R=lrn@google.com

Committed: https://github.com/dart-lang/async/commit/32ffb0394a53f1a29a0c63ff20d53fa4b54257bd

[Lasse Reichstein Nielsen, 2016-01-22 09:13:47]

LGTM except for the name ClosedStreamSink.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
File lib/src/closed_stream_sink.dart (right):

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:12: /// throws [StateError]s when events are
added.
It confuses me that it's "in a closed state" before "close" is called.

It's basically a StreamSink that *ignores* data and error events.
Maybe name it:
  VoidStreamSink
  NullStreamSink (but that's using "null" as in /dev/null, not Null the type)
  DiscardingStreamSink
  IgnoreEventStreamSink
?

Or make it actually act as if close had already been called, in which case the
name fits.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:15: /// to be added. The [new
ClosedStreamSink.error] constructor can be used to
If it's not intended to receive events, why not throw if it does (that is,
actually start in a closed state like after calling close)?

You can call close multiple times.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:16: /// represent errors in creating a sink.
How does errors in a done future represent errors in *creating* a sink?

Is the created sink intended to replace the sink that couldn't be created due to
an error, and the done future is the only place to return the error? (IOW:
sentence is not clear, an example may improve it).

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:22: /// To be consistent with other stream sinks
that have been closed remotely,
I would use the name "canceled" for what the receiving end does (like a stream
subscription's cancel and the StreamController's onCancel).

Still, this is describing the class in terms of what something else does instead
of what it does itself (ignore events).

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:35: : done = new Future.error(error,
stackTrace);
This will cause the error to be uncaught in the next microtask, which may be too
soon for the user of the sink to call close().
Consider ignoring the error:
  new Future.error(..,..)..catchError((_){});
or storing it separately until the user actually asks for a done future.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:39: throw new StateError("Cannot add to a closed
sink.");
How about just:
  if (_closed) throw ...;
The current code seems like a round-about way of doing that (as in: I had to
read it twice to figure out that that was all it did).

Consider a helper function to create the state error (to avoid duplicating the
message):

  if (_closed) throw _closedError();
...
_closedError => new StateError("...");

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:51: 
You need a bit here to remember that you are adding a stream.

You should get a state error if you try to add again (or close) until the
addStream has completed.

Maybe change _closed to a state which is one of OPEN, ADDING or CLOSED, so
add(..) becomes: if (_state == OPEN) return; if (_state == ADDING) throw
_addWhileAddingError(); assert(_state == CLOSED); throw _closedError();

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:52: return stream.listen(null).cancel() ?? new
Future.value();
In dart:async we have a Future<dynamic> completed with null that we reuse
occasionally.
Do you think we should make that public so you could use it here instead of
creating a new future every time?

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.dart
File lib/src/stream_completer.dart (right):

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:77: /// Any of [setSourceStream], [setEmpty], and
[setError] may be called at most
Any one of ...

(The current writing is unclear on whether you can call setEmpty and setError
once each).

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:88: /// Any of [setSourceStream], [setEmpty], and
[setError] may be called at most
Any one of ...

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:99: /// This is useful when the process of loading
the stream fails.
loading the stream -> creating the data for the stream

(loading is too specific, and not something you do to a stream).

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:101: /// Any of [setSourceStream], [setEmpty], and
[setError] may be called at most
Any one ..

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:103: void setError(error, [StackTrace stackTrace])
{
Good idea!

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_sink_complet...
File lib/src/stream_sink_completer.dart (right):

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_sink_complet...
lib/src/stream_sink_completer.dart:34: /// Convert a `Future<StreamSink>` to a
`StreamSink`.
Excellent! That was the original reason to have the class, so why not help you
do the work.

https://codereview.chromium.org/1615253002/diff/1/test/closed_stream_sink_tes...
File test/closed_stream_sink_test.dart (right):

https://codereview.chromium.org/1615253002/diff/1/test/closed_stream_sink_tes...
test/closed_stream_sink_test.dart:91: });
Test that calling add, addError, addStream and close fails until the future
returned by addStream completes (and make it so :).

[nweiz, 2016-01-25 22:38:45]

Code review changes

[nweiz, 2016-01-25 22:39:25]

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
File lib/src/closed_stream_sink.dart (right):

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:12: /// throws [StateError]s when events are
added.
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> It confuses me that it's "in a closed state" before "close" is called.
> 
> It's basically a StreamSink that *ignores* data and error events.
> Maybe name it:
>   VoidStreamSink
>   NullStreamSink (but that's using "null" as in /dev/null, not Null the type)
>   DiscardingStreamSink
>   IgnoreEventStreamSink
> ?
> 
> Or make it actually act as if close had already been called, in which case the
> name fits.

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:15: /// to be added. The [new
ClosedStreamSink.error] constructor can be used to
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> If it's not intended to receive events, why not throw if it does (that is,
> actually start in a closed state like after calling close)?
> 
> You can call close multiple times.

It's meant to be used to mimic the remote stream being closed—see for example
the use in StreamSinkCompleter, where making it throw errors would cause a bunch
of pending adds to fail. In general, the may not know not to add to the sink.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:16: /// represent errors in creating a sink.
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> How does errors in a done future represent errors in *creating* a sink?
> 
> Is the created sink intended to replace the sink that couldn't be created due
to
> an error, and the done future is the only place to return the error? (IOW:
> sentence is not clear, an example may improve it).

From the StreamSink documentation: "If the StreamSink fails with an error [...]
the done future will complete with that error." ClosedStreamSink.error
represents a failed sink—it's just that the sink failed before it was even
constructed.

I'll see if I can clarify that, though.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:22: /// To be consistent with other stream sinks
that have been closed remotely,
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> I would use the name "canceled" for what the receiving end does (like a stream
> subscription's cancel and the StreamController's onCancel).
> 
> Still, this is describing the class in terms of what something else does
instead
> of what it does itself (ignore events).

I think changing the name and updating the class description made this clear, so
I'm just removing the paragraph.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:35: : done = new Future.error(error,
stackTrace);
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> This will cause the error to be uncaught in the next microtask, which may be
too
> soon for the user of the sink to call close().
> Consider ignoring the error:
>   new Future.error(..,..)..catchError((_){});
> or storing it separately until the user actually asks for a done future.

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:39: throw new StateError("Cannot add to a closed
sink.");
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> How about just:
>   if (_closed) throw ...;
> The current code seems like a round-about way of doing that (as in: I had to
> read it twice to figure out that that was all it did).
> 
> Consider a helper function to create the state error (to avoid duplicating the
> message):
> 
>   if (_closed) throw _closedError();
> ...
> _closedError => new StateError("...");

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:51: 
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> You need a bit here to remember that you are adding a stream.
> 
> You should get a state error if you try to add again (or close) until the
> addStream has completed.
> 
> Maybe change _closed to a state which is one of OPEN, ADDING or CLOSED, so
> add(..) becomes: if (_state == OPEN) return; if (_state == ADDING) throw
> _addWhileAddingError(); assert(_state == CLOSED); throw _closedError();
> 

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/closed_stream_sink....
lib/src/closed_stream_sink.dart:52: return stream.listen(null).cancel() ?? new
Future.value();
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> In dart:async we have a Future<dynamic> completed with null that we reuse
> occasionally.
> Do you think we should make that public so you could use it here instead of
> creating a new future every time?

I don't think it's worth the complexity.

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.dart
File lib/src/stream_completer.dart (right):

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:77: /// Any of [setSourceStream], [setEmpty], and
[setError] may be called at most
On 2016/01/22 09:13:47, Lasse Reichstein Nielsen wrote:
> Any one of ...
> 
> (The current writing is unclear on whether you can call setEmpty and setError
> once each).

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:88: /// Any of [setSourceStream], [setEmpty], and
[setError] may be called at most
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> Any one of ...

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:99: /// This is useful when the process of loading
the stream fails.
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> loading the stream -> creating the data for the stream
> 
> (loading is too specific, and not something you do to a stream).

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:101: /// Any of [setSourceStream], [setEmpty], and
[setError] may be called at most
On 2016/01/22 09:13:46, Lasse Reichstein Nielsen wrote:
> Any one ..

Done.

https://codereview.chromium.org/1615253002/diff/1/lib/src/stream_completer.da...
lib/src/stream_completer.dart:103: void setError(error, [StackTrace stackTrace])
{
On 2016/01/22 09:13:47, Lasse Reichstein Nielsen wrote:
> Good idea!

:D

https://codereview.chromium.org/1615253002/diff/1/test/closed_stream_sink_tes...
File test/closed_stream_sink_test.dart (right):

https://codereview.chromium.org/1615253002/diff/1/test/closed_stream_sink_tes...
test/closed_stream_sink_test.dart:91: });
On 2016/01/22 09:13:47, Lasse Reichstein Nielsen wrote:
> Test that calling add, addError, addStream and close fails until the future
> returned by addStream completes (and make it so :).

Done.

[nweiz, 2016-01-25 22:39:39]

Description was changed from

==========
Add ClosedStreamSink, and *Completer.setError, and
StreamSinkCompleter.fromFuture.

R=lrn@google.com
==========

to

==========
Add ClosedStreamSink, and *Completer.setError, and
StreamSinkCompleter.fromFuture.

R=lrn@google.com

Committed:
https://github.com/dart-lang/async/commit/32ffb0394a53f1a29a0c63ff20d53fa4b54...
==========

[nweiz, 2016-01-25 22:39:40]

Committed patchset #2 (id:20001) manually as
32ffb0394a53f1a29a0c63ff20d53fa4b54257bd (presubmit successful).