http: fix two Transport gzip+persist crashes
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
diff --git a/src/pkg/http/persist.go b/src/pkg/http/persist.go
index e4eea68..a9af1f5 100644
--- a/src/pkg/http/persist.go
+++ b/src/pkg/http/persist.go
@@ -222,7 +222,6 @@
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 @@
r: r,
pipereq: make(map[*Request]uint),
writeReq: (*Request).Write,
- readRes: ReadResponse,
}
}
@@ -339,8 +337,13 @@
// 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 @@
}
}
- resp, err = cc.readRes(r, req.Method)
+ resp, err = readRes(r, req.Method)
cc.lk.Lock()
defer cc.lk.Unlock()
if err != nil {
diff --git a/src/pkg/http/transport.go b/src/pkg/http/transport.go
index a7b1b20..281ee62 100644
--- a/src/pkg/http/transport.go
+++ b/src/pkg/http/transport.go
@@ -12,6 +12,7 @@
"encoding/base64"
"fmt"
"io"
+ "io/ioutil"
"log"
"net"
"os"
@@ -285,7 +286,6 @@
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 @@
}
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 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 @@
}
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 @@
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) 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 @@
}
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()
+}
diff --git a/src/pkg/http/transport_test.go b/src/pkg/http/transport_test.go
index 7610856..3401129 100644
--- a/src/pkg/http/transport_test.go
+++ b/src/pkg/http/transport_test.go
@@ -394,6 +394,9 @@
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 @@
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
