Suggest closing http response body when not nil

Question

Suggest closing http response body when not nil

Closed this issue 9 years ago · 3 comments

This project looks really useful! Seems that sometimes using net/http package can be prone to subtle bugs when closing the response body so thought that maybe it would be useful to generate snippets of code which prevent this.

Currently output displayed is like...

// Generated by curl-to-Go: https://mholt.github.io/curl-to-go
resp, err := http.Get("http://www.google.com")
if err != nil {
    // handle err
}
defer resp.Body.Close()

But considering that on redirection failure both variables will be non-nil so there can be a leak

How about maybe changing output to something like below?

// Generated by curl-to-Go: https://mholt.github.io/curl-to-go
resp, err := http.Get("http://www.google.com")
if resp != nil {
        defer resp.Body.Close()
}
if err != nil {
        // handle err
}

Thanks!

Answer 1 · 2016-02-01T05:08:33.000Z

Interesting, thanks for the report. I've never experienced this (or heard of it) -- so I had to look it up... the author of the linked article didn't cite any source (nor is there a date on the article! I had to look in the page source, sigh).

After some digging I don't think this is still an issue (if it ever was) - resp is always set if err is nil.

And to quote Dave Cheney:

if err is nil
then you get a resp
call resp.Body.Close()
it's the law

Although a nil check wouldn't hurt, I don't think it's necessary, and I'd rather keep the code simple, where possible, for brevity's sake.

Answer 2 · 2016-02-01T07:09:59.000Z

Thanks for taking a look! And agree on keeping the code simple where possible :)
Also +1 on sources in original post... though I think the author refers to the behavior described here (also in link you shared):
https://github.com/golang/go/blob/release-branch.go1.5/src/net/http/client.go#L427
golang/go@dfd7f18

There is a test for this in Go describing that behavior so I believe it is correct... https://github.com/golang/go/blob/dfd7f18130e538c53a2974988caecacd53d473f1/src/pkg/net/http/client_test.go#L238

Below is a test adapted from test above which shows behavior in latest versions of Go:

package testing

import (
    "fmt"
    . "net/http"
    "net/http/httptest"
    "strconv"
    "testing"
)

var robotsTxtHandler = HandlerFunc(func(w ResponseWriter, r *Request) {
    w.Header().Set("Last-Modified", "sometime")
    fmt.Fprintf(w, "User-agent: go\nDisallow: /something/")
})

// Snippet based on https://github.com/golang/go/blob/dfd7f18130e538c53a2974988caecacd53d473f1/src/pkg/net/http/client_test.go#L177
func TestFailedRedirect(t *testing.T) {
    var ts *httptest.Server
    ts = httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
        n, _ := strconv.Atoi(r.FormValue("n"))
        // Test Referer header. (7 is arbitrary position to test at)
        if n == 7 {
            if g, e := r.Referer(), ts.URL+"/?n=6"; e != g {
                t.Errorf("on request ?n=7, expected referer of %q; got %q", e, g)
            }
        }
        if n < 15 {
            Redirect(w, r, fmt.Sprintf("/?n=%d", n+1), StatusFound)
            return
        }
        fmt.Fprintf(w, "n=%d", n)
    }))
    defer ts.Close()

    c := &Client{}
    res, err := c.Get(ts.URL)
    if res != nil && err != nil {
        t.Logf("Response: %+v", res)
        t.Logf("Error is: %+v", err)
    } else {
            t.Fatalf("Expected both response and error to be non-nil")
    }
}

Answer 3 · 2017-03-02T12:31:19.000Z

I was confused about whether the response ever had to be checked on error and it appears it does not because the response body will already be closed. According to the docs, it says:

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.