Make sure we close HTTP response body

[MadLittleMods]

Make sure we close HTTP response body as that's what the docs say:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

-- https://pkg.go.dev/net/http#Client.Do

But it also may leak resources 🤷

Spawning from exploring this elsewhere in https://github.com/element-hq/synapse-small-hosts/pull/303

Dev notes

Error handling with res.Body.Close():

• https://blevesearch.com/news/Deferred-Cleanup,-Checking-Errors,-and-Potential-Problems/
• https://stackoverflow.com/questions/47293975/should-i-error-check-close-on-a-response-body
• https://manishrjain.com/must-close-golang-http-response

There is a bodyclose lint which is also available for golangci-lint

There are many false-positives though, for example:

• false positive when close is in another package timakin/bodyclose#39

---

HTTP client methods that deal with response bodies:

DoFederationRequest
DoHTTPRequest

Do
Get
Put
Post
Delete
Head

Pull Request Checklist

• Pull request includes a sign off

Signed-off-by: Eric Eastwood erice@element.io

[MadLittleMods]

There is a bodyclose lint which is also available for golangci-lint but I'm not willing to put in the effort (at this time) to update the the Complement codebase to utilize the latest golangci-lint and add it to CI, etc. For reference, it seems like we do use golangci-lint as part of build/scripts/build-and-lint.sh already but that's not in CI and I've never realized it's there while developing tests.

Additionally, there are many false-positives with the bodyclose. For example, we would immediately hit timakin/bodyclose#39 as our CloseIO utility is in a different package than our various usages. See also the issue list as there are many "false positive" issues.

And is not very well-suited for our use case since we don't expect downstream usages of user.MustDo(...) and user.Do(...) to handle the HTTP response as we handle it for them. Basically tracked by timakin/bodyclose#11 and timakin/bodyclose#76

[anoadragon453]

This is always something that's so tricky to get right.

Nice work with the helper function and using it everywhere.

[MadLittleMods]

Thanks for the review @anoadragon453 🦞