net/http: close Body in client code always, even on errors, and document

Fixes #6981

LGTM=rsc
R=golang-codereviews, nightlyone
CC=adg, dsymonds, golang-codereviews, rsc
https://golang.org/cl/85560045

diff --git a/src/pkg/net/http/client.go b/src/pkg/net/http/client.go
index 91d8fc8934f17ec0813f6155c17aef7897e8b99e..a5a3abe6138a596fbae22bf7e64b50651b638add 100644 (file)
--- a/src/pkg/net/http/client.go
+++ b/src/pkg/net/http/client.go
@@ -91,8 +91,9 @@ type RoundTripper interface {
        // authentication, or cookies.
        //
        // RoundTrip should not modify the request, except for
-       // consuming and closing the Body. The request's URL and
-       // Header fields are guaranteed to be initialized.
+       // consuming and closing the Body, including on errors. The
+       // request's URL and Header fields are guaranteed to be
+       // initialized.
        RoundTrip(*Request) (*Response, error)
 }
 
@@ -140,6 +141,9 @@ func (c *Client) send(req *Request) (*Response, error) {
 // (typically Transport) may not be able to re-use a persistent TCP
 // connection to the server for a subsequent "keep-alive" request.
 //
+// The request Body, if non-nil, will be closed by the underlying
+// Transport, even on errors.
+//
 // Generally Get, Post, or PostForm will be used instead of Do.
 func (c *Client) Do(req *Request) (resp *Response, err error) {
        if req.Method == "GET" || req.Method == "HEAD" {
@@ -162,14 +166,17 @@ func (c *Client) transport() RoundTripper {
 // Caller should close resp.Body when done reading from it.
 func send(req *Request, t RoundTripper) (resp *Response, err error) {
        if t == nil {
+               req.closeBody()
                return nil, errors.New("http: no Client.Transport or DefaultTransport")
        }
 
        if req.URL == nil {
+               req.closeBody()
                return nil, errors.New("http: nil Request.URL")
        }
 
        if req.RequestURI != "" {
+               req.closeBody()
                return nil, errors.New("http: Request.RequestURI can't be set in client requests.")
        }
 
@@ -277,6 +284,7 @@ func (c *Client) doFollowingRedirects(ireq *Request, shouldRedirect func(int) bo
        var via []*Request
 
        if ireq.URL == nil {
+               ireq.closeBody()
                return nil, errors.New("http: nil Request.URL")
        }
 
@@ -399,7 +407,7 @@ func Post(url string, bodyType string, body io.Reader) (resp *Response, err erro
 // Caller should close resp.Body when done reading from it.
 //
 // If the provided body is also an io.Closer, it is closed after the
-// body is successfully written to the server.
+// request.
 func (c *Client) Post(url string, bodyType string, body io.Reader) (resp *Response, err error) {
        req, err := NewRequest("POST", url, body)
        if err != nil {

diff --git a/src/pkg/net/http/request.go b/src/pkg/net/http/request.go
index feafc815772cab88aa727e11d7d70df22c1bc23c..a67092066ad2045d23ea66a44f81b3e7d3d36292 100644 (file)
--- a/src/pkg/net/http/request.go
+++ b/src/pkg/net/http/request.go
@@ -867,3 +867,9 @@ func (r *Request) wantsHttp10KeepAlive() bool {
 func (r *Request) wantsClose() bool {
        return hasToken(r.Header.get("Connection"), "close")
 }
+
+func (r *Request) closeBody() {
+       if r.Body != nil {
+               r.Body.Close()
+       }
+}

diff --git a/src/pkg/net/http/transport.go b/src/pkg/net/http/transport.go
index 75d013eac3e9e9c1d95609a6e0b91584100f3c56..2ffc40471266a83fb958b9ad6ea72b75381e2780 100644 (file)
--- a/src/pkg/net/http/transport.go
+++ b/src/pkg/net/http/transport.go
@@ -160,9 +160,11 @@ func (tr *transportRequest) extraHeaders() Header {
 // and redirects), see Get, Post, and the Client type.
 func (t *Transport) RoundTrip(req *Request) (resp *Response, err error) {
        if req.URL == nil {
+               req.closeBody()
                return nil, errors.New("http: nil Request.URL")
        }
        if req.Header == nil {
+               req.closeBody()
                return nil, errors.New("http: nil Request.Header")
        }
        if req.URL.Scheme != "http" && req.URL.Scheme != "https" {
@@ -173,16 +175,19 @@ func (t *Transport) RoundTrip(req *Request) (resp *Response, err error) {
                }
                t.altMu.RUnlock()
                if rt == nil {
+                       req.closeBody()
                        return nil, &badStringError{"unsupported protocol scheme", req.URL.Scheme}
                }
                return rt.RoundTrip(req)
        }
        if req.URL.Host == "" {
+               req.closeBody()
                return nil, errors.New("http: no Host in request URL")
        }
        treq := &transportRequest{Request: req}
        cm, err := t.connectMethodForRequest(treq)
        if err != nil {
+               req.closeBody()
                return nil, err
        }
 
@@ -193,6 +198,7 @@ func (t *Transport) RoundTrip(req *Request) (resp *Response, err error) {
        pconn, err := t.getConn(req, cm)
        if err != nil {
                t.setReqCanceler(req, nil)
+               req.closeBody()
                return nil, err
        }
 
@@ -885,6 +891,7 @@ func (pc *persistConn) writeLoop() {
                        }
                        if err != nil {
                                pc.markBroken()
+                               wr.req.Request.closeBody()
                        }
                        pc.writeErrCh <- err // to the body reader, which might recycle us
                        wr.ch <- err         // to the roundTrip function

diff --git a/src/pkg/net/http/transport_test.go b/src/pkg/net/http/transport_test.go
index 6c9711931201e1072afbf9b8557234efbac62545..de1a6e275bafe1d0f468fa7153c2a6b43739a886 100644 (file)
--- a/src/pkg/net/http/transport_test.go
+++ b/src/pkg/net/http/transport_test.go
@@ -2028,6 +2028,52 @@ func TestTransportNoReuseAfterEarlyResponse(t *testing.T) {
        }
 }
 
+type errorReader struct {
+       err error
+}
+
+func (e errorReader) Read(p []byte) (int, error) { return 0, e.err }
+
+type closerFunc func() error
+
+func (f closerFunc) Close() error { return f() }
+
+// Issue 6981
+func TestTransportClosesBodyOnError(t *testing.T) {
+       defer afterTest(t)
+       ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
+               ioutil.ReadAll(r.Body)
+       }))
+       defer ts.Close()
+       fakeErr := errors.New("fake error")
+       didClose := make(chan bool, 1)
+       req, _ := NewRequest("POST", ts.URL, struct {
+               io.Reader
+               io.Closer
+       }{
+               io.MultiReader(io.LimitReader(neverEnding('x'), 1<<20), errorReader{fakeErr}),
+               closerFunc(func() error {
+                       select {
+                       case didClose <- true:
+                       default:
+                       }
+                       return nil
+               }),
+       })
+       res, err := DefaultClient.Do(req)
+       if res != nil {
+               defer res.Body.Close()
+       }
+       if err == nil || !strings.Contains(err.Error(), fakeErr.Error()) {
+               t.Fatalf("Do error = %v; want something containing %q", fakeErr.Error())
+       }
+       select {
+       case <-didClose:
+       default:
+               t.Errorf("didn't see Body.Close")
+       }
+}
+
 func wantBody(res *http.Response, err error, want string) error {
        if err != nil {
                return err