-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Conversation
@imkira good catch! We can see in go's |
Thank you @ryanuber . |
@@ -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 { |
There was a problem hiding this comment.
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.
LGTM, thanks! |
fixed: body not closed for non HTTP 200 responses
Co-authored-by: Saad Jamal <[email protected]>
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.