fetch icon indicating copy to clipboard operation
fetch copied to clipboard

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

Open tyoshino opened this issue 9 years ago • 27 comments

/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.

tyoshino avatar Mar 31 '16 13:03 tyoshino

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.

youennf avatar Aug 25 '16 13:08 youennf

I thought we wanted to require Uint8Array?

annevk avatar Aug 25 '16 14:08 annevk

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 https://github.com/yutakahirano/fetch-with-streams/issues/53.

domenic avatar Aug 25 '16 14:08 domenic

"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.

https://github.com/yutakahirano/fetch-with-streams/issues/53 seems mostly about upload.

youennf avatar Aug 25 '16 15:08 youennf

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 avatar Aug 26 '16 06:08 annevk

@yutakahirano is this fixed?

annevk avatar Feb 20 '17 10:02 annevk

Not yet.

yutakahirano avatar Feb 20 '17 10:02 yutakahirano

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.

ricea avatar Feb 01 '21 08:02 ricea

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

annevk avatar Feb 01 '21 08:02 annevk

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.

MattiasBuelens avatar Feb 12 '21 10:02 MattiasBuelens

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.

yoichio avatar Mar 02 '21 08:03 yoichio

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.

yutakahirano avatar Mar 02 '21 08:03 yutakahirano

@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?

annevk avatar Mar 02 '21 08:03 annevk

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?

yoichio avatar Mar 11 '21 06:03 yoichio

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.

yutakahirano avatar Mar 11 '21 07:03 yutakahirano

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

annevk avatar Mar 11 '21 07:03 annevk

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

yoichio avatar Mar 15 '21 05:03 yoichio

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.)

annevk avatar Mar 15 '21 07:03 annevk

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.

yutakahirano avatar Mar 15 '21 07:03 yutakahirano

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?

ricea avatar Mar 15 '21 14:03 ricea

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

annevk avatar Mar 15 '21 14:03 annevk

Can we unblock this from upload-streaming-blocking following https://github.com/whatwg/fetch/pull/1199#issuecomment-819409174 ?

yoichio avatar Apr 19 '21 06:04 yoichio

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).

annevk avatar Apr 19 '21 07:04 annevk

On a spec level, we're trying to tackle https://github.com/whatwg/streams/issues/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.

domenic avatar Apr 19 '21 15:04 domenic

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

NikoKyriakid avatar Mar 02 '22 16:03 NikoKyriakid

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 https://github.com/whatwg/streams/issues/1143.

MattiasBuelens avatar Mar 02 '22 16:03 MattiasBuelens

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

NikoKyriakid avatar Mar 02 '22 16:03 NikoKyriakid

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'})

saschanaz avatar Dec 14 '22 02:12 saschanaz

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.

ricea avatar Jan 10 '23 08:01 ricea

Given https://github.com/whatwg/fetch/issues/267#issuecomment-822559022 is the only thing that's missing here WPT tests or do we need a specification change still?

annevk avatar Jan 10 '23 09:01 annevk