Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tyoshino opened this issue Mar 31, 2016 · 33 comments · Fixed by #1593
Closed
Assignees
Labels
addition/proposal New features or enhancements topic: streams

Comments

@tyoshino
Copy link
Member

/cc @yutakahirano

ReadableByteStream has been merged into ReadableStream interface. It's time to update the ReadableStream section of the Fetch spec (https://fetch.spec.whatwg.org/#readablestream) to adopt the updated ReadableStream and construct a ReadableStream with the byte handling feature (i.e. getReader() returns a ReadableStreamBYOBReader when a certain option is passed).

In the first step, we don't need to fully use the BYOB responding API but may just call the same enqueue() method.

@youennf
Copy link
Collaborator

youennf commented Aug 25, 2016

Currently the fetch spec allows constructing a Response with any ReadableStream, which can thus be enqueued with ArrayBuffers, Uint8Arrays or potatoes.

Restricting the Response body to a ReadableStream with a byte source would remove the possibility to enqueue potatoes, leaving space for ArrayBuffer and ArrayBufferView.

But enqueuing something else than Uint8Array would still lead to TypeError when consuming the body, since "read all bytes" requires Uint8Array objects.
Can we remove that restriction?
That would make it consistent with the possibility to create a Response with whatever ArrayBuffer/ArrayBufferView.

@annevk
Copy link
Member

annevk commented Aug 25, 2016

I thought we wanted to require Uint8Array?

@domenic
Copy link
Member

domenic commented Aug 25, 2016

Yeah, a while ago we discussed whether we should perform some auto-conversion from array buffer views or blobs or strings, and decided to settle on being conservative and just allowing Uint8Array. I think the discussion was at yutakahirano/fetch-with-streams#53.

@youennf
Copy link
Collaborator

youennf commented Aug 25, 2016

"read all bytes" is not really doing any conversion, it merely appends a Uint8Array to a byte sequence. Any BufferSource would be as easily appendable there.

I do not see what is gained with this restriction.
I can see how inconsistent it might be if Response gets restricted to a byte ReadableStream.

yutakahirano/fetch-with-streams#53 seems mostly about upload.

@annevk
Copy link
Member

annevk commented Aug 26, 2016

Yeah, though a Response object created in a service worker feeding a document/worker API is very similar to upload. I think we should try to keep them consistent.

@annevk
Copy link
Member

annevk commented Feb 20, 2017

@yutakahirano is this fixed?

@yutakahirano
Copy link
Member

Not yet.

@ricea
Copy link
Collaborator

ricea commented Feb 1, 2021

This might not happen quickly, as we want to run an experiment in Chrome to ensure it is web-compatible to use a byte stream. In theory there is no problem, but AFAIK it's never been tested and it's not something we can afford to break.

@annevk
Copy link
Member

annevk commented Feb 1, 2021

It seems we should do this before upload streams though. No reason not to have the best-in-class there.

@MattiasBuelens
Copy link

How will Request.clone() and Response.clone() work? Currently, to clone a body, we tee the body's stream. But teeing always returns two regular streams, i.e. non-byte streams. It looks like we first need to define how to tee a readable byte stream into two new readable byte streams.

This might also affect other parts of the specification. For example, in this suggestion on #1144, we'd also want to clone a Request.

@yoichio
Copy link
Contributor

yoichio commented Mar 2, 2021

I don't understand what is proposed initially and why it blocks upload streams.

  • Using ReadableStreamBYOBReader is a part of how we handle a response.
  • The Uint8 restriction is applied only if we upload streams in HTTP-network fetch and
    service worker accepts event.respondWith with non-Uint8 stream.

@yutakahirano
Copy link
Member

The Uint8 restriction is applied only if we upload streams in HTTP-network fetch and service worker accepts event.respondWith with non-Uint8 stream.

I'm not sure if I understand the comment, but a streaming request body containing a non-Uint8Array chunk should result in an errored stream in the service worker.

@annevk
Copy link
Member

annevk commented Mar 2, 2021

@yoichio if ReadableStreamBYOBReader only makes sense for reading (I'm not sure, to be clear, I haven't looked into this in detail), wouldn't it at least impact requests in service worker fetch events?

@yoichio
Copy link
Contributor

yoichio commented Mar 11, 2021

I understood ReadableStreamBYOBReader on response was not a core part of issue but accepting only Uint8Array as a streaming request body was.

but a streaming request body containing a non-Uint8Array chunk should result in an errored stream in the service worker.

However I still don't understand this behavior.
@yutakahirano, what part of the spec declares that?

@yutakahirano
Copy link
Member

Sorry the part is underspecified. We use "create a proxy" in the spec, but it doesn't match our mental mode perfectly. The request and response body transferring between the page and the service worker should reject chunks other than Uint8Arrays, and it doesn't have to preserve the chunk boundary.

@annevk
Copy link
Member

annevk commented Mar 11, 2021

@yutakahirano good point, it seems that should be somewhat straightforward to fix with the new "read incrementally" operation.

@yoichio
Copy link
Contributor

yoichio commented Mar 15, 2021

It should be done by changing "Let requestForServiceWorker be a clone of request." part in HTTP fetch to something having chunk type checking?

@annevk
Copy link
Member

annevk commented Mar 15, 2021

I suppose it's really only observable there, yeah. The network side already does the type check. In retrospect it would have been nicer if we could have done this closer to the source for both Request and Response objects, but so be it I guess. (I don't think we can change that anymore as it would have to change object identity.)

@yutakahirano
Copy link
Member

I was thinking about creating a TransformStream whose transformer

  • checks each chunk's type, and
  • may split / combine chunks.

It may be good to talk with @ricea.

@ricea
Copy link
Collaborator

ricea commented Mar 15, 2021

I was thinking about creating a TransformStream whose transformer

  • checks each chunk's type, and
  • may split / combine chunks.

I've thought the same thing. The question I got stuck on in the past is: do we want to define a "reasonable" chunk size to split/combine to?

@annevk
Copy link
Member

annevk commented Mar 15, 2021

Personally I think it's fine to leave that <a>implementation-defined</a> until it becomes a problem.

annevk pushed a commit that referenced this issue Apr 15, 2021
This change addresses the inconsistency discussed in #267 that a chunk type in a stream body on uploading to the network is limited to Uint8Array, but passing it to a service worker is not restricted. This change limits the latter as well.

Tests: web-platform-tests/wpt#28203.
@yoichio
Copy link
Contributor

yoichio commented Apr 19, 2021

Can we unblock this from upload-streaming-blocking following #1199 (comment) ?

@annevk
Copy link
Member

annevk commented Apr 19, 2021

I think so, but it would be great if @domenic @MattiasBuelens or @ricea could elaborate on what BYOB stream adoption would mean for Fetch first and how feasible it is to get there if we ship things as-is (i.e., without BYOB).

@domenic
Copy link
Member

domenic commented Apr 19, 2021

On a spec level, we're trying to tackle whatwg/streams#1121 to let the spec infrastructure create readable byte streams.

The upgrade path from vanilla readable streams to byte readable streams is seamless: before, getReader({ type: "byob" }) would throw. After implementation, it would work.

@NikoKyriakid
Copy link

NikoKyriakid commented Mar 2, 2022

On a spec level, we're trying to tackle whatwg/streams#1121 to let the spec infrastructure create readable byte streams.

The upgrade path from vanilla readable streams to byte readable streams is seamless: before, getReader({ type: "byob" }) would throw. After implementation, it would work.

Can that be done currently on Chrome ?
I am trying to make response.body to be handled as byte stream so I get use getReader({ type: "byob" }) with the goal to specify byteLength, which we want to be 64K steadily. But I get

TypeError: Failed to execute 'getReader' on 'ReadableStream': Cannot use a BYOB reader with a non-byte stream

@MattiasBuelens
Copy link

Can that be done currently on Chrome ?

No. First, the specification needs to be updated, that's what this issue is about. Afterwards, Chrome can implement the spec change and make it available to its users.

I am trying to make response.body to be handled as byte stream so I get use getReader({ type: "byob" }) with the goal to specify byteLength, which we want to be 64K steadily.

Note that a BYOB reader doesn't guarantee that it'll fill your entire 64 KB view on every read(view). That'll need a new method on ReadableStreamBYOBReader, see whatwg/streams#1143.

@NikoKyriakid
Copy link

NikoKyriakid commented Mar 2, 2022

Thank you @MattiasBuelens for the reply and the helpful suggestions. You just saved my day.

@saschanaz
Copy link
Member

2022 update: Firefox shipped byte stream as of version 102, and it shipped Fetch with byte stream (which was not really intentional but accidental IMO, given that there is still no relevant test). And thus this has been working on Firefox for months:

new Response(new Uint8Array([1,2])).body.getReader({mode: 'byob'})

@ricea
Copy link
Collaborator

ricea commented Jan 10, 2023

2022 update: Firefox shipped byte stream as of version 102, and it shipped Fetch with byte stream

That's great news! It implies that it is web-compatible. We are currently evaluating the priority of implementing it in Chrome.

@annevk
Copy link
Member

annevk commented Jan 10, 2023

Given #267 (comment) is the only thing that's missing here WPT tests or do we need a specification change still?

@saschanaz
Copy link
Member

saschanaz commented Jan 10, 2023

whatwg/streams#1130 added "set up with byte reading support" algorithm, so I guess another work item here is to use that.

@saschanaz
Copy link
Member

Shall I interpret #267 (comment) as a support from Google? I can write a PR in that case.

@ricea
Copy link
Collaborator

ricea commented Jan 11, 2023

Shall I interpret #267 (comment) as a support from Google? I can write a PR in that case.

Yes. We strongly support using byte streams for .body.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: streams
10 participants