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

Support multiple Content-Length values #2470

Closed
ghostd opened this issue Mar 16, 2021 · 4 comments · Fixed by #2471
Closed

Support multiple Content-Length values #2470

ghostd opened this issue Mar 16, 2021 · 4 comments · Fixed by #2471
Labels
A-http1 Area: HTTP/1 specific. C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.

Comments

@ghostd
Copy link
Contributor

ghostd commented Mar 16, 2021

Hi,

I try to work on some failing tests of servo. To summarize the issue : the fetch specification has been recently updated to handle multiple values for the Content-Length header. I found Hyper client does not handle the header as expected by the specification. To be more precise, hyper does not handle the multiple values into only one header entry : Content-Length: 42,42. I don't know if we want to update hyper to follow this specification or not.

I reproduced the error with the example client and the following node http server code:

var http = require('http');

http.createServer(function (req, res) {
    res.writeHead(200, {'Content-Length': '42,42'});
    res.write('012345678901234567890123456789012345678901');
    res.end();
}).listen(8080);
@seanmonstar
Copy link
Member

I coulda sworn hyper supported this, and so dug a little. hyper does correctly support if the multiple values are on multiple lines, but not if comma-separated. So, this works:

HTTP/1.1 200 OK
content-length: 3
content-length: 3

hey

But not content-length: 3,3. Seems like something to fix, and add a test case for.

@seanmonstar seanmonstar added A-http1 Area: HTTP/1 specific. E-easy Effort: easy. A task that would be a great starting point for a new contributor. C-bug Category: bug. Something is wrong. This is bad! labels Mar 16, 2021
@seanmonstar
Copy link
Member

The relevant code is this function:

pub(super) fn content_length_parse_all_values(values: ValueIter<'_, HeaderValue>) -> Option<u64> {

@ghostd
Copy link
Contributor Author

ghostd commented Mar 16, 2021

Ok, i'll provide a PR

@shadowcat-mst
Copy link

@seanmonstar it's not particularly important now, but the original ticket did say "to be more precise, hyper does not handle the multiple values into only one header entry" - which I read as meaning that multiple header entries would work as you independently found. No harm in you having dug to confirm it, obviously, but a principle of "credit where due" meant it seemed worth pointing out that @ghostd had spotted that bit already too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http1 Area: HTTP/1 specific. C-bug Category: bug. Something is wrong. This is bad! E-easy Effort: easy. A task that would be a great starting point for a new contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants