Skip to content

Commit a1ce62b

Browse files
committed
fix: close the full-capture source best-effort and exactly once
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.
1 parent 61e2df7 commit a1ce62b

2 files changed

Lines changed: 89 additions & 10 deletions

File tree

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBody.kt

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -207,14 +207,15 @@ public class LoggableResponseBody
207207
* both through this single guard prevents a double-close on a delegate whose [source]
208208
* returns the same instance and whose [close] closes it (some sockets / streams throw on
209209
* double-close). Callers must hold [lock].
210+
*
211+
* The guard flips in a `finally`, so a delegate whose `close()` throws is still marked
212+
* closed: the failing close propagates to the caller, but the delegate is never closed a
213+
* second time. This matches the drain path, which likewise marks the delegate closed after
214+
* a best-effort source close so a later [close] is a no-op.
210215
*/
211216
@Throws(IOException::class)
212217
private fun closeDelegateOnce() {
213218
if (delegateClosed) return
214-
// Flip the guard whether or not close() succeeds: a delegate whose handle is not safe
215-
// to close twice must still see exactly one close even when that close throws. Marking
216-
// it closed in a finally also matches the drain-path error handler, which marks the
217-
// delegate closed after a failed source close so a later close() is a no-op.
218219
try {
219220
delegate.close()
220221
} finally {
@@ -242,10 +243,11 @@ public class LoggableResponseBody
242243
* the source was never obtained, so the delegate remains open and a subsequent [close]
243244
* will still close it.
244245
*
245-
* If EOF is reached within the cap the body is fully captured: the source is closed and
246-
* [fullyCaptured] is set, preserving the fully-repeatable behavior. If the cap is hit with
247-
* bytes still pending the delegate is **left open** and retained as [liveTail] so the
248-
* consumer still receives the rest of the body via [source].
246+
* If EOF is reached within the cap the body is fully captured: the source is closed
247+
* (best-effort — a close failure does not become a [drainError], since the capture already
248+
* succeeded) and [fullyCaptured] is set, preserving the fully-repeatable behavior. If the
249+
* cap is hit with bytes still pending the delegate is **left open** and retained as
250+
* [liveTail] so the consumer still receives the rest of the body via [source].
249251
*
250252
* If `provider.buffer()` throws (rare; would indicate a misconfigured provider), the error
251253
* is cached in [drainError] and an empty buffer is used as the fallback capture so
@@ -278,8 +280,19 @@ public class LoggableResponseBody
278280
remaining -= n
279281
}
280282
if (fullyCaptured) {
281-
// Whole body captured: close the source (and via ownership the delegate).
282-
capturedSource.close()
283+
// Whole body captured: close the source (and via ownership the delegate). The
284+
// capture already succeeded, so a close failure here is best-effort cleanup, not
285+
// a drain failure — swallow it (as the read-failure handler below does) so the
286+
// complete captured body stays readable and is never reported as a [drainError].
287+
// Mark the delegate closed whether or not close() throws, so a later close() does
288+
// not close the same source a second time (some sockets / streams throw on
289+
// double-close).
290+
try {
291+
capturedSource.close()
292+
} catch (_: Throwable) {
293+
// Best-effort: the body is already fully captured; a close failure must not
294+
// poison it or leave the single-close guard unset.
295+
}
283296
delegateClosed = true
284297
} else {
285298
// The cap was hit (or maxCaptureBytes was <= 0) with bytes still pending: retain

sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBodyTest.kt

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,34 @@ class LoggableResponseBodyTest {
5252
}
5353
}
5454

55+
/**
56+
* A body that fits the (unbounded) cap and whose source's [close] throws every time it is
57+
* called, counting invocations. Models a delegate whose handle is not safe to close twice and
58+
* whose close can fail — exercises the full-capture path's best-effort, single-close behavior.
59+
*/
60+
private fun fullyCapturedBodyWithThrowingClose(
61+
text: String,
62+
sourceCloseCount: AtomicInteger,
63+
): ResponseBody {
64+
val backing = Io.provider.buffer().also { it.writeUtf8(text) }
65+
val throwingOnClose =
66+
object : BufferedSource by backing {
67+
override fun close() {
68+
sourceCloseCount.incrementAndGet()
69+
throw IOException("source close failed")
70+
}
71+
}
72+
return object : ResponseBody() {
73+
override fun mediaType(): MediaType? = MediaType.parse("text/plain")
74+
75+
override fun contentLength(): Long = text.toByteArray(Charsets.UTF_8).size.toLong()
76+
77+
override fun source(): BufferedSource = throwingOnClose
78+
79+
override fun close() {}
80+
}
81+
}
82+
5583
// ----- source() that throws before entering .use {} -----
5684

5785
@Test
@@ -166,6 +194,44 @@ class LoggableResponseBodyTest {
166194
)
167195
}
168196

197+
@Test
198+
fun `fully captured body whose source close throws stays readable and is not poisoned`() {
199+
// Full-capture path: the body fits the cap, so the drain reads it all and then closes the
200+
// source. If that close throws, the capture itself already succeeded — the complete body is
201+
// in the buffer — so source() must still return it and captureException must stay null. A
202+
// close failure after a complete capture is best-effort cleanup, not a drain failure.
203+
val sourceCloseCount = AtomicInteger(0)
204+
val wrapper = LoggableResponseBody(fullyCapturedBodyWithThrowingClose("hello", sourceCloseCount))
205+
206+
assertEquals(
207+
"hello",
208+
wrapper.source().readUtf8(),
209+
"a complete capture must stay readable even when the source close fails",
210+
)
211+
assertNull(
212+
wrapper.captureException,
213+
"a successful capture must not surface a drain error for a best-effort close failure",
214+
)
215+
}
216+
217+
@Test
218+
fun `fully captured body whose source close throws is closed exactly once`() {
219+
// The best-effort close must still happen exactly once: the drain closes the source and,
220+
// because that close threw, the guard must be marked closed so a later wrapper.close() does
221+
// not close the same source a second time (some sockets / streams throw on double-close).
222+
val sourceCloseCount = AtomicInteger(0)
223+
val wrapper = LoggableResponseBody(fullyCapturedBodyWithThrowingClose("hello", sourceCloseCount))
224+
225+
wrapper.source().readUtf8()
226+
wrapper.close()
227+
228+
assertEquals(
229+
1,
230+
sourceCloseCount.get(),
231+
"a fully-captured source whose close() throws must be closed exactly once across drain and wrapper close",
232+
)
233+
}
234+
169235
// ----- additional failure-semantics tests -----
170236

171237
@Test

0 commit comments

Comments
 (0)