fix: close response body in http strategy #718
Conversation
From to the documentation: > If the returned error is nil, the Response will contain a non-nil body which the user is expected to close. Failed requests (due to wrong response or status code) were leaving open connections. Close the response body according to the documentation.
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
wait/http.go
Outdated
| @@ -217,9 +217,11 @@ func (ws *HTTPStrategy) WaitUntilReady(ctx context.Context, target StrategyTarge | |||
| continue | |||
| } | |||
| if ws.StatusCodeMatcher != nil && !ws.StatusCodeMatcher(resp.StatusCode) { | |||
| resp.Body.Close() | |||
There was a problem hiding this comment.
I think we should handle the error here too, as in L227
There was a problem hiding this comment.
thank you for the feedback!
the error is used to continue to the next iteration, we're already calling continue here. I think I'm missing something
There was a problem hiding this comment.
I know that golangci-lint checks for non-handled errors. So I'd say that we need to handle them in case the Close call fails
There was a problem hiding this comment.
I know that golangci-lint checks for non-handled errors.
I'm not able to reproduce this. I'm running golangci-lint v1.50.1 and I can see errcheck is enabled but nothing is being reported.
I've looked at the codebase and it seems there are other places where the error is being assigned to an empty var. I've pushed some changes to do the same here. Let me know what you think 🏓
There was a problem hiding this comment.
Sorry for the 🏓 (please see #653)
if golangci does not complain, I'm good with it.
There was a problem hiding this comment.
No worries, congratulations! 😄
There was a problem hiding this comment.
HI @kruskall , thanks for this fix, where I only see that we are not handling the potential error after closing the Body. Could you add that? 🙏
* main: (44 commits) feat: support passing registry credentials to the reaper (testcontainers#647) fix: close response body in http strategy (testcontainers#718) chore: move e2e module to postgres example module (testcontainers#717) chore: bump containerd transitive dep in examples (testcontainers#715) chore(deps): bump github.com/containerd/containerd from 1.6.12 to 1.6.14 (testcontainers#703) chore(deps): bump github.com/compose-spec/compose-go in /modules/compose (testcontainers#710) chore: bump testcontainers-go to 0.17.0 in examples (testcontainers#714) chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#711) chore: support running MySQL compose in ARM (testcontainers#712) chore: simplify compose replace directives (testcontainers#713) chore: add compose module to dependabot (testcontainers#709) chore: move compose code to a separate module (testcontainers#650) docs: refine onboarding process with quickstart guide (testcontainers#706) chore: move redis-specific tests to the example module (testcontainers#701) chore: bump transitive dependencies (#527) chore: reduce concurrent builds (testcontainers#702) chore: add mysql example (testcontainers#700) chore(deps): bump google.golang.org/api from 0.104.0 to 0.105.0 (testcontainers#699) chore(deps): bump google.golang.org/api in /examples/firestore (testcontainers#683) chore(deps): bump cloud.google.com/go/spanner in /examples/spanner (testcontainers#688) ...
What does this PR do?
From to the documentation:
Close the response body according to the documentation.
Why is it important?
Failed requests (due to wrong response or status code) were leaving open connections.
Related issues