From: Brad Fitzpatrick Date: Tue, 19 Jun 2012 16:10:14 +0000 (-0700) Subject: net/http: clarify client return values in docs X-Git-Tag: go1.1rc2~2914 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=e1d9fcd267e1a827e9841dce4c91def0777a90ee;p=gostls13.git net/http: clarify client return values in docs Also, fixes one violation found during testing where both response and error could be non-nil when a CheckRedirect test failed. This is arguably a minor API (behavior, not signature) change, but it wasn't documented either way and was inconsistent & non-Go like. Any code depending on the old behavior was wrong anyway. R=adg, rsc CC=golang-dev https://golang.org/cl/6307088 --- diff --git a/src/pkg/net/http/client.go b/src/pkg/net/http/client.go index 54564e0989..fba775fddc 100644 --- a/src/pkg/net/http/client.go +++ b/src/pkg/net/http/client.go @@ -14,6 +14,7 @@ import ( "errors" "fmt" "io" + "log" "net/url" "strings" ) @@ -87,9 +88,13 @@ type readClose struct { // Do sends an HTTP request and returns an HTTP response, following // policy (e.g. redirects, cookies, auth) as configured on the client. // -// A non-nil response always contains a non-nil resp.Body. +// 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. // -// Callers should close resp.Body when done reading from it. If +// When err is nil, resp always contains a non-nil resp.Body. +// +// Callers should close res.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. @@ -102,7 +107,8 @@ func (c *Client) Do(req *Request) (resp *Response, err error) { return send(req, c.Transport) } -// send issues an HTTP request. Caller should close resp.Body when done reading from it. +// send issues an HTTP request. +// Caller should close resp.Body when done reading from it. func send(req *Request, t RoundTripper) (resp *Response, err error) { if t == nil { t = DefaultTransport @@ -130,7 +136,14 @@ func send(req *Request, t RoundTripper) (resp *Response, err error) { if u := req.URL.User; u != nil { req.Header.Set("Authorization", "Basic "+base64.URLEncoding.EncodeToString([]byte(u.String()))) } - return t.RoundTrip(req) + resp, err = t.RoundTrip(req) + if err != nil { + if resp != nil { + log.Printf("RoundTripper returned a response & error; ignoring response") + } + return nil, err + } + return resp, nil } // True if the specified HTTP status code is one for which the Get utility should @@ -151,10 +164,15 @@ func shouldRedirect(statusCode int) bool { // 303 (See Other) // 307 (Temporary Redirect) // -// Caller should close r.Body when done reading from it. +// An error is returned if there were too many redirects 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. +// Caller should close resp.Body when done reading from it. // // Get is a wrapper around DefaultClient.Get. -func Get(url string) (r *Response, err error) { +func Get(url string) (resp *Response, err error) { return DefaultClient.Get(url) } @@ -167,8 +185,13 @@ func Get(url string) (r *Response, err error) { // 303 (See Other) // 307 (Temporary Redirect) // -// Caller should close r.Body when done reading from it. -func (c *Client) Get(url string) (r *Response, err error) { +// An error is returned if the Client's CheckRedirect function fails +// 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. +// Caller should close resp.Body when done reading from it. +func (c *Client) Get(url string) (resp *Response, err error) { req, err := NewRequest("GET", url, nil) if err != nil { return nil, err @@ -176,7 +199,7 @@ func (c *Client) Get(url string) (r *Response, err error) { return c.doFollowingRedirects(req) } -func (c *Client) doFollowingRedirects(ireq *Request) (r *Response, err error) { +func (c *Client) doFollowingRedirects(ireq *Request) (resp *Response, err error) { // TODO: if/when we add cookie support, the redirected request shouldn't // necessarily supply the same cookies as the original. var base *url.URL @@ -224,17 +247,17 @@ func (c *Client) doFollowingRedirects(ireq *Request) (r *Response, err error) { req.AddCookie(cookie) } urlStr = req.URL.String() - if r, err = send(req, c.Transport); err != nil { + if resp, err = send(req, c.Transport); err != nil { break } - if c := r.Cookies(); len(c) > 0 { + if c := resp.Cookies(); len(c) > 0 { jar.SetCookies(req.URL, c) } - if shouldRedirect(r.StatusCode) { - r.Body.Close() - if urlStr = r.Header.Get("Location"); urlStr == "" { - err = errors.New(fmt.Sprintf("%d response missing Location header", r.StatusCode)) + if shouldRedirect(resp.StatusCode) { + resp.Body.Close() + if urlStr = resp.Header.Get("Location"); urlStr == "" { + err = errors.New(fmt.Sprintf("%d response missing Location header", resp.StatusCode)) break } base = req.URL @@ -244,13 +267,16 @@ func (c *Client) doFollowingRedirects(ireq *Request) (r *Response, err error) { return } + if resp != nil { + resp.Body.Close() + } + method := ireq.Method - err = &url.Error{ + return nil, &url.Error{ Op: method[0:1] + strings.ToLower(method[1:]), URL: urlStr, Err: err, } - return } func defaultCheckRedirect(req *Request, via []*Request) error { @@ -262,17 +288,17 @@ func defaultCheckRedirect(req *Request, via []*Request) error { // Post issues a POST to the specified URL. // -// Caller should close r.Body when done reading from it. +// Caller should close resp.Body when done reading from it. // // Post is a wrapper around DefaultClient.Post -func Post(url string, bodyType string, body io.Reader) (r *Response, err error) { +func Post(url string, bodyType string, body io.Reader) (resp *Response, err error) { return DefaultClient.Post(url, bodyType, body) } // Post issues a POST to the specified URL. // -// Caller should close r.Body when done reading from it. -func (c *Client) Post(url string, bodyType string, body io.Reader) (r *Response, err error) { +// Caller should close resp.Body when done reading from it. +func (c *Client) Post(url string, bodyType string, body io.Reader) (resp *Response, err error) { req, err := NewRequest("POST", url, body) if err != nil { return nil, err @@ -283,28 +309,30 @@ func (c *Client) Post(url string, bodyType string, body io.Reader) (r *Response, req.AddCookie(cookie) } } - r, err = send(req, c.Transport) + resp, err = send(req, c.Transport) if err == nil && c.Jar != nil { - c.Jar.SetCookies(req.URL, r.Cookies()) + c.Jar.SetCookies(req.URL, resp.Cookies()) } - return r, err + return } -// PostForm issues a POST to the specified URL, -// with data's keys and values urlencoded as the request body. +// PostForm issues a POST to the specified URL, with data's keys and +// values URL-encoded as the request body. // -// Caller should close r.Body when done reading from it. +// When err is nil, resp always contains a non-nil resp.Body. +// Caller should close resp.Body when done reading from it. // // PostForm is a wrapper around DefaultClient.PostForm -func PostForm(url string, data url.Values) (r *Response, err error) { +func PostForm(url string, data url.Values) (resp *Response, err error) { return DefaultClient.PostForm(url, data) } // PostForm issues a POST to the specified URL, // with data's keys and values urlencoded as the request body. // -// Caller should close r.Body when done reading from it. -func (c *Client) PostForm(url string, data url.Values) (r *Response, err error) { +// When err is nil, resp always contains a non-nil resp.Body. +// Caller should close resp.Body when done reading from it. +func (c *Client) PostForm(url string, data url.Values) (resp *Response, err error) { return c.Post(url, "application/x-www-form-urlencoded", strings.NewReader(data.Encode())) } @@ -318,7 +346,7 @@ func (c *Client) PostForm(url string, data url.Values) (r *Response, err error) // 307 (Temporary Redirect) // // Head is a wrapper around DefaultClient.Head -func Head(url string) (r *Response, err error) { +func Head(url string) (resp *Response, err error) { return DefaultClient.Head(url) } @@ -330,7 +358,7 @@ func Head(url string) (r *Response, err error) { // 302 (Found) // 303 (See Other) // 307 (Temporary Redirect) -func (c *Client) Head(url string) (r *Response, err error) { +func (c *Client) Head(url string) (resp *Response, err error) { req, err := NewRequest("HEAD", url, nil) if err != nil { return nil, err diff --git a/src/pkg/net/http/client_test.go b/src/pkg/net/http/client_test.go index 9b4261b9f6..e2a08204e0 100644 --- a/src/pkg/net/http/client_test.go +++ b/src/pkg/net/http/client_test.go @@ -231,7 +231,6 @@ func TestRedirects(t *testing.T) { checkErr = errors.New("no redirects allowed") res, err = c.Get(ts.URL) - finalUrl = res.Request.URL.String() if e, g := "Get /?n=1: no redirects allowed", fmt.Sprintf("%v", err); e != g { t.Errorf("with redirects forbidden, expected error %q, got %q", e, g) }