]> Cypherpunks repositories - gostls13.git/commitdiff
http: fix two Transport gzip+persist crashes
authorBrad Fitzpatrick <bradfitz@golang.org>
Thu, 12 May 2011 02:33:15 +0000 (22:33 -0400)
committerRuss Cox <rsc@golang.org>
Thu, 12 May 2011 02:33:15 +0000 (22:33 -0400)
There were a couple issues:

-- HEAD requests were attempting to be ungzipped,
   despite having no content.  That was fixed in
   the previous patch version, but ultimately was
   fixed as a result of other refactoring:

-- persist.go's ClientConn "lastbody" field was
   remembering the wrong body, since we were
   mucking with it later. Instead, ditch
   ClientConn's readRes func field and add a new
   method passing it in, so we can use a closure
   and do all our bodyEOFSignal + gunzip stuff
   in one place, simplifying a lot of code and
   not requiring messing with ClientConn's innards.

-- closing the gzip reader didn't consume its
   contents.  if the caller wasn't done reading
   all the response body and ClientConn closed it
   (thinking it'd move past those bytes in the
   TCP stream), it actually wouldn't.  so introduce
   a new wrapper just for gzip reader to have its
   Close method do an ioutil.Discard on its body
   first, before the close.

Fixes #1725
Fixes #1804

R=rsc, eivind
CC=golang-dev
https://golang.org/cl/4523058

src/pkg/http/persist.go
src/pkg/http/transport.go
src/pkg/http/transport_test.go

index e4eea6815d0afc31b7068462038d23a0955d94de..a9af1f5516f8dd64aba65e0b3d1af941b868af78 100644 (file)
@@ -222,7 +222,6 @@ type ClientConn struct {
 
        pipe     textproto.Pipeline
        writeReq func(*Request, io.Writer) os.Error
-       readRes  func(buf *bufio.Reader, method string) (*Response, os.Error)
 }
 
 // NewClientConn returns a new ClientConn reading and writing c.  If r is not
@@ -236,7 +235,6 @@ func NewClientConn(c net.Conn, r *bufio.Reader) *ClientConn {
                r:        r,
                pipereq:  make(map[*Request]uint),
                writeReq: (*Request).Write,
-               readRes:  ReadResponse,
        }
 }
 
@@ -339,8 +337,13 @@ func (cc *ClientConn) Pending() int {
 // returned together with an ErrPersistEOF, which means that the remote
 // requested that this be the last request serviced. Read can be called
 // concurrently with Write, but not with another Read.
-func (cc *ClientConn) Read(req *Request) (resp *Response, err os.Error) {
+func (cc *ClientConn) Read(req *Request) (*Response, os.Error) {
+       return cc.readUsing(req, ReadResponse)
+}
 
+// readUsing is the implementation of Read with a replaceable
+// ReadResponse-like function, used by the Transport.
+func (cc *ClientConn) readUsing(req *Request, readRes func(buf *bufio.Reader, method string) (*Response, os.Error)) (resp *Response, err os.Error) {
        // Retrieve the pipeline ID of this request/response pair
        cc.lk.Lock()
        id, ok := cc.pipereq[req]
@@ -383,7 +386,7 @@ func (cc *ClientConn) Read(req *Request) (resp *Response, err os.Error) {
                }
        }
 
-       resp, err = cc.readRes(r, req.Method)
+       resp, err = readRes(r, req.Method)
        cc.lk.Lock()
        defer cc.lk.Unlock()
        if err != nil {
index a7b1b20e6305491b579456fc536a806de3e1bea0..281ee62926f1cea96bd1d7a3ce42c3c41e18ac83 100644 (file)
@@ -12,6 +12,7 @@ import (
        "encoding/base64"
        "fmt"
        "io"
+       "io/ioutil"
        "log"
        "net"
        "os"
@@ -285,7 +286,6 @@ func (t *Transport) getConn(cm *connectMethod) (*persistConn, os.Error) {
 
        pconn.br = bufio.NewReader(pconn.conn)
        pconn.cc = newClientConnFunc(conn, pconn.br)
-       pconn.cc.readRes = readResponseWithEOFSignal
        go pconn.readLoop()
        return pconn, nil
 }
@@ -447,7 +447,25 @@ func (pc *persistConn) readLoop() {
                }
 
                rc := <-pc.reqch
-               resp, err := pc.cc.Read(rc.req)
+               resp, err := pc.cc.readUsing(rc.req, func(buf *bufio.Reader, reqMethod string) (*Response, os.Error) {
+                       resp, err := ReadResponse(buf, reqMethod)
+                       if err != nil || resp.ContentLength == 0 {
+                               return resp, err
+                       }
+                       if rc.addedGzip && resp.Header.Get("Content-Encoding") == "gzip" {
+                               resp.Header.Del("Content-Encoding")
+                               resp.Header.Del("Content-Length")
+                               resp.ContentLength = -1
+                               gzReader, err := gzip.NewReader(resp.Body)
+                               if err != nil {
+                                       pc.close()
+                                       return nil, err
+                               }
+                               resp.Body = &readFirstCloseBoth{&discardOnCloseReadCloser{gzReader}, resp.Body}
+                       }
+                       resp.Body = &bodyEOFSignal{body: resp.Body}
+                       return resp, err
+               })
 
                if err == ErrPersistEOF {
                        // Succeeded, but we can't send any more
@@ -502,6 +520,11 @@ type responseAndError struct {
 type requestAndChan struct {
        req *Request
        ch  chan responseAndError
+
+       // did the Transport (as opposed to the client code) add an
+       // Accept-Encoding gzip header? only if it we set it do
+       // we transparently decode the gzip.
+       addedGzip bool
 }
 
 func (pc *persistConn) roundTrip(req *Request) (resp *Response, err os.Error) {
@@ -533,25 +556,12 @@ func (pc *persistConn) roundTrip(req *Request) (resp *Response, err os.Error) {
        }
 
        ch := make(chan responseAndError, 1)
-       pc.reqch <- requestAndChan{req, ch}
+       pc.reqch <- requestAndChan{req, ch, requestedGzip}
        re := <-ch
        pc.lk.Lock()
        pc.numExpectedResponses--
        pc.lk.Unlock()
 
-       if re.err == nil && requestedGzip && re.res.Header.Get("Content-Encoding") == "gzip" {
-               re.res.Header.Del("Content-Encoding")
-               re.res.Header.Del("Content-Length")
-               re.res.ContentLength = -1
-               esb := re.res.Body.(*bodyEOFSignal)
-               gzReader, err := gzip.NewReader(esb.body)
-               if err != nil {
-                       pc.close()
-                       return nil, err
-               }
-               esb.body = &readFirstCloseBoth{gzReader, esb.body}
-       }
-
        return re.res, re.err
 }
 
@@ -583,16 +593,6 @@ func responseIsKeepAlive(res *Response) bool {
        return false
 }
 
-// readResponseWithEOFSignal is a wrapper around ReadResponse that replaces
-// the response body with a bodyEOFSignal-wrapped version.
-func readResponseWithEOFSignal(r *bufio.Reader, requestMethod string) (resp *Response, err os.Error) {
-       resp, err = ReadResponse(r, requestMethod)
-       if err == nil && resp.ContentLength != 0 {
-               resp.Body = &bodyEOFSignal{body: resp.Body}
-       }
-       return
-}
-
 // bodyEOFSignal wraps a ReadCloser but runs fn (if non-nil) at most
 // once, right before the final Read() or Close() call returns, but after
 // EOF has been seen.
@@ -615,6 +615,9 @@ func (es *bodyEOFSignal) Read(p []byte) (n int, err os.Error) {
 }
 
 func (es *bodyEOFSignal) Close() (err os.Error) {
+       if es.isClosed {
+               return nil
+       }
        es.isClosed = true
        err = es.body.Close()
        if err == nil && es.fn != nil {
@@ -639,3 +642,13 @@ func (r *readFirstCloseBoth) Close() os.Error {
        }
        return nil
 }
+
+// discardOnCloseReadCloser consumes all its input on Close.
+type discardOnCloseReadCloser struct {
+       io.ReadCloser
+}
+
+func (d *discardOnCloseReadCloser) Close() os.Error {
+       io.Copy(ioutil.Discard, d.ReadCloser) // ignore errors; likely invalid or already closed
+       return d.ReadCloser.Close()
+}
index 7610856738de6349b4e34b046f03ea2c0340a1ee..34011293fd2535133f18868118f07b5409a4c4d7 100644 (file)
@@ -394,6 +394,9 @@ func TestTransportGzip(t *testing.T) {
                        t.Errorf("Accept-Encoding = %q, want %q", g, e)
                }
                rw.Header().Set("Content-Encoding", "gzip")
+               if req.Method == "HEAD" {
+                       return
+               }
 
                var w io.Writer = rw
                var buf bytes.Buffer
@@ -463,6 +466,16 @@ func TestTransportGzip(t *testing.T) {
                        t.Errorf("expected Read error after Close; got %d, %v", n, err)
                }
        }
+
+       // And a HEAD request too, because they're always weird.
+       c := &Client{Transport: &Transport{}}
+       res, err := c.Head(ts.URL)
+       if err != nil {
+               t.Fatalf("Head: %v", err)
+       }
+       if res.StatusCode != 200 {
+               t.Errorf("Head status=%d; want=200", res.StatusCode)
+       }
 }
 
 // TestTransportGzipRecursive sends a gzip quine and checks that the