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

Read, close and replace the http Reponse Body #300

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

Jakob3xD
Copy link
Collaborator

Description

With this change the http reponse body gets automatically closed by us so the user don't have to do it and it prevents unclosed connections if the user forgot to close it. This wont brake any existing implementations as you can still call .Close() on the Body.

Issues Resolved

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

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.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@dblock
Copy link
Member

dblock commented Apr 14, 2023

Looks good, needs a unit test, including to what happens with a double close by the caller.

@Jakob3xD Jakob3xD force-pushed the close-response-body branch from aaea13b to eb5be7f Compare April 14, 2023 20:55
@Jakob3xD
Copy link
Collaborator Author

Looks good, needs a unit test, including to what happens with a double close by the caller.

I am not 100% sure what kind of test you expect. Before this MR and after this MR the user can call Close() as often as they want. As the NopCloser just returns nil when calling Close it is obsolete to call it and to test it?

@dblock
Copy link
Member

dblock commented Apr 17, 2023

Looks good, needs a unit test, including to what happens with a double close by the caller.

I am not 100% sure what kind of test you expect. Before this MR and after this MR the user can call Close() as often as they want. As the NopCloser just returns nil when calling Close it is obsolete to call it and to test it?

I think of tests as expectations to prevent regressions. Imagine the implementation changes, what behavior do you expect?

  • I expect that Close can be called multiple times on res.Body.
  • I expect that NopCloser works as designed, i.e. returns nil when calling Close and that you can call Close multiple times, so those would be unit tests for NopCloser

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
@Jakob3xD Jakob3xD force-pushed the close-response-body branch from eb5be7f to f889dc7 Compare April 23, 2023 16:47
@Jakob3xD
Copy link
Collaborator Author

@dblock I added a test. It will fail if the resp.Body is not a NopCloser. Without my added change it would fail with following error:

=== RUN   TestTransportBodyClose
    opensearchtransport_integration_test.go:145: Failed to read the reponse body: http: read on closed response body
--- FAIL: TestTransportBodyClose (0.00s)

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Very good.

@dblock dblock merged commit b9b5afc into opensearch-project:main Apr 24, 2023
@Jakob3xD Jakob3xD deleted the close-response-body branch April 25, 2023 14:01
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.

3 participants