Make it possible to not complete a request when the event source is done.

Make it possible to not complete a request when the event source is done.

Abstract the event source of a stream queue so it doesn't have to be a stream.

This is in anticipation of adding look-ahead which will need to keep the
request alive until the user fast-forwards or rewindes, even after the
original event source is done, and which will need a stream queue fed by
the look-ahead request, not by a stream.

R=nweiz@google.com

Committed: https://github.com/dart-lang/async/commit/5661c47a5fdeb7168952c5bb2ed3010aea2050c3

[Lasse Reichstein Nielsen, 2015-08-21 12:11:47]

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

[nweiz, 2015-08-21 19:43:57]

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (left):

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#o...
lib/src/stream_queue.dart:261: 
Nit: keep this empty line.

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

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:80: /// Whether the event source done.
"is done"

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:244: Future _cancel();
It might be clearer to explicitly divide these private methods into two groups:
methods called by the subclass to signal something for the superclass, and
methods called by the superclass to signal something for the subclass. Without
that, I found it hard to parse out what's called when and in which direction.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:260: void _onDone() {
Document this.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:284: /// May be called by requests which have stalled
after the event queue
"stalled" is confusing for me here—it's not a term that's used elsewhere, and it
makes me think of timeouts which I don't think is an accurate understanding. Can
you clarify?

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:294: if (!_isDone) {
Nit: I find it tends to look better when block-level statements are separated by
a newline. Also applies to a few methods below.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:318: 
Nit: extra newline.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:338: void _onData(T data) {
For what it's worth, these methods are so small I'd just write them inline in
[_ensureListening].

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:371: return _sourceStream;
Nit: consider making this a single-line if.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:376: if (!subscription.isPaused) subscription.pause();
Why pause and re-resume it?

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:384: 
Nit: three newlines here seems excessive. It's also inconsistent with two
newlines above.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:417: /// If the function returns `false` when the
stream has already closed
"the function" -> "this method" or "it"

What are circumstances where we expect this to return `false` when [isDone] is
`true`? If we don't actually expect that to happen, it would be simpler to just
disallow it, and limit the number of circumstances in which a request will call
private methods on the queue.

[Lasse Reichstein Nielsen, 2015-08-23 11:30:01]

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

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:417: /// If the function returns `false` when the
stream has already closed
That's exactly for "lookahead". The look-ahead may still wait for the user to
decide on rollback/fast-forward, and the user won't know what to do until after
seeing the end of the stream (like being looking for a "foo" event, only at the
end can you see that there were none).

[Lasse Reichstein Nielsen, 2015-08-24 08:31:33]

PTAL

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (left):

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#o...
lib/src/stream_queue.dart:261: 
I've documented the function instead.

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

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:80: /// Whether the event source done.
On 2015/08/21 19:43:57, nweiz wrote:
> "is done"

Done.
I mean: Is done! :)

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:244: Future _cancel();
Good idea.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:260: void _onDone() {
On 2015/08/21 19:43:57, nweiz wrote:
> Document this.

Done.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:284: /// May be called by requests which have stalled
after the event queue
On 2015/08/21 19:43:57, nweiz wrote:
> "stalled" is confusing for me here—it's not a term that's used elsewhere, and
it
> makes me think of timeouts which I don't think is an accurate understanding.
Can
> you clarify?

Done.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:294: if (!_isDone) {
I've added some extra lines (not all between block statements, some include a
line before or after that belongs with the loop/if block, and some places I just
didn't think looked better with empty lines (small functions tend to be look
broken up by the extra lines).

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:318: 
On 2015/08/21 19:43:57, nweiz wrote:
> Nit: extra newline.

Done.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:338: void _onData(T data) {
On 2015/08/21 19:43:57, nweiz wrote:
> For what it's worth, these methods are so small I'd just write them inline in
> [_ensureListening].

Done.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:371: return _sourceStream;
I prefer to not hide significant control flow (return/throw) at the end of an if
(if it's an early bailout, then it's ok).

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:376: if (!subscription.isPaused) subscription.pause();
We need to resume it if it was paused, because the SubscriptionStream won't
resume the subscription.

I want to avoid resuming first because that would cause a "resume the pause",
and pause isn't necessarily free if you have to start/stop low-level IO access.
So, I wanted to resume my pause after the SubscriptionStream was created, and
not before.
This was a simple way to do that - another alternative was to remember whether
it was paused in a bool variable, and then only resume afterwards if necessary.
That's probably more readable, so let's do that.

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:384: 
True, should only be two.

[nweiz, 2015-08-25 00:38:21]

lgtm

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

https://codereview.chromium.org/1305063002/diff/1/lib/src/stream_queue.dart#n...
lib/src/stream_queue.dart:417: /// If the function returns `false` when the
stream has already closed
On 2015/08/23 11:30:00, Lasse Reichstein Nielsen wrote:
> That's exactly for "lookahead". The look-ahead may still wait for the user to
> decide on rollback/fast-forward, and the user won't know what to do until
after
> seeing the end of the stream (like being looking for a "foo" event, only at
the
> end can you see that there were none).

SGTM

https://codereview.chromium.org/1305063002/diff/20001/lib/src/stream_queue.dart
File lib/src/stream_queue.dart (right):

https://codereview.chromium.org/1305063002/diff/20001/lib/src/stream_queue.da...
lib/src/stream_queue.dart:446: /// ([isDone] is true), then the request must
call [StreamQueue._updateRequests]
Long line

[Lasse Reichstein Nielsen, 2015-08-25 07:51:56]

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