From a8d90ec3506142b8cc2400cbfcde2acfa834062a Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 14 Apr 2014 08:06:13 -0700 Subject: [PATCH] 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 --- src/pkg/net/http/client.go | 14 +++++++-- src/pkg/net/http/request.go | 6 ++++ src/pkg/net/http/transport.go | 7 +++++ src/pkg/net/http/transport_test.go | 46 ++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 3 deletions(-) diff --git a/src/pkg/net/http/client.go b/src/pkg/net/http/client.go index 91d8fc8934..a5a3abe613 100644 --- 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 feafc81577..a67092066a 100644 --- 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 75d013eac3..2ffc404712 100644 --- 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 6c97119312..de1a6e275b 100644 --- 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 -- 2.50.0