pipeTo() pulls the source even when the dest is not yet writable

[tyoshino]

Forked from #146

aedbf18 changed pipeTo() to call wait() even when the dest is not yet in "writable" state. If this is unexpected, we can change the argument of the Promise.race() call for source:"waiting" && dest:"waiting" to source.closed to watch only "closed" and "errored" event.

Maybe need to wait for conclusion on #146.

[domenic]

The intention is that you only write() to the writable stream when it's in the "writable" state. But calling wait() is fine; the entire purpose of wait() is to allow you to figure out when the stream becomes "writable" so you can write() to it while respecting backpressure.

That said I haven't read through and processed #146 yet so maybe there is something more subtle I am missing.

[tyoshino]

Ah, sorry. I meant calling wait() on the source. Before aedbf18, wait() was called on the source only after we see dest transitions to "writable" but the new pipeTo() code just calls wait() on both and Promise.race() on them. ReadableStream.wait() has a side effect in addition to returning the waitPromise, that is calling [[callOrSchedulePull]]. So, this introduces a subtle change. Though it's subtle we should explicitly choose one from the two approaches (old and new) and give some justification to the decision.

In the first place, we should think of a problem whether we should call [[callOrSchedulePull]] from wait() or not. pull is not called until the first wait() is called on the ReadableStream. So, we can delay the first pull call by not calling wait(). But after the first call, independent from the consumer's choice (call or not calling wait()), the ReadableStream automatically calls [[callOrSchedulePull]] when the [[queue]] becomes empty and [[callOrSchedulePull]] calls by wait() are basically ignored (by [[pulling]] flag).

[domenic]

Oh, I see. So it seems like it is better now than it was before, because now it is consistent. But I agree there is a larger issue.

For pipeTo, it seems like calling wait() asap is probably good. Because, it is good to have the data ready, possibly before the stream becomes writable: it is good to do the "pull data" and "wait for writable" in parallel.

But more generally, I am not sure about automatically scheduling a pull when the queue becomes empty. I would think we would do that only if the HWM was non-zero (or more generally, if needsMore() returns true).

Does that make sense, and does it seem like the right performance tradeoff?

[tyoshino]

For pipeTo, it seems like calling wait() asap is probably good. Because, it is good to have the data ready, possibly before the stream becomes writable: it is good to do the "pull data" and "wait for writable" in parallel.

Agreed!

But more generally, I am not sure about automatically scheduling a pull when the queue becomes empty. I would think we would do that only if the HWM was non-zero (or more generally, if needsMore() returns true).

For the current spec, I agree that pull should be invoked when needsMore() returns true.

In the new API proposal (addition of some new methods for precise flow control information exchanging) I'm writing, I want to notify the underlying source of any change of amount queued in [[queue]] so that the source can probe new amount of space in [[queue]] and do some pulling operation based on that value. For the proposal, I want pull to be invoked every time read() is called.

[tyoshino]

For the current spec, I agree that pull should be invoked when needsMore() returns true.

Prototyped
tyoshino/streams@whatwg:master...tyoshino:callpullonneedsmore

[tyoshino]

Links to remaining issues (the pull request got closed as I cleaned up the branch...).
#169 (comment)
#169 (comment)

[domenic]

I think this is fixed by the work in 032c4a8! The "remaining issues" in your last comment are captured mainly by #190.

Please re-open if I missed something.

[tyoshino]

Agreed! wait() no longer has side-effect. Condition about pull() invocation has been also fixed.

Remaining issues

Yes!