Allow other specifications to create readable byte streams #1130

domenic · 2021-06-02T21:16:38Z

Closes #1121.

Users:

Preview | Diff

ricea

lgtm

index.bs

ricea · 2021-06-04T06:13:20Z

index.bs

+  1. Perform ! [$ReadableByteStreamControllerEnqueue$](|stream|.[=ReadableStream/[[controller]]=],
+     |uint8Array|).
+ 1. Otherwise,
+  1. Write |bytes| into


Do we need an assertion that it will fit?

Once we start using whatwg/webidl#987 there will be such an assertion transitively, but it can't hurt to add it here too.

domenic · 2021-06-04T12:46:35Z

Marking as "do not merge yet" pending #1121 and updating to use whatwg/webidl#987.

Define pullAlgorithm and cancelAlgorithm, and set up ReceiveStream with them. This relies on whatwg/streams#1130. Fixes #185.

domenic · 2021-06-29T15:45:35Z

This is ready to merge but we're getting some good feedback in w3c/webtransport#297 (comment) so I'll hold off a bit...

domenic

@yutakahirano, can you take a look at the latest commit? I should also update the Fetch and Web Serial PRs to make sure this works well for them too.

domenic · 2021-07-01T22:46:34Z

index.bs

+ 1. If |byobRequest| is non-null, and |view|.\[[ViewedArrayBuffer]].\[[ArrayBufferData]] is the same
+    [=Data Block=] as |byobRequest|.\[[view]].\[[ViewedArrayBuffer]].\[[ArrayBufferData]], then:
+  1. Assert: |view|.\[[ByteOffset]] is |byobRequest|.\[[view]].\[[ByteOffset]].
+  1. Assert: |view|.\[[ByteLength]] ≤ |byobRequest|.\[[view]].\[[ByteLength]].


I made these asserts instead of if tests (compared to w3c/webtransport#297 (comment)) since it seems like it would be bad if the caller used the view but went past the requested bounds.

domenic · 2021-07-01T22:46:57Z

index.bs

+     |view|.\[[ByteLength]]).
+  1. Otherwise, perform ?
+     [$ReadableByteStreamControllerRespondWithNewView$](|stream|.[=ReadableStream/[[controller]]=],
+     |view|).


I handled the case where you transfer the backing Data Block here, which goes a bit beyond w3c/webtransport#297 (comment) but seems like a good idea.

index.bs

yutakahirano

Thank you, LGTM!

Closes #1121.

domenic · 2021-07-08T20:52:19Z

Almost done. Still TODO: update Fetch and Web Serial PRs for the new WebTransport-inspired model, to make sure this still works.

domenic · 2021-07-12T18:22:16Z

Seems to work. There's a bit of boilerplate if you, like the Web Serial and Fetch specs, already have a byte sequence on hand. Then you need to branch as to whether there's a BYOB request (and, in the case of Fetch, whether the byte sequence fits). I considered adding a helper for this, which would put us back to "enqueue bytes given a byte sequence", but I think it's OK to be more explicit. In particular it's more obviously optimizable by reading the spec in question, instead of having to go to Streams.

Still, to leave open the door for such a helper in the future, I'll rename the operation from "enqueue bytes" to "enqueue view".

Any final reviews, of either this, WICG/serial#137, or whatwg/fetch#1246 would be appreciated. If there aren't any, I'll merge this tomorrow-ish.

index.bs

Co-authored-by: Mattias Buelens <[email protected]>

MattiasBuelens · 2021-07-12T21:32:16Z

Okay, there's still an important problem. After a specification calls "close a readable stream" on a readable byte stream, the current BYOB request is still pending. We must somehow perform the equivalent of controller.byobRequest.respond(0) (like we do in our tests) to return ownership of the BYOB buffer and fulfill any pending byobReader.read(view) calls.

I suppose we have to extend "close a readable stream" to handle this? But then:

Is it safe to assume that, if the other specification calls "close a readable stream", then it is done using the current BYOB request view?
What if the other specification has transferred the current BYOB request view to a different realm? 😬

domenic · 2021-07-12T22:43:21Z

I think we can mandate that other specifications not transfer the current BYOB request view. Even if implementations do so (and then do the correspondingly-different ending dance), it wouldn't be observable, so we can write the specifications to assume they don't.

Similarly I think we can require that other specifications not write into the current BYOB request view after closing the readable stream.

MattiasBuelens

That seems reasonable. In that case, we need to do something similar as in the read request close steps of ReadableByteStreamTee. (See suggestion below.)

index.bs

Co-authored-by: Mattias Buelens <[email protected]>

domenic · 2021-07-14T20:25:52Z

Alright, this is ready to merge, but since previous waiting periods have turned up great finds, I'll give it another day before doing so.

ricea · 2021-07-14T20:55:02Z

index.bs

+  1. If |stream|.[=ReadableStream/[[controller]]=].[=ReadableByteStreamController/[[pendingPullIntos]]=]
+     is not [=list/is empty|empty=], perform !
+     [$ReadableByteStreamControllerRespond$](|stream|.[=ReadableStream/[[controller]]=], 0).
+     <p class="note">The caller has to make sure not to write into the


I feel some ambiguity here. How long before closing the stream is it okay to write into the view?

Yeah, I think this does not match my understanding of #1130 (comment) . Reviewing that, I think instead we want to say something like:

The calling specification must never transfer the current BYOB request view. Note: implementations could do something equivalent to a transfer, but in that case they'd need to modify the steps in close to use RespondWithNewView instead of Respond(,0) to keep the same observable behavior as not transfering.

The calling specification must not write into the current BYOB request view after closing the stream.

@MattiasBuelens, WDYT?

The calling specification must never transfer the current BYOB request view. Note: implementations could do something equivalent to a transfer, but in that case they'd need to modify the steps in close to use RespondWithNewView instead of Respond(,0) to keep the same observable behavior as not transfering.

That's correct.

Do we want to provide an algorithm for closing a readable byte stream after the calling specification has already transferred the current BYOB request view? Or maybe add an optional newByobBuffer argument and switch on that to use either Respond(0) or RespondWithNewView(emptyViewOnNewByobBuffer)? (Kinda like the closeWithNewView() method I suggested in #1123.)

The calling specification must not write into the current BYOB request view after closing the stream.

Makes sense. If they write into the BYOB request, they should enqueue it first, and then close the stream.

ricea · 2021-07-14T20:57:49Z

index.bs

+ To <dfn export for="ReadableStream">error</dfn> a {{ReadableStream}} |stream| given a JavaScript
+ value |e|:
+
+ 1. If |stream|.[=ReadableStream/[[controller]]=] [=implements=] {{ReadableByteStreamController}},


All this switching on controller type is unfortunate. If we had more types we might add some polymorphism to avoid it. But with only two types this is probably simpler.

ricea · 2021-07-14T21:19:41Z

index.bs

+  1. Let |byobRequest| be !
+     [$ReadableByteStreamControllerGetBYOBRequest$](|stream|.[=ReadableStream/[[controller]]=]).
+  1. If |byobRequest| is non-null, and |chunk|.\[[ViewedArrayBuffer]].\[[ArrayBufferData]] is the
+     same [=Data Block=] as |byobRequest|.\[[view]].\[[ViewedArrayBuffer]].\[[ArrayBufferData]],


There may be a problem here. If the calling specification needs to transfer |byobRequest|.[[view]].[[ViewedArrayBuffer]] for some reason, then it will be detached and not match.

Ah, but [[ArrayBufferData]] will still match. That's the hidden internal slot that the ECMAScript spec uses to abstract the concept of "memory regions", so it will survive such transfers.

But if it's transferred it will be detached, right? And if it's detached then [[ArrayBufferData]] is set to null: https://tc39.es/ecma262/#sec-detacharraybuffer

Oh, dang, you're right. In that case we should probably solve it by, per the above discussion, prohibiting transferring at a spec level (but allowing it an implementation level as long as the implementation does appropriate behind-the-scenes contortions).

I think that's fine. If someone finds they need to transfer the buffer we can revisit.

D'oh, how could I forget? I even explained this in #1123. 😛

We could make two separate algorithms: one for enqueuing a new chunk, and another for responding with a (transferred) BYOB request view. We still can't check if the given buffer is a transferred version of the BYOB request's buffer, so we'd have to rely on the calling specification to not mess this up.

Or yeah, we could just forbid transferring for now. 😁

ricea

lgtm

domenic mentioned this pull request Jun 2, 2021

Make response body streams readable byte streams whatwg/fetch#1246

Closed

3 tasks

domenic added the other spec interface How the Streams spec interfaces with other specifications label Jun 2, 2021

ricea approved these changes Jun 4, 2021

View reviewed changes

domenic added the do not merge yet Pull request must not be merged per rationale in comment label Jun 4, 2021

domenic mentioned this pull request Jun 22, 2021

Use readable byte streams WICG/serial#137

Merged

domenic force-pushed the setup-rbs branch from 611a312 to 157d374 Compare June 25, 2021 19:28

domenic mentioned this pull request Jun 25, 2021

Add and improve operations on BufferSources whatwg/webidl#987

Merged

yutakahirano added a commit to w3c/webtransport that referenced this pull request Jun 28, 2021

Set up ReceiveStream with proper algorithms

503696f

Define pullAlgorithm and cancelAlgorithm, and set up ReceiveStream with them. This relies on whatwg/streams#1130. Fixes #185.

yutakahirano mentioned this pull request Jun 28, 2021

Set up ReceiveStream with proper algorithms w3c/webtransport#297

Merged

domenic commented Jul 1, 2021

View reviewed changes

MattiasBuelens reviewed Jul 1, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

MattiasBuelens reviewed Jul 1, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

yutakahirano approved these changes Jul 6, 2021

View reviewed changes

Allow other specifications to create readable byte streams

195d45a

Closes #1121.

domenic force-pushed the setup-rbs branch from 903a1de to 195d45a Compare July 8, 2021 20:51

domenic mentioned this pull request Jul 12, 2021

ReSpec no longer seems to allow {{[internal slot]}} references WICG/serial#143

Closed

Enqueue a view

60c37bb

MattiasBuelens requested changes Jul 12, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

domenic and others added 4 commits July 12, 2021 17:07

Update index.bs

80c1824

Co-authored-by: Mattias Buelens <[email protected]>

Update index.bs

3efb6a2

Co-authored-by: Mattias Buelens <[email protected]>

Update index.bs

ca10093

Co-authored-by: Mattias Buelens <[email protected]>

Add assert

8c892e5

MattiasBuelens requested changes Jul 12, 2021

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Update index.bs

df3493d

Co-authored-by: Mattias Buelens <[email protected]>

Consolidate the algorithms

a5a82b6

MattiasBuelens approved these changes Jul 14, 2021

View reviewed changes

ricea reviewed Jul 14, 2021

View reviewed changes

Disallow transferring/detaching

b6e8404

ricea approved these changes Jul 15, 2021

View reviewed changes

domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Jul 15, 2021

domenic merged commit bb53bec into main Jul 15, 2021

domenic deleted the setup-rbs branch July 15, 2021 18:51

saschanaz mentioned this pull request Jan 10, 2023

Use a ReadableStream with byte source (formerly called ReadableByteStream) for .body whatwg/fetch#267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow other specifications to create readable byte streams #1130

Allow other specifications to create readable byte streams #1130

domenic commented Jun 2, 2021 •

edited by pr-preview bot

Loading

ricea left a comment

ricea Jun 4, 2021

domenic Jun 4, 2021

domenic commented Jun 4, 2021

domenic commented Jun 29, 2021

domenic left a comment

domenic Jul 1, 2021

domenic Jul 1, 2021

yutakahirano left a comment

domenic commented Jul 8, 2021

domenic commented Jul 12, 2021

MattiasBuelens commented Jul 12, 2021

domenic commented Jul 12, 2021

MattiasBuelens left a comment

domenic commented Jul 14, 2021

ricea Jul 14, 2021

domenic Jul 14, 2021

MattiasBuelens Jul 14, 2021 •

edited

Loading

ricea Jul 14, 2021

ricea Jul 14, 2021

domenic Jul 14, 2021

ricea Jul 14, 2021

domenic Jul 14, 2021

ricea Jul 14, 2021

MattiasBuelens Jul 14, 2021

ricea left a comment

Allow other specifications to create readable byte streams #1130

Allow other specifications to create readable byte streams #1130

Conversation

domenic commented Jun 2, 2021 • edited by pr-preview bot Loading

ricea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Jun 4, 2021

domenic commented Jun 29, 2021

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutakahirano left a comment

Choose a reason for hiding this comment

domenic commented Jul 8, 2021

domenic commented Jul 12, 2021

MattiasBuelens commented Jul 12, 2021

domenic commented Jul 12, 2021

MattiasBuelens left a comment

Choose a reason for hiding this comment

domenic commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattiasBuelens Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricea left a comment

Choose a reason for hiding this comment

domenic commented Jun 2, 2021 •

edited by pr-preview bot

Loading

MattiasBuelens Jul 14, 2021 •

edited

Loading