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

Streams based uploading with Content-Length (not chunked encoding) #95

Closed
tyoshino opened this issue Jul 28, 2015 · 24 comments
Closed

Streams based uploading with Content-Length (not chunked encoding) #95

tyoshino opened this issue Jul 28, 2015 · 24 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api topic: streams

Comments

@tyoshino
Copy link
Member

For the first release of Streams/Fetch integration, we've decided to choose the always-chunked approach. See this minutes: https://etherpad.mozilla.org/streams-f2f-july

There're still some issues to resolve for better user experience. E.g. Domenic pointed out that it would be a pain for Amazon S3 users at https://lists.w3.org/Archives/Public/public-webapps/2014OctDec/0420.html . This issues is for reminder and place for discussion to happen in the future.

Future plan described by @domenic (moved from whatwg/streams#378)

Yes. We can do chunked for now. In the future I would like to explore allowing setting content-length (and thus not auto-setting chunked) but we can leave that for another time.

@wanderview
Copy link
Member

If we want to eventually support fixed length content for streams like this it might be nice to let the stream specialize for it. The internal source could provide a "total length" getter that the stream can then expose. So things like file streams and fixed-string streams could then expose their total length. Fetch could then just DTRT if the total length is available.

@annevk
Copy link
Member

annevk commented Feb 20, 2017

Do we really need this in a world that's moving towards H2?

@mnot
Copy link
Member

mnot commented Feb 21, 2017

Content-Length is (very) often required by servers not because they need it to delimit the body, but because they want to know how many bytes the client intends to send, so they can refuse it early, rather than suck down a few M/G and then refuse the request.

@annevk
Copy link
Member

annevk commented Feb 21, 2017

I guess then we'll hear this come up a lot when folks start using streams...

@jakearchibald
Copy link
Collaborator

If the Content-Length header is set, why do we still do chunked encoding?

@annevk
Copy link
Member

annevk commented Jul 28, 2017

@jakearchibald I'm not sure I understand the question. The setup is supposed to be such that anything but streams uses Content-Length (and no chunked) and streams uses chunked (and no Content-Length). This issue is about considering the combination of streams and Content-Length (and no chunked).

@jakearchibald
Copy link
Collaborator

I couldn't figure out why this was complicated, I realise now it's because Content-Length is a forbidden header.

@annevk
Copy link
Member

annevk commented Jul 28, 2017

The problem is that when it's set (we can make that work somehow) there's no guarantee the stream will meet the requirements. There are some solutions to that (padding 0x00 bytes, stop when you hit the limit, etc.), though it's unclear how those should interact with timeouts. Furthermore, given that H/2 doesn't have this problem at all, it's not clear to me it's worth solving unless many folks find H/1 chunked a problem and cannot migrate to H/2.

@jakearchibald
Copy link
Collaborator

Having a Content-Length allows the server to respond sooner if the size is unacceptable.

@annevk
Copy link
Member

annevk commented Jul 28, 2017

That's fair. If we really need it then we'll need to address the questions above. Perhaps if a custom Content-Type is always with a CORS preflight it's not so bad. Though we should really only allow it for streams I think. No reason to allow misrepresentation for other types of bodies.

@jakearchibald
Copy link
Collaborator

We could allow the setting of Content-Length as long as the value matches /\d+/ and value == Number(value).

I think if the stream closes before enough content is provided, we abort the request. Once the stream provides content-length bytes, the stream can be cancelled is it tries to provide more data.

@annevk
Copy link
Member

annevk commented Jul 28, 2017

Making such restrictions (or forbidding it from being set more than once) are actually somewhat tricky given that all the header APIs are generic. I wonder if a nicer alternative could be wrapping a ReadableStream somehow: { stream, length: 123 }. That way we have typed data from the start and then the fetch algorithm can just use it to set the header correctly.

@jakearchibald
Copy link
Collaborator

Allowing Content-Length to be set after a preflight seems reasonable.

@annevk
Copy link
Member

annevk commented Aug 15, 2017

And then have validation for the value and switching between that and chunked in the fetch layer somewhere? That seems rather unpredictable.

@jakearchibald
Copy link
Collaborator

Do we need validating if it's been given the all-clear via a preflight?

As for the switching, I'm used to it since nodejs does the same thing with responses. From https://nodejs.org/api/http.html#http_http_request_options_callback:

Sending a 'Content-Length' header will disable the default chunked encoding.

Of course that doesn't mean it's the right thing for fetch to do.

@annevk
Copy link
Member

annevk commented Aug 15, 2017

Do we need validating if it's been given the all-clear via a preflight?

Dunno, what's the processing model if I pass test as value or some such?

@annevk
Copy link
Member

annevk commented Aug 15, 2017

I'd also like to stress again that this seems like a lot of complexity to make H/1 scenarios work a little better. I guess it also helps with the case of letting the server reject early in H/2, but usually those kind of limits are already enforced on the client.

@mnot
Copy link
Member

mnot commented Aug 15, 2017

/me wonders if we need a new, request-specific header with the semantics "I will send at most this many bytes, but only use that to make policy decisions, not to delimit the message."

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api labels Apr 11, 2018
@annevk
Copy link
Member

annevk commented Jan 29, 2021

@yutakahirano @yoichio is this still something you want to pursue?

@yoichio
Copy link
Contributor

yoichio commented Feb 2, 2021

I don't have.
@jakearchibald, do you have a customer demand?

@matthewjumpsoffbuildings
Copy link

matthewjumpsoffbuildings commented Oct 4, 2021

Im running into this issue regarding the lack of Content-Length headers. Im using Cloudflare Workers, and the fetch() API removes the Content-Length header when using Streams, since they are returned as Transfer-Encoding: chunked. Even if I manually create the response object and attach the Content-Length header its stripped out.

The end result is a download is non-resumable, since there is no Content-Length Chrome doesnt even attempt to make it resumable, if there is a network dropout Chrome just deletes the .crdownload file that contains the bytes received so far. This is rather frustrating, since if I could send Content-Length and Accept-Range headers, I am guessing Chrome would utilise them and allow my Streamed download to be properly resumable?

Unless there are some other headers you can send when doing Transfer-Encoding: chunked to still let modern browsers understand to keep the bytes received on net dropouts, and to send a Range header to resume downloading

@basketofsoftkittens

This comment was marked as duplicate.

@annevk
Copy link
Member

annevk commented Jun 13, 2022

Closing this per the earlier discussion. Given that Content-Length is playing less of a role in newer protocol versions I don't expect us to revive this.

@annevk annevk closed this as completed Jun 13, 2022
@alanshaw
Copy link

@matthewjumpsoffbuildings and anyone else using cloudflare workers. You can use FixedLengthStream in this environment to set Content-Length.

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 needs implementer interest Moving the issue forward requires implementers to express interest topic: api topic: streams
Development

No branches or pull requests

9 participants