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

Uploading a Request made from a ReadableStream body #425

Merged
merged 6 commits into from Jan 17, 2017

Conversation

@yutakahirano
Copy link
Member

commented Nov 28, 2016

This change enables developers to create a request from a ReadableStream body. fetch() reads data from the body and sends it to the server.

Fixing yutakahirano/fetch-with-streams#66.

@yutakahirano yutakahirano force-pushed the upload-stream branch from 079a0be to 82d0b86 Nov 28, 2016
Copy link
Member

left a comment

Generally this looks okay, but I'm a little concerned that the first step of https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch is not modified. It seems this should directly affect that, no?

fetch.bs Outdated
@@ -589,6 +589,9 @@ user-agent-defined <a for=header>value</a> for the

<li><p>A <dfn export for=body id=concept-body-total-bytes>total bytes</dfn> (an
integer), initially 0.

<li><p>A <dfn export for=body id=concept-body-replayable>replayable flag</dfn> (a boolean),
initially false.

This comment has been minimized.

Copy link
@annevk

annevk Nov 28, 2016

Member

I know this is annoying, and at some point we're going to burn all the flags, but for now we should keep consistency and only set and unset flags and don't say they're booleans.

This comment has been minimized.

Copy link
@annevk

annevk Nov 28, 2016

Member

Also, I think we want to call this cloneable maybe? That is what the operation we want to fail is called.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Nov 29, 2016

Author Member

Replaced boolean/true/false with (none)/set/unset.

We actually clone a body regardless of the flag. So calling it cloneable is confusing, I think.

fetch.bs Outdated
@@ -3025,6 +3028,11 @@ in addition to <a>HTTP fetch</a> above.
<a lt="include credential">includes credentials</a>, then return a
<a>network error</a>.

<li><p>If <var>request</var>'s <a for=request>method</a> is not <code>303</code>,

This comment has been minimized.

Copy link
@annevk

annevk Nov 28, 2016

Member

I think you mean to check response's status here, no?

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Nov 29, 2016

Author Member

Thanks, fixed.

fetch.bs Outdated
typedef (Blob or BufferSource or FormData or URLSearchParams or USVString) BodyInit;

typedef (BodyInit or ReadableStream) ResponseBodyInit;</pre>
typedef (Blob or BufferSource or FormData or URLSearchParams or USVString or ReadableStream) BodyInit;</pre>

This comment has been minimized.

Copy link
@annevk

annevk Nov 28, 2016

Member

I think we should continue to put USVString last.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Nov 29, 2016

Author Member

Done.

@annevk

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

Okay, so you are saying that request.clone() will continue to work. However, I don't understand why https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch should continue to clone. The whole reason we have additional checks is that we can be more conservative there.

fetch.bs Outdated
@@ -3025,6 +3027,11 @@ in addition to <a>HTTP fetch</a> above.
<a lt="include credential">includes credentials</a>, then return a
<a>network error</a>.

<li><p>If <var>actualResponse</var>'s <a for=response>status</a> is not <code>303</code>,
<var>request</var>'s <a for=request>body</a> is not null, and <var>request</var>'s

This comment has been minimized.

Copy link
@annevk

annevk Nov 29, 2016

Member

non-null

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Nov 29, 2016

Author Member

Done.

fetch.bs Outdated
@@ -3366,6 +3373,10 @@ steps:
<li class=XXX><p>Needs testing: multiple `<code>WWW-Authenticate</code>` headers, missing,
parsing issues.

<li><p>If <var>request</var>'s <a for=request>body</a> is not null and <var>request</var>'s

This comment has been minimized.

Copy link
@annevk

annevk Nov 29, 2016

Member

non-null

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Nov 29, 2016

Author Member

Done.

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2016

However, I don't understand why https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch should continue to clone. The whole reason we have additional checks is that we can be more conservative there.

As concept body has the body data as a ReadableStream, we need to clone it to replay uploading regardless of the source data type (String, ArrayBuffer, ReadableStream, ...). There are some options I could think of to handle this issue:

  1. Stop holding the body data as a ReadableStream.
  2. Define a new concept, say "replayable stream", to handle replaying
  3. Keep teeing, but adding a flag to forbid user-visible teeing to allow implementers to omit teeing.

I chose the last option as it looked the easiest.

@annevk

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

I think we should way more carefully explain that setup then, but I don't like it as much as just being explicit about it as we have done thus far. Maybe 1 is the better solution if that is what implementations actually do.

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2016

Hmm, 2ab2fce introduces the original object (source) instead of replayable flag. Does that look better to you? With the change we don't need teeing stream for body in http-network-or-cache-fetch, but apparently cloning a request is needed for headers according to the comment. Should I create clone-request-without-teeing-body?

@annevk

This comment has been minimized.

Copy link
Member

commented Dec 7, 2016

Should I create clone-request-without-teeing-body?

Yeah, I think that might be needed. There used to be some handwavy language along the lines of "clone except for body", but maybe that's gone now due to other changes.

fetch.bs Outdated
@@ -4268,6 +4301,8 @@ typedef (BodyInit or ReadableStream) ResponseBodyInit;</pre>
<p>If <var>object</var>'s {{Blob/type}}
attribute is not the empty byte sequence, set <var>Content-Type</var> to its value.

<p>Set <var>source</var> to object.

This comment has been minimized.

Copy link
@annevk

annevk Dec 7, 2016

Member

<var>object</var> here and below, no?

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 9, 2016

Author Member

Done.

fetch.bs Outdated
then return a <a>network error</a>.

<li>
<p>

This comment has been minimized.

Copy link
@annevk

annevk Dec 7, 2016

Member

This <p> seems like a typo.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 9, 2016

Author Member

Done.

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2016

Removed teeing from http-network-or-cache-fetch. PTAL again.

fetch.bs Outdated
@@ -3093,16 +3093,36 @@ steps:
<i>authentication-fetch flag</i>.

<ol>
<li><p>Let <var>httpRequest</var> be a new <a for=/>request</a>.

This comment has been minimized.

Copy link
@annevk

annevk Dec 16, 2016

Member

It seems you should just let it be null, no? Since every substep overrides what it is.

fetch.bs Outdated
"<code>error</code>", and the result of <a lt=clone for=request>cloning</a>
<var>request</var> otherwise.
<p>Otherwise, if <var>request</var>'s <a for=request>body</a> is null, then set
<var>httpRequest</var> to a copy of <var>request</var> except for its <a for=request>body</a>.

This comment has been minimized.

Copy link
@annevk

annevk Dec 16, 2016

Member

It seems you can combine this step with the next step. And only if request's body is non-null do the dance of moving the pointer across.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 16, 2016

Author Member

I thought the current structure was easier to read because it saved one nesting level. Don't you think so?

This comment has been minimized.

Copy link
@annevk

annevk Dec 16, 2016

Member

I'm not sure. The fact that the note needs to be duplicated makes me think it's not.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 19, 2016

Author Member

Done.

@annevk

This comment has been minimized.

Copy link
Member

commented Dec 16, 2016

Apologies for the delay, I've been traveling/vacationing.

Another thing I forgot to ask for is tests. We're trying to get to a place where all specification changes are accompanied by tests in w3c/web-platform-tests.

fetch.bs Outdated
nullity has already been checked.

<p class="note no-backref">The <a lt=extract for=BodyInit>extracting</a> operation cannot throw
as it was called for the same <a for=body>source</a> before.

This comment has been minimized.

Copy link
@annevk

annevk Dec 16, 2016

Member

Can you combine these two notes into one?

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 19, 2016

Author Member

Done.

fetch.bs Outdated

<li><p>Set <var>request</var>'s <a for=request>body</a> to a new <a for=/>body</a> whose
<a for=body>stream</a> is null and whose <a for=body>source</a> is <var>httpRequest</var>'s
<a for=request>body</a>'s <a for=body>source</a>.

This comment has been minimized.

Copy link
@annevk

annevk Dec 16, 2016

Member

I think it's worth pointing out here why the request isn't simply cloned.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 19, 2016

Author Member

Done.

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2016

I'll write tests soon...

fetch.bs Outdated
<p class="note no-backref">Here we do not <a for=request>clone</a> <var>request</var> in order
to reduce memory consumption. <var>request</var> can be reused with redirects, authentication,
and proxy authentication.
</ol>

This comment has been minimized.

Copy link
@annevk

annevk Dec 19, 2016

Member

This still seems wrong. Shouldn't we be setting httpRequest's body rather than that of request? Also, what happens to request's body?

The note should probably also move to after the </ol> closing tag. We copy and move body around to preserve memory consumption. And we copy in the first place since it needs to be possible to add headers without affecting request, as request is reused with redirects et al. It seems you dropped part of that note, but we should keep that.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 20, 2016

Author Member

Oops, sorry. Fixed. The intention is, passing the ReadableStream to httpRequests for sending the payload and keeping the source information in request in order to restore the payload if necessary.

fetch.bs Outdated
@@ -577,7 +577,7 @@ user-agent-defined <a for=header>value</a> for the
<ul>
<li>
<p>A <dfn export for=body id=concept-body-stream>stream</dfn> (a
{{ReadableStream}} object).
{{ReadableStream}} object or null).

This comment has been minimized.

Copy link
@annevk

annevk Dec 19, 2016

Member

Nit: this should be "null or a ReadableStream object". Maybe now is also a good time to remove the XXX paragraph that follows this? It's been long resolved per yutakahirano/fetch-with-streams#46.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Dec 20, 2016

Author Member

Done.

@guest271314

This comment has been minimized.

Copy link

commented Dec 19, 2016

Does body at body:/* ReadableStream instance */ call .getReader().read() on the value set as to body? If yes, before passing the accumulated value to fetch() call?

@annevk

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

@guest271314 I appreciate you're interested in learning how this all works, but a better place to ask questions is on IRC (#whatwg, Freenode). Pull requests are meant for reviewing changes to a standard.

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Jan 5, 2017

Can you take a look again?

@annevk

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

I didn't realize you were waiting for me, apologies. Let me clean up the PR a bit and evaluate where we stand.

Basic test: web-platform-tests/wpt#4362. More tests are expected to be written as part of the implementation effort.

Further work: #441.

Fixes #88.
@annevk annevk force-pushed the upload-stream branch from e57203c to 0cef38e Jan 5, 2017
fetch.bs Outdated

<p class="note no-backref">Here we do not <a for=request>clone</a> <var>request</var> in order
to reduce memory consumption. <var>request</var> can be reused with redirects, authentication,
and proxy authentication.

This comment has been minimized.

Copy link
@annevk

annevk Jan 5, 2017

Member

This note is still problematic I think compared to the original. I think it should say something like the following:

A request is copied as httpRequest here as we need to be able to add headers to httpRequest and read its body without affecting request. Namely, request can be reused with redirects, authentication, and proxy authentication. We copy rather than clone in order to reduce memory consumption. In case request's body's source is a ReadableStream object, redirects and authentication will end up failing the fetch.

Does that make sense to you?

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Jan 5, 2017

Author Member

I'd say "request's body's source is null" or "request's body is made from ReadableStream" insetad of "request's body's source is a ReadableStream object".

This comment has been minimized.

Copy link
@annevk

annevk Jan 5, 2017

Member

Right, yes, that makes sense.

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Jan 6, 2017

Author Member

Done.

@annevk

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Aside from that note, the other thing I'd like to understand is what the plan is surrounding the contents of these streams. When should we fail upon non-Uint8Array object usage? Is that going to be fixed as part of #441 for both requests and responses?

@davidfowl

This comment has been minimized.

Copy link

commented Jul 27, 2019

Is this feature available in any browsers as yet?

@annevk

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

We should probably remove this from the standard as it's not implemented, nobody appears to have plans, and what is in the standard today is incomplete.

@ioquatix

This comment has been minimized.

Copy link

commented Aug 6, 2019

But it’s super useful!

@davidfowl

This comment has been minimized.

Copy link

commented Aug 6, 2019

@annevk Agreed but it makes me very sad as we're extremely close to being able to potentially do full duplex streaming with the fetch APIs which would basically make it a real replacement for things like websockets (which in referred to as a legacy API in another issue).

@danielkorczak

This comment has been minimized.

Copy link

commented Aug 6, 2019

@davidfowl

So at this point, we’re waiting on HTTP/3 standards being finalized and then server implementations of HTTP/3?

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

We'll re-assess whether we (Chromium) would implement this in the near future.

@davidfowl

This comment has been minimized.

Copy link

commented Aug 7, 2019

I’ve been following up with the edge folks about helping to push this as well (since they now work on chromium).

@annevk

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@davidfowl reportedly you'll hit middle-ware snags trying to do full-duplex HTTP and therefore still need WebSocket. So as long as that's the case and as long as "Web Transport" isn't really a thing, WebSocket is here to stay.

And then while it would indeed be nice to stream request bodies, since you likely have to support resumption of some kind splitting the payload yourself and sending it as a series of requests over H/2 or H/3 might not be that much worse.

@ioquatix

This comment has been minimized.

Copy link

commented Aug 7, 2019

WebSockets are a mess to implement on the server side, HTTP streaming is semantically much simpler. So prefer this proposed approach.

What problems did you run into?

@davidfowl

This comment has been minimized.

Copy link

commented Aug 7, 2019

@davidfowl reportedly you'll hit middle-ware snags trying to do full-duplex HTTP and therefore still need WebSocket. So as long as that's the case and as long as "Web Transport" isn't really a thing, WebSocket is here to stay.

@annevk Not necessarily, using https limits what middleware can do and things like GRPC already take advantage of HTTP/2 to support streaming. That said, I don't really have an issue with websockets staying, but until the websocket over h2/h3 specs are implemented I'd like to experiment with cheaper ways to do duplex streaming (over HTTP/2, QUIC/HTTP/3 streams instead of a TCP connection). This is a path to doing so.

And then while it would indeed be nice to stream request bodies, since you likely have to support resumption of some kind splitting the payload yourself and sending it as a series of requests over H/2 or H/3 might not be that much worse.

The use case I have is for SignalR a real time framework that is transport agnostic (much like socket.io). It already runs over websockets, Server-Sent-Events (EventSource APIs in the browser) and long polling. I'd like to explore an H/2/H/3 bidirectional streaming transport which would pretty much work just like the other transports I've called out but be cheaper in terms of client and server resources.

@oliverjanik

This comment has been minimized.

Copy link

commented Aug 8, 2019

Websocket or H2/H3 ideas aside, upload progress is the last major thing missing from fetch that xhr has.

There's no way to track upload progress using fetch at the moment, which is a major blocker for adoption from what i'm seeing.

@ioquatix

This comment has been minimized.

Copy link

commented Aug 8, 2019

WebSocket over H2 already exists. There is an RFC and client/server is implemented in https://github.com/socketry/async-websocket

@annevk

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@davidfowl I thought HTTPS would do so as well, but reportedly corporate deployment of middleware is significant enough to pose a problem. This might well change over time though and I don't necessarily think it should block us from offering the ability to attempt full-duplex HTTP.

@oliverjanik see the FetchObserver idea. All it needs is time and flushing out.

@davidfowl

This comment has been minimized.

Copy link

commented Aug 8, 2019

@ioquatix do browsers support it?

@ioquatix

This comment has been minimized.

Copy link

commented Aug 8, 2019

Apparently it does but I could not get it to work in my testing.

@bradisbell

This comment has been minimized.

Copy link

commented Aug 9, 2019

In case abandoning this feature is being considered, I'd like to pile on a use case:

The existence of this API has implications for streaming media sourced from browsers and browser-like clients. Without a way to make a simple HTTP PUT request with a stream from a browser, all of us working in this space have been hacking around the problem by proxying the stream data via Web Socket servers or repeatedly making requests with chunks every second or so.

For 20 years, we have been streaming media via HTTP PUT on the source end, and HTTP GET on the receiving end. This works most everywhere, except for the source end on browsers, which is important. Implementing ReadableStream as a request body would remove the need for a lot of hackarounds.

I suspect there are many others with different use cases who are generating data on-the-fly where a normal HTTP request would be more appropriate for sending data to the server. In at least some of these use cases, we don't always have control over the servers, which already support HTTP and may not be able to support other protocols. Please, when considering this stream-as-request-body feature, keep in mind that it goes far beyond file uploads. And, thank you for your efforts!

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

I started a thread on a chromium mailing list: https://groups.google.com/a/chromium.org/forum/#!topic/blink-network-dev/bsVgOxNCzFc

You feedback will be appreciated there too.

@annevk, can we talk about this topic with other vendors at TPAC?

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

@annevk, can we talk about this topic with other vendors at TPAC?

@annevk: ^^^

@annevk

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Yeah, that seems fine to me.

@yutakahirano yutakahirano referenced this pull request Sep 12, 2019
21 of 51 tasks complete
@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Sep 12, 2019

I've proposed the topic for SW WG (w3c/ServiceWorker#1460).

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

Mozilla: Interested
Apple: Interested
Google: Interested

TODO(@yutakahirano ): Explicitly state that bi-directional streaming is not supported.

See notes for details.

@ioquatix

This comment has been minimized.

Copy link

commented Sep 16, 2019

[yutaka] we aren't trying to support bi-directional streams; we won't read response headers until we finish sending the body. This also means we won't support redirect

... why not? Bidirectional streaming is really important for so many use cases.

@davidfowl

This comment has been minimized.

Copy link

commented Sep 16, 2019

I’m glad to hear there’s an interest here, 2 streaming unidirectional requests are a big stepping stone 😁. The ability to do bidirectional steaming from a single request would avoid the need to use sticky sessions on your load balancer to make the up and down request hit the same server.

Something to think about in the future.

@yutakahirano

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2019

I understand that bi-directional streaming is important for some users, but at least for Chromium it's very difficult to implement.

@bradisbell

This comment has been minimized.

Copy link

commented Sep 16, 2019

@yutakahirano

we won't read response headers until we finish sending the body

How will the client handle cases of authentication problems? That is, if the server sends a 401 Unauthorized or some other error instead of a 100 Continue, I would expect that this response will be accessible in JavaScript in the same way that any other response is. Is that still the case?

This also means we won't support redirect

Could you elaborate on this? Suppose I'm PUT-ing a stream and server the server immediately sends a redirect upon receiving request headers. (That is, no 100 Continue, but a 307 Temporary Redirect.) I assume the stream in the request body would be closed, and the response from the Fetch would be available to JavaScript. Is this the case?

@annevk

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

No, it would network error. Exposing redirects would have to be opt-in and is tracked by #601.

@TestPolygon TestPolygon referenced this pull request Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
You can’t perform that action at this time.