ESD-1546: Close response body on Do error returns - #167
Open
Phil-Browne wants to merge 5 commits into
Open
Conversation
Client.Do returned without closing resp.Body on its error paths (failed CheckResponse, copy or decode error), leaking the underlying connection. Add a deferred close guarded by a success flag so the body is released on every error return; on success the caller still owns the body.
Phil-Browne
changed the title
Phil-Browne
changed the title
Reorder the LogResponseBody read-error path so the body is closed exactly once, add a doDiscard helper that drains and closes the body, and switch the call sites that discarded the response to it.
The existing regression test only covered the CheckResponse failure path. Add coverage for the other two error returns that the deferred close protects.
A close error means the connection was already broken, not that the request failed. doDiscard backs mutations whose response payload is ignored, so returning that error turned a completed mutation into a reported failure. Adds a test for the close-error path and one for the response-body logging branch, where Do closes the original body and swaps in a replacement reader.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Client.Doreturned on its error paths (failedCheckResponse, or a copy / decode error) without closingresp.Body, leaking the underlying connection.Adds a deferred close guarded by a
successflag so the body is released on every error return. On success the caller still owns the body, so behavior there is unchanged.doDiscard, which backs the mutations whose response payload is ignored, no longer returns the body-close error. A close failure means the connection was already broken, not that the request failed, so it must not turn a completed mutation into a reported failure.Includes regression tests that assert the body is closed when
Doreturns an error, that the original body is still closed exactly once when response-body logging swaps in a replacement reader, and thatdoDiscardignores a close error. The first and last fail without the fix.