New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dweitzman opened this Issue Jun 18, 2018 · 6 comments

Comments

Projects
None yet
7 participants
@dweitzman

dweitzman commented Jun 18, 2018

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.

@dweitzman dweitzman changed the title from cmd/vet: detect http.Response assignment to "_", which is a memory leak to cmd/vet: detect Get/Post http.Response assignment to "_", which is a memory leak Jun 18, 2018

@meirf

This comment has been minimized.

Member

meirf commented Jun 18, 2018

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: Azure/azure-sdk-for-go@475dde0#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

This comment has been minimized.

Member

mvdan commented Jun 18, 2018

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

This comment has been minimized.

Member

andybons commented Jun 18, 2018

@kaedys

This comment has been minimized.

kaedys commented Jul 10, 2018

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

This comment has been minimized.

Contributor

alandonovan commented Jul 13, 2018

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

This comment has been minimized.

Contributor

adamdecaf commented Oct 30, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment