How to test for leaking connections in a Go test?

Question

After I've found leaks in my program, I've solved the problem. However, now I'm trying to find out how to " test " leaking connections in a Go test? This is my question.

I've tried to change the number of requests in my test, didn't matter. No matter what I do, the current number of TCP connections in my test stay the same.

func TestLeakingConnections(t *testing.T) {
    getter := myhttp.New()

    s := newServer(ok)
    defer s.Close()

    cur := tcps(t)
    for i := 0; i < 1000; i++ {
        r, _ := getter.GetWithTimeout(s.URL, time.Millisecond*10)
        r.Body.Close()
    }

    for tries := 10; tries >= 0; tries-- {
        growth := tcps(t) - cur
        if growth > 5 {
            t.Error("leaked")
            return
        }
    }
}

// find tcp connections
func tcps(t *testing.T) (conns int) {
    lsof, err := exec.Command("lsof", "-n", "-p", strconv.Itoa(os.Getpid())).Output()
    if err != nil {
        t.Skip("skipping test; error finding or running lsof")
    }

    for _, ls := range strings.Split(string(lsof), "\n") {
        if strings.Contains(ls, "TCP") {
            conns++
        }
    }
    return
}

func newServer(f http.HandlerFunc) *httptest.Server {
    return httptest.NewServer(http.HandlerFunc(f))
}

func ok(w http.ResponseWriter, r *http.Request) {
    w.Header().Add("Content-Type", "application/xml")
    io.WriteString(w, "<xml></xml>")
}

// myhttp package

// ...other code omitted for clarification

func (g *Getter) GetWithTimeout(
    url string,
    timeout time.Duration,
) (
    *http.Response, error,
) {
    // this is the leaking part
    // moving this out of here will stop leaks
    transport := http.Transport{
        DialContext: (&net.Dialer{
            Timeout: timeout,
        }).DialContext,
        TLSHandshakeTimeout:   timeout,
        ResponseHeaderTimeout: timeout,
        ExpectContinueTimeout: timeout,
    }

    client := http.Client{
        Timeout:   timeout,
        Transport: &transport,
    }

    return client.Get(url)
}

// fixture worker package

// some outside code injects getter into fixture_worker like this:
getter := myhttp.New()

// NewWithTimeout creates a new fetcher with timeout threshold
func NewWithTimeout(
    getter myhttp.HTTPGetter,
    fetchURL string,
    timeout time.Duration,
) *Fetcher {
    return &Fetcher{getter, fetchURL, timeout}
}

// Fetch fetches fixture xml
func (f *Fetcher) Fetch() (*parser.FixtureXML, error) {
    res, err := f.getter.GetWithTimeout(f.fetchURL, f.timeout)
    if err != nil {
        if res != nil && res.Body != nil {
            res.Body.Close()
        }
        return nil, errors.Wrap(err, ErrFetch.Error())
    }
    defer res.Body.Close()

    ioutil.ReadAll(res.Body)

    return &parser.FixtureXML{}, nil
}

---

---

Output of fixture worker lsof: https://pastebin.com/fDUkpYsE

Output of test: https://pastebin.com/uGpK0czn

Test never leaks whereas fixture worker it leaks.

Fixture worker is using the same code as the test, to request http gets using myhttp package.

Answer 1

If you want your test to represent reality, you need to use it in the same manner that you do outside of tests. In this case you're not reading any of the responses, and the transport happens to be preventing lost connections because it's discarding them as quickly as possible since they can't be reused.

Reading the response will use the connection, and get it into a state where it's "leaked". You also need to properly handle errors in all cases, and you always need to Close() the response body. The pattern to handle an http response and make sure it's closed is very simple, and doesn't necessarily require testing (see What could happen if I don't close response.Body in golang?)

    resp, err := GetWithTimeout(s.URL)
    if err != nil {
        t.Fatal(err)
    }
    ioutil.ReadAll(resp.Body)
    resp.Body.Close()

This is arguably of limited usefulness, since the most common bugs would result from improper error and response handling, and you're not testing that because the test needs to do it correctly in the first place.

The remaining problem here is that your GetWithTimeout method returns an error value and a valid http response, which contradicts the http package documentation as well as most user's expectations. If you're going to insert an error, it would be better to also handle the response at the same point to ensure the body is closed and discarded.

Finally, most of GetWithTimeout is superfluous, since not only is creating Transports every time incorrect, but creating an http.Client every request is usually unnecessary as they are meant to be reused and are safe for concurrent use.