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

Comments

Projects
None yet
4 participants
@gopherbot
Copy link

commented Oct 12, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2014

Comment 1:

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

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 12, 2014

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

This comment has been minimized.

Copy link
Member

commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Author

commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Author

commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Author

commented Oct 13, 2014

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

This comment has been minimized.

Copy link
Author

commented Oct 14, 2014

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

This comment has been minimized.

Copy link
Author

commented Oct 15, 2014

Comment 8:

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

This comment has been minimized.

Copy link
Member

commented Oct 15, 2014

Comment 9:

This issue was closed by revision 42c3130.

Status changed to Fixed.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Oct 15, 2014

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

This comment has been minimized.

Copy link
Author

commented Nov 12, 2014

Comment 11 by hgarg.india:

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

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

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 added a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018

net/http: don't send implicit gzip Accept-Encoding on Range requests
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 added a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018

net/http: don't send implicit gzip Accept-Encoding on Range requests
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 added a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018

net/http: don't send implicit gzip Accept-Encoding on Range requests
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 added a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018

net/http: don't send implicit gzip Accept-Encoding on Range requests
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.
You can’t perform that action at this time.