fix: keep LoggableResponseBody's single-close guarantee honest when close() throws#137
Merged
Merged
Conversation
LoggableResponseBody.closeDelegateOnce() set the delegateClosed flag only after delegate.close() returned normally. When that close throws, the flag was left false, so a second close() reaching the delegate through the other entry point (the over-cap one-shot PrefixThenTailSource.close() vs the wrapper's own close()) would close the delegate a second time. The guard exists precisely to guarantee a single close for delegates whose handles are not safe to close twice, and the error path silently broke that guarantee. Flip the guard in a finally so the delegate is marked closed whether or not its close() succeeds, while still letting the exception propagate. This also matches the drain-path error handler, which already marks the delegate closed after a failed source close so a later close() is a no-op. Closes #115
On the full-capture path, drainAndCache() closed the source and only then set the single-close guard. If that close() threw, the guard was left unset and control fell into the catch block, which closed the same source a second time — the double-close the wrapper's guard exists to prevent (some sockets/streams throw on double-close). The close failure was also stored as a drainError, so source() then re-threw on every call and the caller could no longer read the complete body that had already been buffered. Close the source as best-effort on this path: swallow a close failure (mirroring the read-failure handler) and mark the delegate closed whether or not close() succeeds. A failure to release the handle after a complete capture is cleanup, not a capture failure, so the buffered body stays readable, captureException stays null, and the source is closed once. Add tests for a fully-captured source whose close() throws: the body remains readable and unpoisoned, and the source is closed exactly once across the drain and a later wrapper close. Also fold the close-guard rationale into the closeDelegateOnce KDoc to drop a duplicated comment.
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
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.
Problem
LoggableResponseBodyguarantees its delegate (and, by ownership, the delegate's source) is closed at most once — some sockets and streams throw on double-close, which is the whole reason thedelegateClosedguard and thecloseDelegateOnce()helper exist. Two close paths broke that guarantee when aclose()itself threw.1.
closeDelegateOnce()— over-cap pathThe guard was flipped only after
delegate.close()returned normally:If
delegate.close()throws,delegateClosedis leftfalse. Both the wrapper's ownclose()and the over-cap one-shot source (PrefixThenTailSource.close()) reach the delegate through this method from independent entry points, so a failure on the first path leaves the guard unset and the second path closes the delegate again.2.
drainAndCache()— full-capture pathWhen the whole body fits within the cap, the drain closes the source and only then sets the guard:
If
capturedSource.close()throws, the guard is again left unset and control falls into thecatchblock, which closes the same source instance a second time. Worse, the close failure is stored as adrainError, sosource()then re-throws on every subsequent call — denying the caller the complete body that had already been buffered, even though the capture itself succeeded. That also contradicts the documented contract thatcaptureExceptionisnullwhen the body was captured successfully.Change
Over-cap path: flip the guard in a
finallyso the delegate is marked closed whether or not itsclose()succeeds, while still letting the exception propagate:Full-capture path: close the source as best-effort. The capture has already succeeded, so a failure to release the handle is cleanup, not a capture failure: swallow it (mirroring the existing read-failure handler) and mark the delegate closed either way.
The complete body stays readable,
captureExceptionstaysnull, and the source is closed exactly once.Tests
close()throws is invoked exactly once across twoclose()calls (the over-cap one-shot source close followed by the wrapper close).close()throws stays readable and is not poisoned (source()returns the complete body,captureExceptionisnull).close()throws is closed exactly once across the drain and a later wrapper close.Gated build
./gradlew buildpasses — the full multi-module build, including ktlint, detekt,allWarningsAsErrors, explicit-API strict mode,apiCheck, the Kover 80% aggregate coverage gate, and the R8 shrink-survival guard. No public-API change, soapiCheckpassed without anapiDump.Closes #115