]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: clean up the Client redirect code, document Body.Close rules more
authorBrad Fitzpatrick <bradfitz@golang.org>
Thu, 31 Mar 2016 12:36:20 +0000 (05:36 -0700)
committerBrad Fitzpatrick <bradfitz@golang.org>
Fri, 1 Apr 2016 00:48:38 +0000 (00:48 +0000)
Issue #8633 (and #9134) noted that we didn't document the rules about
closing the Response.Body when Client.Do returned both a non-nil
*Response and a non-nil error (which can only happen when the user's
CheckRedirect returns an error).

In the process of investigating, I cleaned this code up a bunch, but
no user-visible behavior should have changed, except perhaps some
better error messages in some cases.

It turns out it's always been the case that when a CheckRedirect error
occurs, the Response.Body is already closed. Document that.

And the new code makes that more obvious too.

Fixes #8633

Change-Id: Ibc40cc786ad7fc4e0cf470d66bb559c3b931684d
Reviewed-on: https://go-review.googlesource.com/21364
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>
src/net/http/client.go

index e2b82705ebca0e1d793b278125589c1b2db184bb..10f5684a797addf1edecba0bcb97929ea1aa83d3 100644 (file)
@@ -44,9 +44,9 @@ type Client struct {
        // following an HTTP redirect. The arguments req and via are
        // the upcoming request and the requests made already, oldest
        // first. If CheckRedirect returns an error, the Client's Get
-       // method returns both the previous Response and
-       // CheckRedirect's error (wrapped in a url.Error) instead of
-       // issuing the Request req.
+       // method returns both the previous Response (with its Body
+       // closed) and CheckRedirect's error (wrapped in a url.Error)
+       // instead of issuing the Request req.
        //
        // If CheckRedirect is nil, the Client uses its default policy,
        // which is to stop after 10 consecutive requests.
@@ -153,28 +153,33 @@ func (c *Client) send(req *Request, deadline time.Time) (*Response, error) {
                        c.Jar.SetCookies(req.URL, rc)
                }
        }
-       return resp, err
+       return resp, nil
 }
 
 // Do sends an HTTP request and returns an HTTP response, following
-// policy (e.g. redirects, cookies, auth) as configured on the client.
+// policy (such as redirects, cookies, auth) as configured on the
+// client.
 //
 // An error is returned if caused by client policy (such as
-// CheckRedirect), or if there was an HTTP protocol error.
-// A non-2xx response doesn't cause an error.
-//
-// When err is nil, resp always contains a non-nil resp.Body.
+// CheckRedirect), or failure to speak HTTP (such as a network
+// connectivity problem). A non-2xx status code doesn't cause an
+// error.
 //
-// Callers should close resp.Body when done reading from it. If
-// resp.Body is not closed, the Client's underlying RoundTripper
-// (typically Transport) may not be able to re-use a persistent TCP
-// connection to the server for a subsequent "keep-alive" request.
+// If the returned error is nil, the Response will contain a non-nil
+// Body which the user is expected to close. If the Body is not
+// closed, the Client's underlying RoundTripper (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.
 //
+// On error, any Response can be ignored. A non-nil Response with a
+// non-nil error only occurs when CheckRedirect fails, and even then
+// the returned Response.Body is already closed.
+//
 // Generally Get, Post, or PostForm will be used instead of Do.
-func (c *Client) Do(req *Request) (resp *Response, err error) {
+func (c *Client) Do(req *Request) (*Response, error) {
        method := valueOrDefault(req.Method, "GET")
        if method == "GET" || method == "HEAD" {
                return c.doFollowingRedirects(req, shouldRedirectGet)
@@ -416,54 +421,83 @@ func (c *Client) Get(url string) (resp *Response, err error) {
 
 func alwaysFalse() bool { return false }
 
-func (c *Client) doFollowingRedirects(ireq *Request, shouldRedirect func(int) bool) (resp *Response, err error) {
-       var base *url.URL
-       redirectChecker := c.CheckRedirect
-       if redirectChecker == nil {
-               redirectChecker = defaultCheckRedirect
+// checkRedirect calls either the user's configured CheckRedirect
+// function, or the default.
+func (c *Client) checkRedirect(req *Request, via []*Request) error {
+       fn := c.CheckRedirect
+       if fn == nil {
+               fn = defaultCheckRedirect
        }
-       var via []*Request
+       return fn(req, via)
+}
 
-       if ireq.URL == nil {
-               ireq.closeBody()
+func (c *Client) doFollowingRedirects(req *Request, shouldRedirect func(int) bool) (*Response, error) {
+       if req.URL == nil {
+               req.closeBody()
                return nil, errors.New("http: nil Request.URL")
        }
 
-       req := ireq
-       deadline := c.deadline()
-
-       urlStr := "" // next relative or absolute URL to fetch (after first request)
-       redirectFailed := false
-       for redirect := 0; ; redirect++ {
-               if redirect != 0 {
-                       nreq := new(Request)
-                       nreq.Cancel = ireq.Cancel
-                       nreq.Method = ireq.Method
-                       if ireq.Method == "POST" || ireq.Method == "PUT" {
-                               nreq.Method = "GET"
+       var (
+               deadline = c.deadline()
+               reqs     []*Request
+               resp     *Response
+       )
+       uerr := func(err error) error {
+               req.closeBody()
+               method := valueOrDefault(reqs[0].Method, "GET")
+               var urlStr string
+               if resp != nil {
+                       urlStr = resp.Request.URL.String()
+               } else {
+                       urlStr = req.URL.String()
+               }
+               return &url.Error{
+                       Op:  method[:1] + strings.ToLower(method[1:]),
+                       URL: urlStr,
+                       Err: err,
+               }
+       }
+       for {
+               // For all but the first request, create the next
+               // request hop and replace req.
+               if len(reqs) > 0 {
+                       loc := resp.Header.Get("Location")
+                       if loc == "" {
+                               return nil, uerr(fmt.Errorf("%d response missing Location header", resp.StatusCode))
                        }
-                       nreq.Header = make(Header)
-                       nreq.URL, err = base.Parse(urlStr)
+                       u, err := req.URL.Parse(loc)
                        if err != nil {
-                               break
+                               return nil, uerr(fmt.Errorf("failed to parse Location header %q: %v", loc, err))
                        }
-                       if len(via) > 0 {
-                               // Add the Referer header.
-                               lastReq := via[len(via)-1]
-                               if ref := refererForURL(lastReq.URL, nreq.URL); ref != "" {
-                                       nreq.Header.Set("Referer", ref)
-                               }
-
-                               err = redirectChecker(nreq, via)
-                               if err != nil {
-                                       redirectFailed = true
-                                       break
-                               }
+                       ireq := reqs[0]
+                       req = &Request{
+                               Method: ireq.Method,
+                               URL:    u,
+                               Header: make(Header),
+                               Cancel: ireq.Cancel,
+                       }
+                       if ireq.Method == "POST" || ireq.Method == "PUT" {
+                               req.Method = "GET"
+                       }
+                       // Add the Referer header from the most recent
+                       // request URL to the new one, if it's not https->http:
+                       if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL); ref != "" {
+                               req.Header.Set("Referer", ref)
+                       }
+                       if err := c.checkRedirect(req, reqs); err != nil {
+                               // Special case for Go 1 compatibility: return both the response
+                               // and an error if the CheckRedirect function failed.
+                               // See https://golang.org/issue/3795
+                               // The resp.Body has already been closed.
+                               ue := uerr(err)
+                               ue.(*url.Error).URL = loc
+                               return resp, ue
                        }
-                       req = nreq
                }
 
-               urlStr = req.URL.String()
+               reqs = append(reqs, req)
+
+               var err error
                if resp, err = c.send(req, deadline); err != nil {
                        if !deadline.IsZero() && !time.Now().Before(deadline) {
                                err = &httpError{
@@ -471,46 +505,21 @@ func (c *Client) doFollowingRedirects(ireq *Request, shouldRedirect func(int) bo
                                        timeout: true,
                                }
                        }
-                       break
+                       return nil, uerr(err)
                }
 
-               if shouldRedirect(resp.StatusCode) {
-                       // Read the body if small so underlying TCP connection will be re-used.
-                       // No need to check for errors: if it fails, Transport won't reuse it anyway.
-                       const maxBodySlurpSize = 2 << 10
-                       if resp.ContentLength == -1 || resp.ContentLength <= maxBodySlurpSize {
-                               io.CopyN(ioutil.Discard, resp.Body, maxBodySlurpSize)
-                       }
-                       resp.Body.Close()
-                       if urlStr = resp.Header.Get("Location"); urlStr == "" {
-                               err = fmt.Errorf("%d response missing Location header", resp.StatusCode)
-                               break
-                       }
-                       base = req.URL
-                       via = append(via, req)
-                       continue
+               if !shouldRedirect(resp.StatusCode) {
+                       return resp, nil
                }
-               return resp, nil
-       }
-
-       method := valueOrDefault(ireq.Method, "GET")
-       urlErr := &url.Error{
-               Op:  method[:1] + strings.ToLower(method[1:]),
-               URL: urlStr,
-               Err: err,
-       }
 
-       if redirectFailed {
-               // Special case for Go 1 compatibility: return both the response
-               // and an error if the CheckRedirect function failed.
-               // See https://golang.org/issue/3795
-               return resp, urlErr
-       }
-
-       if resp != nil {
+               // Read the body if small so underlying TCP connection will be re-used.
+               // No need to check for errors: if it fails, Transport won't reuse it anyway.
+               const maxBodySlurpSize = 2 << 10
+               if resp.ContentLength == -1 || resp.ContentLength <= maxBodySlurpSize {
+                       io.CopyN(ioutil.Discard, resp.Body, maxBodySlurpSize)
+               }
                resp.Body.Close()
        }
-       return nil, urlErr
 }
 
 func defaultCheckRedirect(req *Request, via []*Request) error {