Go: cmd/vet: detect Get/Post http.Response assignment to "_", which is a memory leak

[dweitzman]

I made a rookie mistake and introduced a memory leak into a codebase by failing to realize that you must close the http.Response body returned by http.Post() even if you don't need the response content.

I suspect I'm not only one who has made this mistake.

I have a diff, if the proposal is accepted.

[meirf]

While ignoring the response might be common for Post, it should be rare for Get. So if we want a general solution, simply detecting that response is ignored ("_" as your title suggest) is only helpful for Post. In other words, the main use case of Get requires looking at the response so detecting "_" won't help the vast majority of Get cases.

If the general case (*Response is a return value of a function call and that response's body is not closed) is difficult to detect, may be better to narrow the scope of this issue to Post (and Do).

Here's a particularly nasty case of not closing the body: https://github.com/Azure/azure-sdk-for-go/commit/475dde0c61b55d1619215fe28c3aba375690162b#diff-08eb04d438c2ee00895ac528dc7790b5L112. *http.Response is returned from the function. The retry loop was overwriting resp causing it to leak without closing. But solving this particular case is a stretch - can still get huge value from detecting the regular cases.

I am surprised that this hasn't been discussed before; certainly seems useful. Even though it's clear in the docs, I've seen both rookies and veterans suffer this. I don't see it discussed in github - perhaps there's some discussion elsewhere. Tangentially related.

cc @bradfitz

[mvdan]

There's also the more general problem of not using an io.Closer properly once it is constructed. That is, never calling Close() on it.

If we only check for the _, err := http.Post(...) pattern, it seems to me like the check wouldn't be very powerful. What if we first construct the http.Request and then call Do on it? What if we don't ignore the http.Response, but we don't close it in some cases? The Azure bug linked above has both of these properties.

On the other hand, if we implement the more generic check, I imagine it would be very tricky to avoid false positives. For example, some implementations of io.Closer may be no-ops, and fine to ignore.

[andybons]

@alandonovan

[kaedys]

For example, some implementations of io.Closer may be no-ops, and fine to ignore.

There's even such a helper in the standard library (https://golang.org/pkg/io/ioutil/#NopCloser), for converting from a simple io.Reader to a ReadCloser with a no-op Close() method.

[alandonovan]

We already have in vet a list of functions whose result is not to be ignored; it sounds like http.Post should be on that list.

The general analysis that would solve this class of problems is called type-state verification. It checks, for example, that on a file returned by Open, all calls to Read and Write occur before Close, and that a call to Close occurs on all control flow paths. (A looser version of the check would ensure that at least some path calls Close.) The quality of such an analysis depends on how precisely it models data, control, and the call graph. Generally it takes a lot of analytical firepower (and CPU/RAM) to do a good job in the more sophisticated cases, but we can catch dumb errors with simpler heuristics. An example to follow might be the lostcancel check in vet, which ensures that a context's cancel function is "used" on all control flow paths after it is created. The check could perhaps be generalized to solve other problems of this form.

[adamdecaf]

What if we add -closer as a vet flag which errors if a local io.Closer is ignored? We would whitelist ioutil.nopCloser from the stdlib. It's a start towards getting the check on by default.

We already have in vet a list of functions whose result is not to be ignored; it sounds like http.Post should be on that list.

Could you share where this is?