Skip to content
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

fixed: body not closed for non HTTP 200 responses #608

Merged
merged 3 commits into from
Jan 19, 2015

Conversation

imkira
Copy link
Contributor

@imkira imkira commented Jan 16, 2015

requireOK function does not close the body for HTTP codes != 200.
Also, currently all calls to requireOK return immediately if there is an error and don't even look at the response.
In the end, no one closes the body so it is leaking.

I don't know if requestOK should be fixed, or if the caller should be fixed.
It seems to me that requireOK should be fixed but I was wondering if the current requireOK implementation is returning the response so that the caller may, say, look at the status code and do something rather than simply returning.
Rather than fixing it I assumed handling on the caller side would be acceptable since we still have to close the body for successful responses and that it just introduces an if resp != nil {} block.

Let me know if this looks good.

@ryanuber
Copy link
Member

@imkira good catch! We can see in go's http.Client Do() method, which is what's getting called under the hood, that it either returns nil, err, or resp, nil, so IMO it is probably safe to assume we would never do anything with the response body if err is not nil. I agree though that it wouldn't hurt to just check if the response struct is nil, and if not, at least close the Body to prevent leaking handles. Maybe this belongs in requireOK, which will also ensure that future uses of that method get the same protection.

@imkira
Copy link
Contributor Author

imkira commented Jan 19, 2015

Thank you @ryanuber .
I also agree that, like http.Client.Do, closing all resources on error (in this case resp.Body) is the right approach.
I am sending a new commit to attempt to fix this issue.
Please let me know if it looks good.

@@ -339,12 +339,16 @@ func encodeBody(obj interface{}) (io.Reader, error) {
// requireOK is used to wrap doRequest and check for a 200
func requireOK(d time.Duration, resp *http.Response, e error) (time.Duration, *http.Response, error) {
if e != nil {
return d, resp, e
if resp != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If resp comes from a http.Client.Do, there's no need to do this but I am trying not to take any assumptions.
Even if it comes from http.Client.Do, there should be no harm in closing it here.

@ryanuber
Copy link
Member

LGTM, thanks!

ryanuber added a commit that referenced this pull request Jan 19, 2015
fixed: body not closed for non HTTP 200 responses
@ryanuber ryanuber merged commit b30af95 into hashicorp:master Jan 19, 2015
duckhan pushed a commit to duckhan/consul that referenced this pull request Oct 24, 2021
Co-authored-by: Saad Jamal <saad@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants