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

Streaming upload #1438

Closed
yutakahirano opened this issue May 11, 2022 · 76 comments
Closed

Streaming upload #1438

yutakahirano opened this issue May 11, 2022 · 76 comments

Comments

@yutakahirano
Copy link
Member

Past discussions: #425, #966, ...

Please also see: other upload-streaming-blocking issues.

Let's discuss whether we want to have the streaming upload feature, i.e., a way to attach a ReadableStream to a Request, on the Web.
Last time we discussed, all the browser vendors agreed that supporting full-duplex (c.f., #1254) would be very difficult, so I'd like to exclude the option from this discussion. Feel free discuss that at #1254 or another issue.

We, Chrome, have a partial implementation, but failed to show its effectiveness thourgh a past experiment.

We have two paths forward:

  1. Get more support from other browser vendors and web developers, and ship this feature.
  2. Give up this feature and remove the logic from the spec.

Thoughts?

@yutakahirano
Copy link
Member Author

(Feel free to restate your position here even if you've already done that in #425).

@annevk annevk added the removal/deprecation Removing or deprecating a feature label May 11, 2022
@drzraf
Copy link

drzraf commented May 12, 2022

It's a great opportunity to make the web a bit more bidirectional.

ReadableStream means on-the-fly transformation, the first obvious candidate being... encryption.

Use case: I want to submit a form (to an openstack POST-handler) which contains attachments encrypted on-the-fly. In the current situation, client memory is ~ twice the file size what is unsuitable for multi-GB uploads.

The lack of ReadableStream upload incentives the use of chunking as an alternative (which has the additional benefit of being easily resumable).
Since I believe streams find their use for large data (unpredictable size), reliability (resumable/error-handing), if not directly involved, needs to at least be considered by implementations.

@benbucksch
Copy link

benbucksch commented May 27, 2022

I'd like to echo #425 (comment) . This feature is desperately needed for streaming media from a client to a server.

The alternatives like WebSockets etc. all need manual, custom code on both the client and server side, which makes interoperability practically impossible. That's more like FTP. The beauty of HTTP is its simplicity.

We can stream from server to client in response bodies, so we should also be able to stream from client to server in request bodies. In the same way that I can stream a video from server to client, I should be able to stream a video or audio from client to server. This avoids having to create custom protocols and servers and clients for streaming media.

@benbucksch
Copy link

One important project of mine died exactly at this point, because this wasn't possible.

@ioquatix
Copy link

ioquatix commented May 29, 2022

From an implementation point of view, HTTP is pretty elegant, at least, HTTP/1 is. HTTP/2 trades questionable performance gains for elegance, and HTTP/3 goes further in this direction without any real semantic advantages. Honestly, I'm so disappointed that we are reinventing the network protocol at the application layer, but that's probably for another discussion.

Browsers need to implement the full semantics of HTTP. Even since HTTP/1.1 full bi-directional streaming is possible with chunked encoding. Of course you can argue that it's not used in the real world, but it's a true chicken and egg problem, of course if browsers don't support it no one can use it.

From an implementation perspective, I can only imagine that browsers put "full semantics of HTTP" in the too hard basket and instead invent things like WebSockets and WebTransport, WebRTC which are all ridiculously complicated to implement correctly on the client and server, have various compatibility issues and honestly don't solve the problem as well as bi-directional HTTP would.

I respect that I probably don't see the full picture from the perspective of a browser developer, but I have implemented a full HTTP/1 stack in C++ and HTTP/1 + HTTP/2 in Ruby (and hopefully soon 3), both client and server, including on top of that client and server WebSockets, including on top of that, interfaces for web applications to build on. So, I think I do have a pretty good perspective when I say, I wish browsers would just fully commit to bi-directional HTTP vs all these extra layers on top. Not only is it hard for developers, it's hard to communicate the reason why it's built like this, because frankly, the reasons aren't very good.

@yutakahirano
Copy link
Member Author

Thank you for your feedback.

I'm happy to make some more efforts, meaning I'm happy to make another try to spec and implement the minimal viable ship.

If you are happy with them, I'll fix the spec and implementation, file a TAG review and send an intent to ship to blink-dev. With your support ("developer's opinions"), I may be able to get enough support from API owners.

@ioquatix
Copy link

ioquatix commented May 31, 2022

If this is the best middle ground we can find to start with, I support it. However, I'd also say, full-duplex streaming and HTTP/1 streaming are definitely possible in practice, and the only limitation seems to be browser implementors. Because, I've personally implemented all of that already.

It might be useful to allow specs to be full-duplex and to support HTTP/1.1 as optional, e.g. node.js might choose to support this because they don't depend on a browser for the client/server implementation limitations.

@benbucksch
Copy link

benbucksch commented May 31, 2022

@yutakahirano

HTTP/1.1 support is essential for me.

I need this as internal application API between different components of a distributed system. Both end points are usually in the same internal local network, so there's typically no need for SSL encryption, and there will not be valid CA certificates. The clients may be either a) node.js, or b) small devices with an ESP32 using HTTP/1.1, or c) a webpage in a browser on the open web using HTTP/2, or a wild combination of all of those. Custom protocols based on WebSockets, WebRTC or other layers which require complicated server and client implementations are out of question. The entire point of using HTTP APIs is to create a standard for interoperable devices, with many different implementations, and making it really easy to implement new clients of various kinds, within hours of dev time.

I was really surprised to find out that streaming upload for fetch() is not implemented. I took it for granted that if I can download and upload files, and a stream can be returned from server to client, a stream can also be sent from client to server. There are many applications: Essentially all the use cases for server->client streaming, just that the connection is established by the other end.

Such holes in the specs need to be avoided, where it works in some cases but not in others, e.g. file download works, file upload works, stream download works, but stream upload doesn't work. The elegance of HTTP is that there is a simple concept, and you can cover most use cases with this same concept. This is unlike FTP, and many other more complicated protocols with complex layers and multiple channels that were all superceded by HTTP, due to the power of its simplicity.

@benbucksch
Copy link

#425 discussed a number of approaches for HTTP/1.1. A stream could be terminated by dropping the connection, or it could use chunking. My preference would be that both methods work.

@wenbozhu
Copy link

I worked with the Chrome team on the experiment mentioned by Yutaka, with Google's user-facing services (gmail etc).

Unfortunately the API designed for the Origin Trial didn't allow us to collect the data needed, i.e. % of broken H1 streams due to chunked TE. The main reason is that HTTPS traffic default to H2+.

To make the experiment work, we need an Origin Trial API that forces HTTP/1.1 over HTTPS.

@yutakahirano
Copy link
Member Author

yutakahirano commented Jun 1, 2022

Let me state our position more clearly.

We, Chrome Web Networking team, have limited bandwith available on this feature. We are happy to re-try shipping the MVP part (no HTTP/1.1, no full-duplex) ([A]), but we don't have bandwith to drive and resolve the HTTP/1.1 or full-duplex discussions.

So, I want to know If you want to use the MVP part. At this moment @drzraf (comment) and @ioquatix (comment) do, right?

I understand that some of you want more, and don't want to use streaming upload if it doesn't support HTTP/1.1 and/or full-duplex. Unfortunally, we (again, Chrome) will not have enough bandwith to drive discussions to resolve these problems in the near future.

If you want to drive such discussions instead of us, that will be greatly appreciated. We don't want to block you at all ([B]).

If few people want the MVP part, and no one wants to drive the blocking discussions, then I think we should remove the feature from the spec ([C]).

So, in addition to the two options I originally described ([A] and [C]), there is a way to support HTTP/1.1 and/or full-duplex ([B]), but Chrome doesn't have bandwidth to pursue this option, and we need another owner to pursue the option.

@ioquatix
Copy link

ioquatix commented Jun 1, 2022

I'm fine with [A] as long as it doesn't prevent [B] in the future. However I'd be literally unable to use it to replace WebSockets without full-duplex support, which is what I'd like to do. HTTP/1.1 is less of an issue except that there is no reason why it can't be supported in theory (as has been demonstrated in non-browser environments). I'd rather make some progress in the right direction than none at all.

@martin-traverse
Copy link

I agree with @ioquatix - I'd been keen to have access to the feature at least in HTTP/2. I already have fallback functionality, but it would be great to use streaming upload in environments where it is available. My application is deployed internally for businessness, so we can know for the software environment of a particular business that the feature is always available (or always not available).

@yutakahirano
Copy link
Member Author

Let me confirm. @ioquatix, it sounds like you're not planning to use the feature unless the full-duplex communication is implemented. Is that correct?

@campersau
Copy link

This would also help in some WebAssembly scenarios as other languages support streaming uploads via Streams / Pipes or other concepts. For example .NET is waiting for it: dotnet/runtime#36634

@yutakahirano
Copy link
Member Author

Thank you @campersau!
Let me confirm: do you think the feature would be useful even without HTTP/1.1 support and full-duplex communication?

@campersau
Copy link

@yutakahirano It wouldn't be as useful as with HTTP/1.1 be but still an improvement. Full-duplex communication isn't needed, at least for .NET.

@benbucksch
Copy link

FWIW, none of the use cases I have in mind would need full-duplex streams. All of the cases I encountered were either sending or receiving streams, but never both in both directions at the same time in the same request.

@ioquatix
Copy link

ioquatix commented Jun 9, 2022

@yutakahirano I want full duplex streaming but I won't let that get in the way of progress towards that goal. Since I basically want to replace WebSockets with something simpler/better.

@yutakahirano
Copy link
Member Author

yutakahirano commented Jun 9, 2022

@drzraf (comment), @martin-traverse(comment) and @campersau(comment) have shown their support.

@annevk, I'd like to try shipping the MVP, given the developers' support.
Steps:

  1. Land Disallow streaming upload on HTTP/1.1 connections #1444.
  2. Remove "upload-stream-blocking" from Request body streams should use chunked encoding #966 and Fetch body streams are not full duplex #1254. I'm fine with keeping them open, given there are people interested in the features.
  3. File a TAG review.
  4. Request positions to Mozilla and Apple.
  5. (I need to fix our implementation.)
  6. Send an intent to ship to blink-dev.

Are you fine with these?

@benbucksch
Copy link

@yutakahirano If you choose to not implement streaming upload for HTTP/1.1 in Chrome, that's an engineering resources choice. However, I do not see a reason to explicitly forbid it in the spec. The spec applies to all of Chromium, Firefox, Apple WebKit, and node.js fetch(). In other words, is there a reason why node.js should be explicitly forbidden to implement streaming upload in fetch with HTTP/1.1 , if they want to implement it?

@yutakahirano
Copy link
Member Author

@benbucksch The reason why we don't support HTTP/1.1 is not engineering resource, but a security concern. See this comment for example. If you want to allow HTTP/1.1, that's great, I'll ask you to drive the discussion at #966. I don't want #966 to block the MVP part, and I'm proposing blocking it for now, because blocking a feature now and allowing it later is easier than allowing it now and blocking it later.

@benbucksch
Copy link

The comment you cite does not mention security concerns. Are you saying that you want to explicitly support SSL-breaking man-in-the-middle boxen, and would like to remove a feature from HTTP, because such encryption-breaking boxes might not explicitly support it? Or am I misunderstanding you?

The spec currently does allow HTTP/1.1. Now, PR #1444 is proposing to explicitly forbid it. That's new. I do not see a good reason to forbid it and to stop others from implementing it.

Instead, could you change the spec to more clearly state how you intend to support this feature with HTTP/2, allowing you to implement what you call "MVP", but could you leave the HTTP/1.1 part of the spec simply as-is, allowing others like Firefox or node.js to implement it for HTTP/1.1 as well?

@yutakahirano
Copy link
Member Author

yutakahirano commented Jun 9, 2022

The #966 (comment) does not mention security concerns. Are you saying that you want to explicitly support SSL-breaking man-in-the-middle boxen, and would like to remove a feature from HTTP, because such encryption-breaking boxes might not explicitly support it? Or am I misunderstanding you?

Maybe this comment is clearer? "Security issue" may be a wrong term, but I think it's clear there is an issue.

The spec currently does allow HTTP/1.1. Now, PR #1444 is proposing to explicitly forbid it. That's new. I do not see a good reason to forbid it and to stop others from implementing it.

Yes, and concerns have been raised from multiple people at #966. We need to address the concerns before shipping this feature. The easiest way is blocking HTTP/1.1, and that's what I'm doing.

Instead, could you change the spec to more clearly state how you intend to support this feature with HTTP/2, allowing you to implement what you call "MVP", but could you leave the HTTP/1.1 part of the spec simply as-is, allowing others like Firefox or node.js to implement it for HTTP/1.1 as well?

That's explicitly opposed in #966.

I'd be opposed to leaving the decision whether to support chunked encoding up to implementers.
#966 (comment)

Suggestion
4. Require implementors to minimally support the above (i.e they MUST attempt to support chunked encoding if the above invariants hold)
#966 (comment)

I recommend you to discuss at #966.

I'm proposing to block HTTP/1.1 for now. Blocking it for now doesn't mean blocking it forever. When you get an agreement at #966, we'll be able to allow it.

@benbucksch
Copy link

benbucksch commented Jun 21, 2022

@yutakahirano mentioned:

TPAC 2019

Reading the protocol (search for "fetch#425"), I see you discussed the authentication error case that I mentioned here as well: "
[annevk] same for authentication challenge? other things that would cause multiple requests?
[yutaka]: ok, I'm fine to implement in this case
"
I'm not sure I understand your response "ok to implement this case" correctly. Were you saying that you would ensure that a 401 is handled and passed on immediately? As a special case?

In your API proposal, how exactly would that look like for a caller? I start a live streaming upload, e.g. a meeting recording, or a security camera feed upload, and the server says 401. If I receive the 401 error only after the stream ends, we have data loss.

In other words, whatever the API, the browser cannot sit on server errors, but needs to report them back as soon as the server sends them. This is true even for non-live streams. Streams are unnecessary for a few KB, but they can be very long, even hours of upload. To ignore errors and pass them on only after the stream ends, is way too late, causes useless re-transmisssion of large files at best, or even data loss. I cannot see any other solution than what I proposed. Or maybe you have something in mind how to solve this?

@yutakahirano
Copy link
Member Author

all I'm suggesting is to not treat step 6 differently than step 7, and to not change the flow in some cases and not others, but to do for step 6 the same as for step 7, which also matches the flow for downstream. Is that a possible compromise?

I don't understand your proposal. Step 6 and step 7 are different option values representing different behaviors.

I'm not sure I understand your response "ok to implement this case" correctly. Were you saying that you would ensure that a 401 is handled and passed on immediately? As a special case?

I said I was happy to cancel the session when we see 401 (as well as redirects).
That's actually specced now.

https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

  1. If response’s status is 401, httpRequest’s response tainting is not "cors", includeCredentials is true, and request’s window is an environment settings object, then:
    1. ...
    2. If request’s body is non-null, then:
      1. If request’s body’s source is null, then return a network error.

Working with redirects / authentication requests is difficult because for non-streaming requests we repeat the request body. We don't want to store the entire stream for potential redirects / authentication requests, so we decided to cancel the requests in such cases. Of course, it is open to extention, so if you want to add an alternative behavior, please feel free to file an issue.

In other words, whatever the API, the browser cannot sit on server errors, but needs to report them back as soon as the server sends them. This is true even for non-live streams. Streams are unnecessary for a few KB, but they can be very long, even hours of upload. To ignore errors and pass them on only after the stream ends, is way too late, causes useless re-transmisssion of large files at best, or even data loss. I cannot see any other solution than what I proposed. Or maybe you have something in mind how to solve this?

This is true, but this is generally true for requests with big bodies. The server can send a response and reset the stream when it thinks it doesn't need furthur request body.

@benbucksch
Copy link

benbucksch commented Jun 21, 2022

I don't understand your proposal.
Step 6 and step 7 are different option values representing different behaviors.

Yes, I am proposing to make the behavior consistent for all cases. That makes it more consistent for developers, and clearer for error cases, and for later full-duplex use. I.e. in both full and half duplex:
"Step 6: The Promise returned from fetch to resolves immediately when the response headers have been received, regardless of whether the request body has been sent."

It is my understanding that you rejected the idea, although would solve the error case as well.

cancel the session when we see 401 (as well as redirects).
then return a network error.

I see. Thanks. How would the caller be able to distinguish between a connection error, 401 auth error, and 302 redirect? Because the recovery action in each case needs to be different (e.g. get new OAuth2 token), so the caller needs to know which case happened. Unfortunately, the "network error" spec specifies that the status message must be empty, so I wouldn't know how to get that information. How did you envision that?

@annevk
Copy link
Member

annevk commented Jun 21, 2022

@benbucksch returning the response before the data has been transmitted is full duplex. You might not care for response body streaming, but that doesn't change the fact that returning the response while the request is still in progress is full duplex behavior and not something that will happen at this point in browsers.

@ioquatix
Copy link

ioquatix commented Jun 21, 2022

@benbucksch I feel your pain and as it stands half duplex behaviour as proposed might not be very useful in some cases. But it opens a path forward to full duplex and I'm certainly positive on implementing something which opens the door on a future full duplex implementation. It also sounds like in the error case, the streaming upload would be cancelled. I would argue we should drop this behaviour if/when we implement full duplex streaming, as I don't believe a 4xx should mean "stop streaming the body". Assuming I'm understanding the proposed spec.

@yutakahirano
Copy link
Member Author

It is my understanding that you rejected the idea, although would solve the error case as well.

@benbucksch Yeah, as I said before, what you want is what we call full-duplex in this issue. Removing the "half-duplex" behavior is unacceptable to me.

@benbucksch
Copy link

benbucksch commented Jun 22, 2022

@annevk: I understand and respect that. I below, I assume that it's implemented as @yutakahirano suggests.

However, the question above remains about server errors which happen early on, e.g. authentication. I am still not clear how a web app would know that it needs to re-authenticate.

@yutakahirano specced:

cancel the session when we see 401 (as well as redirects).
then return a network error.

How would the caller be able to distinguish between a connection error, 401 auth error, and 302 redirect? Unfortunately, the "network error" spec specifies that the status message must be empty, so I wouldn't know how to get that information. The recovery action in each case needs to be different (e.g. get new OAuth2 token), so the caller needs to know which kind of error happened. How can the caller know which error it was?

One option would be to create a new "server error" (instead of "network error"), where the status message and code fields are populated. Or you could allow "network error" to have an error message and code. Or maybe you or somebody have a better idea.

@yutakahirano
Copy link
Member Author

We discussed error on redirects at #538. See this comment and below. @annevk was reluctant to reveal the information.

Redirects and server-initiated authentication prompts. And the rejection is with the normal TypeError with perhaps a console-only message of the reason (which is "cannot be replayed" btw). We should not expose the failure reason to web content.

#538 (comment)

@yutakahirano
Copy link
Member Author

@ioquatix

I can't imagine there is anything other than half and full duplex. To avoid using a string for such a case, why not just use a boolean toggle, e.g. fullDuplex: true or bidirectional: true or streaming: true?

I weakly prefer duplex: 'full' | 'half' because it's symmetric. IIUC, when we decided to delay the decision on the default value, we chose the option to be symmetric.

For example, the absense of fullDuplex option sounds closer to fullDuplex: false than fullDuplex: true. I'd like to avoid that situation given we don't know what the absense ends up meaning.

@hawkinsw
Copy link

I hope that I am not doing the "useless" "Me Too" here, but I wanted to chime and say how important having this functionality would be. We are building a tool in Go that we will deploy via wasm and their runtime does not support streaming POST:

https://go.googlesource.com/go/+/refs/heads/master/src/net/http/roundtrip_js.go#96

Using an older version of Chromium from during the Origin Trial I was able to add support to Go's wasm runtime and everything worked great. It would be wonderful to have this support.

In case you are wondering, our project is: https://github.com/network-quality/goresponsiveness

Thank you all for the tremendous effort you are dedicating to this feature!

@annevk
Copy link
Member

annevk commented Jun 28, 2022

@benbucksch essentially you would not use this with an arbitrary endpoint. For that you would want full duplex. And even then it might end up being problematic if the server or intermediary (which could only be used by X% of your end users) does not support full duplex. It's not clear to me that could ever work well.

@lmcarreiro
Copy link

Not sure if a lot of people use this pattern, but our app was using this code to check if streaming upload is enabled, and it broke for users using browsers based on 105 beta of chromium:

  public static doesSupportStreamBody (): boolean {
    // This works because the browser adds a Content-Type header of text/plain;charset=UTF-8 to the request 
    // if the body is text. 
    // The browser only treats the body as text if it doesn't support request streams, 
    // otherwise it won't add a Content-Type header at all.
    return ReadableStream && !new Request('', {
        body: new ReadableStream(),
        method: 'POST',
      }).headers.has('Content-Type')
  }

We had to add this to fix it:

        // @ts-expect-error
        duplex: 'half',

@hawkinsw
Copy link

Not sure if a lot of people use this pattern, but our app was using this code to check if streaming upload is enabled, and it broke for users using browsers based on 105 beta of chromium:

  public static doesSupportStreamBody (): boolean {
    // This works because the browser adds a Content-Type header of text/plain;charset=UTF-8 to the request 
    // if the body is text. 
    // The browser only treats the body as text if it doesn't support request streams, 
    // otherwise it won't add a Content-Type header at all.
    return ReadableStream && !new Request('', {
        body: new ReadableStream(),
        method: 'POST',
      }).headers.has('Content-Type')
  }

We had to add this to fix it:

        // @ts-expect-error
        duplex: 'half',

Thank you for noting this. It appears that this requirement is documented in the fetch standard but nowhere else. I am planning on submitting a few pull requests for MDN to add something.

Thoughts?

@jimchou-dev
Copy link

jimchou-dev commented Sep 11, 2023

It seems like it's also not supported on Safari and getting the error "Error sending request: ReadableStream uploading is not supported".

And this check against safari does not work

Not sure if a lot of people use this pattern, but our app was using this code to check if streaming upload is enabled, and it broke for users using browsers based on 105 beta of chromium:

  public static doesSupportStreamBody (): boolean {
    // This works because the browser adds a Content-Type header of text/plain;charset=UTF-8 to the request 
    // if the body is text. 
    // The browser only treats the body as text if it doesn't support request streams, 
    // otherwise it won't add a Content-Type header at all.
    return ReadableStream && !new Request('', {
        body: new ReadableStream(),
        method: 'POST',
      }).headers.has('Content-Type')
  }

We had to add this to fix it:

        // @ts-expect-error
        duplex: 'half',

Use the snippet here instead WebKit/standards-positions#24 (comment)

@sumitsahoo
Copy link

Looks like it isn't supported in Firefox, if anyone tried to implement any workaround in Firefox please let me know. Here is the Firefox bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1387483

My use case is also similar as we encrypt then upload and loading the entire file in memory is not an option (also not optimal)

@stalkerg

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests