Connector: close response bodies

[yunginnanet]

HTTP response bodies must be closed.

[leoluk]

Nice catch, thanks! I think this fix is insufficient, though, since it doesn't catch error cases. The idomatic way is to put defer res.Body.Close() before the ioutil.ReadAll, which will catch all return conditions.

[yunginnanet]

Nice catch, thanks! I think this fix is insufficient, though, since it doesn't catch error cases. The idomatic way is to put defer res.Body.Close() before the ioutil.ReadAll, which will catch all return conditions.

I'd wager that defer is really not necessary for GetStatus because there are no early returns. With that being said I made sure we return the error if it exists, and I added a defer call in Request. Let me know what you think.

Edit 1

defer func() {
err = res.Body.Close()
}()

data, err = ioutil.ReadAll(res.Body)
if err != nil {
return nil, err
}

return 

I do realize that we have a risk here of overwriting the error produced by ioutil.ReadAll(res.Body).

I feel that at that rate it might as well be the same error. The response body failed to read, and so of course it also failed to close.
Either way we need to attempt to close it, and for debugging I'd say it's 6 one way and half a dozen the other, if you will.

The other options that come to mind for avoiding that likely harmless edge-case seem over complicated to me.

As for the case where we have a non-standard HTTP return as well as a response body that won't close, I'd wager that the body not closing is more pressing as it implies something is much more broken than a bad status code. That being said if we want to preserve that error in addition, we may need to check for a populated err in the defer call and do some sort of wrapping.

Update: fixed in 843d591

[yunginnanet]

There. Now we won't overwrite non-nil errors in the defer call, I'm gonna try to pry my ADHD eyes away from this though for the moment.

pre-squash branch for any desired cherry picking here

[yunginnanet]

have been using a vendorized copy of this module with all of these changes in this PR through dev -> qa -> and now prod for the past 5 months or so btw

[hendrikhofstadt]

Sorry I missed this. Thanks a lot for the contribution.