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 #8923

LGTM=adg
R=adg
CC=golang-codereviews
https://golang.org/cl/155420044

diff --git a/src/net/http/response_test.go b/src/net/http/response_test.go
index 2dd0fad11dea1323abdf4f12cb4a67c56de92cbf..06e940d9aba0c96275988ea474b37ada98712ce4 100644 (file)
--- a/src/net/http/response_test.go
+++ b/src/net/http/response_test.go
@@ -377,6 +377,34 @@ some body`,
 
                "Body here\n",
        },
+
+       // 206 Partial Content. golang.org/issue/8923
+       {
+               "HTTP/1.1 206 Partial Content\r\n" +
+                       "Content-Type: text/plain; charset=utf-8\r\n" +
+                       "Accept-Ranges: bytes\r\n" +
+                       "Content-Range: bytes 0-5/1862\r\n" +
+                       "Content-Length: 6\r\n\r\n" +
+                       "foobar",
+
+               Response{
+                       Status:     "206 Partial Content",
+                       StatusCode: 206,
+                       Proto:      "HTTP/1.1",
+                       ProtoMajor: 1,
+                       ProtoMinor: 1,
+                       Request:    dummyReq("GET"),
+                       Header: Header{
+                               "Accept-Ranges":  []string{"bytes"},
+                               "Content-Length": []string{"6"},
+                               "Content-Type":   []string{"text/plain; charset=utf-8"},
+                               "Content-Range":  []string{"bytes 0-5/1862"},
+                       },
+                       ContentLength: 6,
+               },
+
+               "foobar",
+       },
 }
 
 func TestReadResponse(t *testing.T) {

diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 70e574fc86e24f6571b4593b9c48c4a357521c48..782f7cd395b132a96a5aa10d78e37432e95cc24e 100644 (file)
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -1040,11 +1040,14 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
        }
 
        // Ask for a compressed version if the caller didn't set their
-       // own value for Accept-Encoding. We only attempted to
+       // own value for Accept-Encoding. We only attempt to
        // uncompress the gzip stream if we were the layer that
        // requested it.
        requestedGzip := false
-       if !pc.t.DisableCompression && req.Header.Get("Accept-Encoding") == "" && req.Method != "HEAD" {
+       if !pc.t.DisableCompression &&
+               req.Header.Get("Accept-Encoding") == "" &&
+               req.Header.Get("Range") == "" &&
+               req.Method != "HEAD" {
                // Request gzip only, not deflate. Deflate is ambiguous and
                // not as universally supported anyway.
                // See: http://www.gzip.org/zlib/zlib_faq.html#faq38
@@ -1053,6 +1056,10 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
                // due to a bug in nginx:
                //   http://trac.nginx.org/nginx/ticket/358
                //   http://golang.org/issue/5522
+               //
+               // We don't request gzip if the request is for a range, since
+               // auto-decoding a portion of a gzipped document will just fail
+               // anyway. See http://golang.org/issue/8923
                requestedGzip = true
                req.extraHeaders().Set("Accept-Encoding", "gzip")
        }

diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
index 66fcc3c7d4b159be0a8617e78f4966db3194367c..defa6337082b703569659c196b235a57254a620d 100644 (file)
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -2216,6 +2216,39 @@ func TestTransportCloseIdleConnsThenReturn(t *testing.T) {
        wantIdle("after final put", 1)
 }
 
+// This tests that an client requesting a content range won't also
+// implicitly ask for gzip support. If they want that, they need to do it
+// on their own.
+// golang.org/issue/8923
+func TestTransportRangeAndGzip(t *testing.T) {
+       defer afterTest(t)
+       reqc := make(chan *Request, 1)
+       ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               reqc <- r
+       }))
+       defer ts.Close()
+
+       req, _ := NewRequest("GET", ts.URL, nil)
+       req.Header.Set("Range", "bytes=7-11")
+       res, err := DefaultClient.Do(req)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       select {
+       case r := <-reqc:
+               if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") {
+                       t.Error("Transport advertised gzip support in the Accept header")
+               }
+               if r.Header.Get("Range") == "" {
+                       t.Error("no Range in request")
+               }
+       case <-time.After(10 * time.Second):
+               t.Fatal("timeout")
+       }
+       res.Body.Close()
+}
+
 func wantBody(res *http.Response, err error, want string) error {
        if err != nil {
                return err