Support teeing readable byte streams #1114

MattiasBuelens · 2021-03-20T00:52:06Z

With this PR, teeing a readable byte stream will now create two readable byte streams (instead of two "default" readable streams). See #1111 for context.

This implements the "hard" solution: when a branch is being read using a BYOB reader, the view of that read request is forwarded to the original stream, so it can read directly into that view. After the view is filled, an identical copy is created and enqueued on the other branch, so they both see the same data.

To do:

Write tests
Update specification text to match new reference implementation

At least two implementers are interested (and none opposed):
- Chrome (@ricea)
- ...
Tests are written and can be reviewed and commented upon at:
- Streams: tests for teeing readable byte streams web-platform-tests/wpt#29190
Implementation bugs are filed:
- Chrome: https://crbug.com/1227496
- Firefox: …
- Safari: …

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

index.bs

reference-implementation/lib/abstract-ops/readable-streams.js

index.bs

reference-implementation/lib/abstract-ops/readable-streams.js

index.bs

Previously, if you cancel the stream while a BYOB read is pending, the stream has to wait for the underlying byte source to call respond(0) before it can return the BYOB request's buffer to the caller. This makes underlying byte sources difficult to write in a robust way. After this change, the contract changes so that canceling a stream while a BYOB read is pending will always lead to the stream not giving you back your buffer. This means that cancel() can immediately resolve all pending BYOB reads with the classic { done: true, value: undefined }, without having to wait for the underlying byte source. This solves the problem, and would make it easier to implement an underlying byte source. To make this work, an additional change was required: when the stream is canceled, any pending BYOB request is now immediately invalidated, so the underlying byte source doesn't erroneously think that it still needs to provide a response. (This aligns with the behavior of controller.enqueue(), which throws if the stream is already closed.) See #1114 (comment) and #1123 (comment) for some discussion and background.

Previously, if you cancel the stream while a BYOB read is pending, the stream has to wait for the underlying byte source to call respond(0) before it can return the BYOB request's buffer to the caller. This makes underlying byte sources difficult to write in a robust way. After this change, the contract changes so that canceling a stream while a BYOB read is pending will always lead to the stream not giving you back your buffer. This means that cancel() can immediately resolve all pending BYOB reads with the classic { done: true, value: undefined }, without having to wait for the underlying byte source. This solves the problem, and would make it easier to implement an underlying byte source. To make this work, an additional change was required: when the stream is canceled, any pending BYOB request is now immediately invalidated, so the underlying byte source doesn't erroneously think that it still needs to provide a response. (This aligns with the behavior of controller.enqueue(), which throws if the stream is already closed.) See whatwg#1114 (comment) and whatwg#1123 (comment) for some discussion and background.

domenic

LGTM with nits. Exciting...

index.bs

ricea

I've checked the algorithms and cross-referenced against the reference implementation and it all looks good.

I haven't analysed it for re-entrancy risk. I'm a bit worried about that.

index.bs

MattiasBuelens

I've added handling for errors from cloning chunks.

Still gotta find some time to write tests for all these edge cases. 😛

index.bs

domenic

Spec looks good to me at this point! I guess the new error cases, including for non-byte tee() are testable (although probably not in the reference implementation).

index.bs

domenic

LGTM. ~~Not sure why the bots are failing; try rebasing?~~ That was the wrong PR.

@ricea feel free to take a look

MattiasBuelens · 2021-07-08T21:00:02Z

Yay! 🥳

I assume this means Chrome is interested in implementing? I'll go ahead and file an implementation bug then. 🙂

ricea · 2021-07-09T02:42:35Z

Yes, Chrome is interested.

…streams, a=testonly Automatic update from web-platform-tests Streams: tests for teeing readable byte streams Follows whatwg/streams#1114. -- wpt-commits: 87a4c80598aee5178c385628174f1832f5a28ad6 wpt-pr: 29190

This CL implements teeing support for readable byte streams, so that teeing a byte stream creates two readable byte streams instead of two default readable streams. The corresponding spec change was done in whatwg/streams#1114. Bug: 1227496 Change-Id: Ica29d939b7b3a3f7d6086b42ee1e8a538b611b25 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3757034 Commit-Queue: Nidhi Jaju <[email protected]> Reviewed-by: Adam Rice <[email protected]> Cr-Commit-Position: refs/heads/main@{#1128460}

MattiasBuelens force-pushed the tee-byte-stream branch 5 times, most recently from 18544b8 to e3c2532 Compare March 24, 2021 01:20

MattiasBuelens commented Mar 24, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

reference-implementation/lib/abstract-ops/readable-streams.js Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved