Change the model for ReadableStream to have async read() #296

domenic · 2015-03-12T09:18:19Z

This replaces the dual ready + read() approach previously, which was derived from the epoll(7) + read(2) paradigm. In #253 we did a deep dive on real-world implementation strategies for byte streams, and discovered that the ready + read() model causes a conflict with the semantics we want for byte streams. Briefly, because some byte streams will demand to know the size of the buffer they must fill before doing any I/O (the fread(3) model), the byte stream method that reads into a given typed array must be asynchronous. If such byte streams are then to conform to the readable stream interface, with a no-argument read() method that is an auto-allocating version of the read-into method, then read() must also be async, across all readable streams.

This is a usability upgrade for consumers, in many cases. However, for non-byte streams, it potentially costs more microtasks when multiple chunks of data would be available synchronously. We are hopeful that even if current JS engine microtask queues are not as fast as they could be, they will improve over time until this overhead is negligible (which in theory it should be).

Alongside this change, we moved the read() method from the readable stream to the reader (now called "readable stream reader" instead of "exclusive stream reader"). This drastically simplifies the stream/reader interaction, and also allows the possibility of different types of readers which have different reading behavior---again, very helpful for byte streams. The usability impact is also positive, as it guides consumers toward using piping instead of directly reading chunks from the stream.

Note that read() promises fulfill with { value, done } instead of using an EOS sentinel value. This avoids a number of problems (see extensive discussion in #253), and also provides a mechanism by which readable byte streams can smuggle out "unused" buffers given to them, by using { value: zeroLengthViewOntoBuffer, done: true }.

Finally, the state property is now removed (from readable stream), since there is no longer a "waiting" vs. "readable" distinction.

This commit also adds some new infrastructure for templated tests, and ports some portion of the existing tests there. This is our solution for #217 and #264.

Note that we re-merged all related code into a single readable-stream.js file, as the setup with the three separate files (readable-stream.js, exclusive-stream-reader.js, and readable-stream-abstract-ops.js) was problematic in causing circular dependencies.

Closes Support reading bytes into buffers allocated by user code on platforms where only async read is available #253!
Closes Requirement on promises equality #266 by simplifying reader-related stuff, removing the problematic ready promise, and ensuring that cancel() always returns a new promise instead of reusing [[closedPromise]].
Closes Test ReadableStream does not call pull multiple times after previous pull finishes is incorrectly specified. #291 since the relevant tests have been re-written and the related infrastructure around pull simplified.
Closes Factor out portions of a test suite that can be run against already-constructed streams #264 by introducing templated tests.

We apologize for this large of a change, this late in the game! We really got bit by not considering the zero-copy byte stream use case carefully enough. We spent a lot of concentrated time working out how this change will enable byte streams, and @tyoshino is working on a prototype implementation over in #287 which will allow us to bring byte streams into the main spec/reference implementation/test suite soon. In the end, it's painful to see this large of a diff, but the result will be a lot better from many angles---not only byte stream compatibility, but general developer-facing ergonomics as well.

Branch snapshot at https://streams.spec.whatwg.org/branch-snapshots/async-read-take-3/. Sections that are good for at-a-glance understanding of the change, or sections that have seen particular rework, include:

New discussion of locking
New examples of using readable streams
New ReadableStream and ReadableStreamReader class definitions
New ReadableStreamReader.prototype.read() method definition, including non-normative description
New section on "other streams", replacing the crusty "subclassing streams" section, describing how different types of streams (e.g. ReadableStreams and putative ReadableByteStreams) share common idioms and can all interoperate with anything obeying the writable stream contract.

tyoshino · 2015-03-16T12:51:32Z

reference-implementation/lib/readable-stream.js

+
+    if (this._ownerReadableStream === undefined) {
+      return Promise.resolve(undefined);
+    }


Return a promise rejected with the error if this was released when it got errored?

Good catch, will update

Amended last commit in the PR with fix

tyoshino · 2015-03-16T13:09:55Z

reference-implementation/lib/readable-stream.js

+function CreateReadableStreamCloseFunction(stream) {
+  return () => {
+    if (stream._state !== 'readable') {
+      return undefined;


It's out of the main topic of the PR, but don't we want to say "you're making a mistake" when close()-ed twice or more?

Hmm. I think you are right. Underlying source should be more careful than this. Open a new issue?

Created #298

tyoshino · 2015-03-16T13:31:21Z

Took a look at readable-stream.js. Overall looks good.

domenic · 2015-03-16T13:39:03Z

Awesome. index.bs review would be good too (especially to the non-algorithm text). Tests might not be worth it.

tyoshino · 2015-03-16T13:40:16Z

https://streams.spec.whatwg.org/branch-snapshots/async-read-take-3/ ?

domenic · 2015-03-16T13:41:00Z

Yep!

This replaces the dual ready + read() approach previously, which was derived from the epoll(7) + read(2) paradigm. In #253 we did a deep dive on real-world implementation strategies for byte streams, and discovered that the ready + read() model causes a conflict with the semantics we want for byte streams. Briefly, because some byte streams will demand to know the size of the buffer they must fill before doing any I/O (the fread(3) model), the byte stream method that reads into a given typed array must be asynchronous. If such byte streams are then to conform to the readable stream interface, with a no-argument read() method that is an auto-allocating version of the read-into method, then read() must also be async, across all readable streams. This is a usability upgrade for consumers, in many cases. However, for non-byte streams, it potentially costs more microtasks when multiple chunks of data would be available synchronously. We are hopeful that even if current JS engine microtask queues are not as fast as they could be, they will improve over time until this overhead is negligible (which in theory it should be). Alongside this change, we moved the read() method from the readable stream to the reader (now called "readable stream reader" instead of "exclusive stream reader"). This drastically simplifies the stream/reader interaction, and also allows the possibility of different types of readers which have different reading behavior---again, very helpful for byte streams. The usability impact is also positive, as it guides consumers toward using piping instead of directly reading chunks from the stream. Note that read() promises fulfill with { value, done } instead of using an EOS sentinel value. This avoids a number of problems (see extensive discussion in #253), and also provides a mechanism by which readable byte streams can smuggle out "unused" buffers given to them, by using { value: zeroLengthViewOntoBuffer, done: true }. Finally, the state property is now removed (from readable stream), since there is no longer a "waiting" vs. "readable" distinction. This commit also adds some new infrastructure for _templated tests_, and ports some portion of the existing tests there. This is our solution for #217 and #264. Note that we re-merged all related code into a single readable-stream.js file, as the setup with the three separate files (readable-stream.js, exclusive-stream-reader.js, and readable-stream-abstract-ops.js) was problematic in causing circular dependencies. - Closes #253! - Closes #266 by simplifying reader-related stuff, removing the problematic `ready` promise, and ensuring that cancel() always returns a new promise instead of reusing [[closedPromise]]. - Closes #264 by introducing templated tests.

See discussion in #297. This commit implements the following changes: - Allow acquiring readers for closed or errored streams; they simply act closed or errored. - Stop auto-releasing readers when streams close/error. - Disallow canceling a stream that is locked to a reader (you should use the reader cancel). - Piping from a closed or errored stream will close or abort the destination stream, instead of immediately failing the pipe.

See discussion in #297. pipeTo now returns a { finished, unpipe() } object, instead of just a promise.

See #297 (comment)

This reverts commit 0de787a. Per discussions in #297, we can punt on unpiping until we also figure out cancelable promises. Conflicts: reference-implementation/lib/readable-stream.js

Per discussion in #297. We can always add it back later if we really want it, but it has become increasingly useless in the newer, reader-centric design.

The conservative thing to do is to not have this ability for now. Released readers are now indistinguishable from readers for closed streams.

As illustrated by the new tests, there were previously issues around auto-releasing not always giving the right result for errored or closed streams, especially with regard to allowing future readers to be acquired. This commit simplifies the logic and ensures consistency across all readers obtained from errored or closed streams.

Readable stream transitioned from .ready + sync .read() to async .read() in whatwg/streams#296. This PR updates the examples to use this new API. It also removes a lot of trailing whitespace. The diff might be best viewed without whitespace tracked.

domenic assigned tyoshino Mar 12, 2015

yutakahirano mentioned this pull request Mar 13, 2015

Getting errored/closed reader, Auto-releasing reader, Forcible cancel() on a stream #297

Closed

tyoshino mentioned this pull request Mar 16, 2015

Add readableAmount getter to ReadableByteStream #279

Closed

domenic force-pushed the async-read-take-3 branch from 9b52d30 to 0aabdda Compare March 16, 2015 09:49

tyoshino reviewed Mar 16, 2015
View reviewed changes

domenic force-pushed the async-read-take-3 branch from 0aabdda to 385e7dd Compare March 16, 2015 12:59

tyoshino reviewed Mar 16, 2015
View reviewed changes

tyoshino mentioned this pull request Mar 16, 2015

Warning misbehaving underlying sources #298

Closed

domenic force-pushed the async-read-take-3 branch from 385e7dd to be3bb61 Compare March 16, 2015 13:34

domenic added 9 commits March 17, 2015 16:13

Update pipeTo to allow unpiping

726b08d

See discussion in #297. pipeTo now returns a { finished, unpipe() } object, instead of just a promise.

Add back auto-releasing of readers

3d44c82

See #297 (comment)

Revert "Update pipeTo to allow unpiping"

963c5c4

This reverts commit 0de787a. Per discussions in #297, we can punt on unpiping until we also figure out cancelable promises. Conflicts: reference-implementation/lib/readable-stream.js

Remove ReadableStream.prototype's closed getter

3be6ad6

Per discussion in #297. We can always add it back later if we really want it, but it has become increasingly useless in the newer, reader-centric design.

Remove reader.isActive

07f8a58

The conservative thing to do is to not have this ability for now. Released readers are now indistinguishable from readers for closed streams.

Update requirements doc with new byte stream discoveries

0c6240f

domenic force-pushed the async-read-take-3 branch from be3bb61 to 0c6240f Compare March 17, 2015 07:14

domenic merged commit 0c6240f into master Mar 17, 2015

tyoshino mentioned this pull request Mar 17, 2015

Requirement on promises equality #266

Closed

domenic mentioned this pull request Mar 25, 2015

Update for readable stream spec changes sysapps/tcp-udp-sockets#86

Merged

domenic deleted the async-read-take-3 branch April 20, 2015 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the model for ReadableStream to have async read() #296

Change the model for ReadableStream to have async read() #296

domenic commented Mar 12, 2015

tyoshino Mar 16, 2015

domenic Mar 16, 2015

domenic Mar 16, 2015

tyoshino Mar 16, 2015

domenic Mar 16, 2015

tyoshino Mar 16, 2015

tyoshino commented Mar 16, 2015

domenic commented Mar 16, 2015

tyoshino commented Mar 16, 2015

domenic commented Mar 16, 2015

Change the model for ReadableStream to have async read() #296

Change the model for ReadableStream to have async read() #296

Conversation

domenic commented Mar 12, 2015

tyoshino Mar 16, 2015

Choose a reason for hiding this comment

domenic Mar 16, 2015

Choose a reason for hiding this comment

domenic Mar 16, 2015

Choose a reason for hiding this comment

tyoshino Mar 16, 2015

Choose a reason for hiding this comment

domenic Mar 16, 2015

Choose a reason for hiding this comment

tyoshino Mar 16, 2015

Choose a reason for hiding this comment

tyoshino commented Mar 16, 2015

domenic commented Mar 16, 2015

tyoshino commented Mar 16, 2015

domenic commented Mar 16, 2015