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

net/http: Content-Range not honored by http.Client #8923

Closed
gopherbot opened this issue Oct 12, 2014 · 12 comments
Closed

net/http: Content-Range not honored by http.Client #8923

gopherbot opened this issue Oct 12, 2014 · 12 comments
Milestone

Comments

@gopherbot
Copy link

by dahankzter:

What does 'go version' print?
go version go1.3.3 linux/amd64
and
go version devel +6d6eef8d916b Sat Oct 11 22:01:04 2014 +1100 linux/amd64

What steps reproduce the problem?
Assuming Nginx default index.html at localhost and a README.md at github (in the
playground link)

http://play.golang.org/p/u3j4oTlcm0

If possible, include a link to a program on play.golang.org.
1. Try with the url pointintg to localhost and see the body nicely truncated.
2. Switch the url to the github README.md and watch it disappear.
3.

What happened?
There is no body at all in (2) 

What should have happened instead?
I expect a body to appear with the size specified by the range 

Please provide any additional information below.
curl handles this for example in:
curl -vv --header 'Range: bytes=0-5' 'http://localhost/index.html'

The difference seems to be that the failing case does not have a Content-Length in the
response which I believe the spec does not mandate.
@ianlancetaylor
Copy link
Contributor

Comment 1:

Labels changed: added repo-main, release-none.

@gopherbot
Copy link
Author

Comment 2 by dahankzter:

I have been meaning to start contributing for quite a while.
Would this be a good candidate or is it too complex? 
I did have a look around in the http package, transport.go and response.go mostly and it
does not seem overly complex but it could be wishful thinking.

@bradfitz
Copy link
Contributor

Comment 3:

The repro program given ignores the error value, which is the most interesting part in
this bug.
More information for posterity and laziness:
Curl says:
< HTTP/1.1 206 Partial Content
< Date: Mon, 13 Oct 2014 10:14:40 GMT
* Server Apache is not blacklisted
< Server: Apache
< Access-Control-Allow-Origin: https://render.githubusercontent.com
< Content-Security-Policy: default-src 'none'
< X-XSS-Protection: 1; mode=block
< X-Frame-Options: deny
< X-Content-Type-Options: nosniff
< Strict-Transport-Security: max-age=31536000
< ETag: "272e65d6363f086441d12d519ac3ec60fbf1cda2"
< Content-Type: text/plain; charset=utf-8
< Cache-Control: max-age=300
< Accept-Ranges: bytes
< Via: 1.1 varnish
< X-Served-By: cache-lcy1122-LCY
< X-Cache: MISS
< X-Cache-Hits: 0
< Vary: Authorization,Accept-Encoding
< Expires: Mon, 13 Oct 2014 10:19:40 GMT
< Source-Age: 0
< Content-Range: bytes 0-5/1862
< Content-Length: 6
<
Go says:
$ go run client.go
Res: &http.Response{Status:"206 Partial Content", StatusCode:206, Proto:"HTTP/1.1",
ProtoMajor:1, ProtoMinor:1, Header:http.Header{"Source-Age":[]string{"0"},
"Access-Control-Allow-Origin":[]string{"https://render.githubusercontent.com"},
"Content-Security-Policy":[]string{"default-src 'none'"},
"X-Content-Type-Options":[]string{"nosniff"}, "Cache-Control":[]string{"max-age=300"},
"Accept-Ranges":[]string{"bytes"}, "Via":[]string{"1.1 varnish"},
"X-Cache-Hits":[]string{"0"}, "X-Frame-Options":[]string{"deny"},
"Etag":[]string{"\"272e65d6363f086441d12d519ac3ec60fbf1cda2\""},
"X-Served-By":[]string{"cache-lcy1129-LCY"},
"Vary":[]string{"Authorization,Accept-Encoding"}, "Expires":[]string{"Mon, 13 Oct 2014
10:22:26 GMT"}, "Date":[]string{"Mon, 13 Oct 2014 10:17:26 GMT"},
"Server":[]string{"Apache"}, "Content-Type":[]string{"text/plain; charset=utf-8"},
"X-Xss-Protection":[]string{"1; mode=block"},
"Strict-Transport-Security":[]string{"max-age=31536000"}, "X-Cache":[]string{"MISS"},
"Content-Range":[]string{"bytes 0-5/856"}}, Body:(*http.bodyEOFSignal)(0xc208144d00),
ContentLength:-1, TransferEncoding:[]string(nil), Close:false, Trailer:http.Header(nil),
Request:(*http.Request)(0xc208032820), TLS:(*tls.ConnectionState)(0xc20804fc00)}
Expires: [Mon, 13 Oct 2014 10:22:26 GMT]
X-Frame-Options: [deny]
Etag: ["272e65d6363f086441d12d519ac3ec60fbf1cda2"]
X-Served-By: [cache-lcy1129-LCY]
Vary: [Authorization,Accept-Encoding]
Date: [Mon, 13 Oct 2014 10:17:26 GMT]
Server: [Apache]
Content-Type: [text/plain; charset=utf-8]
X-Xss-Protection: [1; mode=block]
Strict-Transport-Security: [max-age=31536000]
X-Cache: [MISS]
Content-Range: [bytes 0-5/856]
Accept-Ranges: [bytes]
Via: [1.1 varnish]
X-Cache-Hits: [0]
Source-Age: [0]
Access-Control-Allow-Origin: [https://render.githubusercontent.com]
Content-Security-Policy: [default-src 'none']
X-Content-Type-Options: [nosniff]
Cache-Control: [max-age=300]
length: -1
Body: "" (0); unexpected EOF
So I think transfer.go is converting the actual "Content-Length: 6" from the server into
Content-Length: -1 (unknown) because the StatusCode (206) is unexpected.  And then the
transfer body reader is returning unexpected EOF because there's a Content-Length of -1
(which normally means chunked), but it's not in chunked encoding.
So the test and fix should be be that a response with 206 + a Content-Length keeps the
Content-Length, and then reads the body, in response_test.go.  The fix is probably in
transfer.fo.
Feel free to try to fix it soon here if you can in the next day or so, otherwise I will.

Labels changed: added release-go1.4, removed release-none.

Owner changed to @bradfitz.

Status changed to Accepted.

@gopherbot
Copy link
Author

Comment 4 by dahankzter:

Ah the error from ReadAll says that indeed, I tend to scald others for not checking.
Thanks!
Lets see what I can code up.

@gopherbot
Copy link
Author

Comment 5 by dahankzter:

It seems to come out of ReadResponse just fine and a test hopefully shows that as well
by adding another small respTest entry: http://play.golang.org/p/PWsy9tmeG8
It rhymes ok with my stepping through as well where it seems to reset the ContentLength
when considering gzip.
Just dumping out a few thoughts. Not ready to give up yet but any hints are welcome.

@gopherbot
Copy link
Author

Comment 6 by dahankzter:

I am not sure this is an error anymore... Correct me if I am wrong but if you run curl
like this:
curl -vv --header 'Range: bytes=0-5' --header 'Accept-Encoding: gzip'
'https://raw.githubusercontent.com/square/okhttp/master/README.md'
I.e. enforcing gzip there is no content in curl either. I assume it is because the
server has to zip the entire content before it sends it and that makes it impossible to
decode the little received chunk. Knock, knock.
If we get a gzip header we wrap a gzipReader innermost in the reader stack (is that the
proper term, stack?). This reader tries to automatically unzip the payload I guess and
then it errors out.
Can that be it? If so it either seems like a harder problem or quite a lot simpler. Just
skip the wrapped gzipReader and leave it to the caller to assemble and unzip the
resulting file.
Does that make sense?

@gopherbot
Copy link
Author

Comment 7 by hgarg.india:

Actually, curl -vv --header 'Range: bytes=0-5' --header 'Accept-Encoding: gzip'
'https://raw.githubusercontent.com/square/okhttp/master/README.md' is giving the raw
gzipped data.
If you will run without giving Range header, 
 curl -vv --header 'Accept-Encoding: gzip' 'https://raw.githubusercontent.com/square/okhttp/master/README.md'
then, also it will give the raw gzipped data and not print the real content.

@gopherbot
Copy link
Author

Comment 8:

CL https://golang.org/cl/155420044 mentions this issue.

@bradfitz
Copy link
Contributor

Comment 9:

This issue was closed by revision 42c3130.

Status changed to Fixed.

@gopherbot
Copy link
Author

Comment 10 by dahankzter:

Ah so unless the caller actively sets gzip the http does not implicitly set it.
Actively setting works then? What was different in the implicit case?

@gopherbot
Copy link
Author

Comment 11 by hgarg.india:

I want to know whether it will be fixed in Go1.3.3 or Go1.4.

@bradfitz
Copy link
Contributor

Comment 12:

Go 1.4.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
The http package by default adds "Accept-Encoding: gzip" to outgoing
requests, unless it's a bad idea, or the user requested otherwise.
Only when the http package adds its own implicit Accept-Encoding header
does the http package also transparently un-gzip the response.

If the user requested part of a document (e.g. bytes 40 to 50), it appears
that Github/Varnish send:
        range(gzip(content), 40, 50)

And not:
        gzip(range(content, 40, 50))

The RFC 2616 set of replacements (with the purpose of
clarifying ambiguities since 1999) has an RFC about Range
requests (http://tools.ietf.org/html/rfc7233) but does not
mention the interaction with encodings.

Regardless of whether range(gzip(content)) or gzip(range(content)) is
correct, this change prevents the Go package from asking for gzip
in requests if we're also asking for Range, avoiding the issue.
If the user cared, they can do it themselves. But Go transparently
un-gzipping a fragment of gzip is never useful.

Fixes golang#8923

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/155420044
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018
The http package by default adds "Accept-Encoding: gzip" to outgoing
requests, unless it's a bad idea, or the user requested otherwise.
Only when the http package adds its own implicit Accept-Encoding header
does the http package also transparently un-gzip the response.

If the user requested part of a document (e.g. bytes 40 to 50), it appears
that Github/Varnish send:
        range(gzip(content), 40, 50)

And not:
        gzip(range(content, 40, 50))

The RFC 2616 set of replacements (with the purpose of
clarifying ambiguities since 1999) has an RFC about Range
requests (http://tools.ietf.org/html/rfc7233) but does not
mention the interaction with encodings.

Regardless of whether range(gzip(content)) or gzip(range(content)) is
correct, this change prevents the Go package from asking for gzip
in requests if we're also asking for Range, avoiding the issue.
If the user cared, they can do it themselves. But Go transparently
un-gzipping a fragment of gzip is never useful.

Fixes golang#8923

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/155420044
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
The http package by default adds "Accept-Encoding: gzip" to outgoing
requests, unless it's a bad idea, or the user requested otherwise.
Only when the http package adds its own implicit Accept-Encoding header
does the http package also transparently un-gzip the response.

If the user requested part of a document (e.g. bytes 40 to 50), it appears
that Github/Varnish send:
        range(gzip(content), 40, 50)

And not:
        gzip(range(content, 40, 50))

The RFC 2616 set of replacements (with the purpose of
clarifying ambiguities since 1999) has an RFC about Range
requests (http://tools.ietf.org/html/rfc7233) but does not
mention the interaction with encodings.

Regardless of whether range(gzip(content)) or gzip(range(content)) is
correct, this change prevents the Go package from asking for gzip
in requests if we're also asking for Range, avoiding the issue.
If the user cared, they can do it themselves. But Go transparently
un-gzipping a fragment of gzip is never useful.

Fixes golang#8923

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/155420044
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018
The http package by default adds "Accept-Encoding: gzip" to outgoing
requests, unless it's a bad idea, or the user requested otherwise.
Only when the http package adds its own implicit Accept-Encoding header
does the http package also transparently un-gzip the response.

If the user requested part of a document (e.g. bytes 40 to 50), it appears
that Github/Varnish send:
        range(gzip(content), 40, 50)

And not:
        gzip(range(content, 40, 50))

The RFC 2616 set of replacements (with the purpose of
clarifying ambiguities since 1999) has an RFC about Range
requests (http://tools.ietf.org/html/rfc7233) but does not
mention the interaction with encodings.

Regardless of whether range(gzip(content)) or gzip(range(content)) is
correct, this change prevents the Go package from asking for gzip
in requests if we're also asking for Range, avoiding the issue.
If the user cared, they can do it themselves. But Go transparently
un-gzipping a fragment of gzip is never useful.

Fixes golang#8923

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/155420044
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants