TIL: Go Response Body MUST be closed, even if you don’t read it

I recently witnessed a goroutine leak in my Go server. It looked like this.

goroutine profile: total 41053
20524 @ 0x43a656 0x44a7fe 0x65c512 0x46bec1
#   0x65c511    net/http.(*persistConn).writeLoop+0xf1  /usr/local/go/src/net/http/transport.go:2410

20523 @ 0x43a656 0x44a7fe 0x65b425 0x46bec1
#   0x65b424    net/http.(*persistConn).readLoop+0xd84  /usr/local/go/src/net/http/transport.go:2227

Searching online, the overwhelming consensus was that there must be a unclosed http.Response Body. Now, I was under the impression that you only need to close the response body, if you read it. If you didn’t read it, then there’s no need to close it.

So, I kept chasing all the places where I was reading the response, ensuring that each and every one of them was being closed.

client := u.GetHTTPClient()
resp, err := client.Do(req)
if err != nil {
    return errors.Wrapf(err, "addCloudflareDNS")
}
defer resp.Body.Close()

respBody, err := ioutil.ReadAll(resp.Body)
if err != nil {
    return errors.Wrapf(err, "addCloudflareDNS")
}

But, completely glossed over this seemingly simple health check code.

resp, err := http.Get("https://url/health")
if err != nil {
    return err
}
// MUST read resp.Body and do a resp.Body.Close() here
if resp.StatusCode == 200 {
    return nil
}
return fmt.Errorf("invalid response status: %d", resp.StatusCode)

Turns out, you MUST always read the response.Body and close it, irrespective of whether you need it or not.

The http Client and Transport guarantee that Body is always non-nil, even on responses without a body or responses with a zero-length body. It is the caller’s responsibility to close Body. The default HTTP client’s Transport may not reuse HTTP/1.x “keep-alive” TCP connections if the Body is not read to completion and closed.

Now, it’s still possible that you forget to close the body somewhere. A simple way to deal with that is to never use http.Get, or any methods which are using http.DefaultClient. Instead, only use a client that you created.

var httpClient *http.Client

func HTTPClient() *http.Client {
    return httpClient
}

func init() {
    // Initialize an HTTP client. We'll use this for every connection.
    t := http.DefaultTransport.(*http.Transport).Clone()
    t.MaxIdleConns = 100
    t.MaxConnsPerHost = 100
    t.MaxIdleConnsPerHost = 100

    httpClient = &http.Client{
        Timeout:   time.Minute,
        Transport: t,
    }
}

If you always use this httpClient, then Go(ds) would be kinder to you and close your unclosed response bodies for you.

Go HTTP Response is an Anti-Pattern

Go’s http.Response (link) is, dare I say, an anti-pattern because of implicit expectations. The expectation that one must read and close http.Response.Body even if you don’t need it, would be considered a bad API choice if you were to design it today. Here’s why:

A typical pattern for objects which need to be cleaned up is this:

obj, err := GetObject(...)
if err != nil { check(err) }
defer obj.Close()

Doing obj.Close() would allow obj to run whatever cleanup was necessary to deal with its internal states. If this was the pattern, no one would be confused by http.Get, or httpClient.Do, etc. But, the pattern here is:

obj, err := GetObject(...)
if err != nil { check(err) }
defer func() {
    if obj.InnerField != nil {
        cleanUp(obj.InnerField)
        obj.InnerField.Close()
    }
}

Which is strange, that you need to know about the InnerField and how to clean it up, everytime you get the obj.

If this still doesn’t feel strange to you, imagine that you have another InnerField2 which also needs to be closed.

obj, err := GetObject(...)
if err != nil { check(err) }
defer func() {
    if obj.InnerField1 != nil {
        cleanUp1(obj.InnerField1)
        obj.InnerField1.Close()
    }

    if obj.InnerField2 != nil {
        cleanUp2(obj.InnerField2)
        obj.InnerField2.Close()
    }
}()

Now, that we can all agree starts to look real strange. So, I wager that http.Response is actually missing a Close() function, and that having to explicitly run read and close an inner field (Body) within Response, even if you don’t touch it, is an anti-pattern. An anti-pattern that’s entrenched into one of the most used libraries, http.

---

P.S. How long have I been writing Go code? Almost a decade. But, I guess I never had to deal with direct HTTP connections. There was always a gRPC in my previous projects.

Update: There’s a healthy discussion on Reddit about this blog post.

---

---

---

---