diff --git a/docs/http-body-logging-and-concurrency.md b/docs/http-body-logging-and-concurrency.md index 29668c8e..951e8be4 100644 --- a/docs/http-body-logging-and-concurrency.md +++ b/docs/http-body-logging-and-concurrency.md @@ -176,39 +176,60 @@ bytes tee'd up to the failure point — useful for diagnosing a failed request. the delegate's replayability: `isReplayable()` delegates, and `toReplayable()` returns a new `LoggableRequestBody` wrapping the delegate's replayable form. -### LoggableResponseBody — Drain-Once Strategy +### LoggableResponseBody — Bounded-Capture Strategy -**Problem**: Log response body content without consuming the stream that the caller needs. +**Problem**: Log response body content without consuming the stream that the caller needs, and +without buffering an arbitrarily large body into memory. -**Solution**: Drain once. On first access, the entire delegate body is drained into an internal -`Buffer` with a single `writeAll` (a segment transfer on Okio-backed providers) and the delegate -is closed. All subsequent reads are served from the captured buffer: +**Solution**: Drain a bounded prefix on first access. The wrapper reads at most `maxCaptureBytes` +bytes from the delegate into an internal `Buffer`; what happens next depends on whether the body +fit within that cap: ``` First access (source / snapshot / captureException): - delegate.source() ──writeAll──► Buffer (drains into pooled segments, - delegate closed via use {} then closes the source) - │ - ▼ -Subsequent access: captured.peek() - source() returns a fresh non-consuming view (no copy, no consumption) + read up to maxCaptureBytes from delegate.source() ──► Buffer (the capture prefix) + + Body fit within the cap (default, unlimited): + delegate closed; capture is the whole body + │ + ▼ + Subsequent access: captured.peek() (fresh non-consuming view, fully repeatable) + + Body exceeded the cap: + delegate left OPEN; only the prefix is buffered + │ + ▼ + source() (once): captured.peek() ─then─► live delegate tail + (one-shot: replays the prefix, then continues from the wire) ``` -**Why drain-once over tee-read?** +When the whole body fits within `maxCaptureBytes` the behavior is the classic drain-once: the +delegate is closed and every `source()` call returns a fresh `peek()` view. When the body is +larger than the cap, only the preview prefix is buffered, the delegate stays open, and `source()` +returns a **one-shot** stream that replays the captured prefix and then continues from the live +delegate tail — so the caller still receives the complete body. Because the tail is single-use, a +second `source()` call on an over-cap body throws `IllegalStateException`. -For responses, a tee-read (wrapping the source) would require the consumer to read the body to -trigger observation — if the consumer never reads, nothing is logged. Draining ensures the body -is always available for logging regardless of whether the consumer reads it. +The default `maxCaptureBytes` is `Long.MAX_VALUE` (unbounded, fully repeatable). The +instrumentation steps construct the wrapper via the internal `bounded(...)` seam capped at the +configured `bodyPreviewMaxBytes`, so logging never buffers more than a preview while the caller +still streams the rest. -**Trade-off**: The full response is held in memory. This is acceptable for API responses -(typically JSON, < 1 MB) but not suitable for streaming large downloads. The class documents -this constraint and offers `snapshot(maxBytes)` for capturing only a bounded preview prefix. +**Why bound the capture rather than always drain?** -**Why `source()` returns a new non-consuming view each time:** +A tee-read (wrapping the source) would require the consumer to read the body to trigger +observation — if the consumer never reads, nothing is logged. Draining a bounded prefix ensures a +preview is always available for logging regardless of whether the consumer reads, while keeping +memory bounded and avoiding a hang on a large or unbounded streaming body. -Each call returns an independent, non-consuming `BufferedSource` over the captured buffer (via -`Buffer.peek()`). This turns a single-use body into a repeatable one — the caller can read the -body multiple times without re-fetching or copying. +**Trade-off**: only the preview prefix is held in memory. For a body within the cap the capture is +the whole body and reads are fully repeatable; for a larger body the wrapper is single-consumer +(the live tail can be read only once). `snapshot(maxBytes)` returns a bounded prefix of the +capture, leaving the capture intact. + +**`contentLength()`**: returns the captured size only when the body fit within the cap (then the +capture *is* the body); otherwise it returns the delegate's true length, since the capture is just +a prefix. **Failure semantics**: a network error during the drain does not silently truncate. The wrapper retains whatever bytes were read before the failure and caches the exception: @@ -224,8 +245,8 @@ retains whatever bytes were read before the failure and caches the exception: The only logging output is a raw `ByteArray`: ```kotlin -fun snapshot(): ByteArray // the full captured body -fun snapshot(maxBytes: Int): ByteArray // a bounded prefix, leaving the capture intact +fun snapshot(): ByteArray // the full captured prefix (the whole body when it fit the cap) +fun snapshot(maxBytes: Int): ByteArray // a bounded slice of the capture, leaving it intact ``` `snapshot()` throws `IllegalStateException` if the captured size exceeds @@ -254,6 +275,13 @@ network resource. Direct buffer access (`tee.buffer`) is unsupported and throws backing buffer would reach only the tap, silently corrupting the wire body. Callers must use the typed `write*` methods so bytes reach both sinks. +`TeeSink` accepts a `tapLimit` (default `Long.MAX_VALUE`) that bounds how many bytes are mirrored +into the tap. Once the limit is reached the tee stops copying into the tap entirely but keeps +forwarding the **full** payload to the primary — the wire body is never truncated. The +instrumentation steps build the request wrapper via `LoggableRequestBody.bounded(...)` with the +limit set to `bodyPreviewMaxBytes`, so a multi-GB `FileRequestBody` upload mirrors only a bounded +preview into memory while streaming zero-copy to the transport. + ### Buffer-Based Capture `LoggableRequestBody` allocates its tap `Buffer` lazily (only on first write or snapshot) from diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/client/AsyncHttpClient.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/client/AsyncHttpClient.kt index 445ffde6..c6d03266 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/client/AsyncHttpClient.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/client/AsyncHttpClient.kt @@ -13,8 +13,9 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.util.Futures import java.io.IOException +import java.io.InterruptedIOException import java.util.concurrent.CompletableFuture -import java.util.concurrent.CompletionException +import java.util.concurrent.ExecutionException import java.util.concurrent.Executor /** @@ -56,9 +57,10 @@ import java.util.concurrent.Executor * - `HttpClient.asAsync(executor)` wraps a blocking transport in an async facade by submitting * each `execute(...)` call to the given [Executor]. Pair with a virtual-thread executor on * JDK 21+ for cheap concurrency. - * - `AsyncHttpClient.asBlocking()` returns a sync [HttpClient] that calls - * `executeAsync(...).join()` and unwraps the [CompletionException] so callers see the - * original [Exception] / [Response] semantics. + * - `AsyncHttpClient.asBlocking()` returns a sync [HttpClient] that blocks on + * `executeAsync(...).get()` and unwraps the [ExecutionException] so callers see the + * original [Exception] / [Response] semantics. The blocking wait honours + * `Thread.interrupt()`. */ public fun interface AsyncHttpClient : AutoCloseable { /** @@ -102,23 +104,37 @@ public fun HttpClient.asAsync(executor: Executor): AsyncHttpClient = } /** - * Wraps an [AsyncHttpClient] as a blocking [HttpClient] by calling `executeAsync(...).join()` - * and unwrapping the [CompletionException] so callers see the original exception. The current + * Wraps an [AsyncHttpClient] as a blocking [HttpClient] by blocking on `executeAsync(...).get()` + * and unwrapping the [ExecutionException] so callers see the original exception. The current * thread blocks until the future completes — pair with virtual threads (JDK 21+) to keep * carrier threads available. * + * The blocking wait honours `Thread.interrupt()`: interrupting the calling thread restores the + * interrupt flag, cancels the in-flight future, and throws an [InterruptedIOException] (an + * [IOException] subtype, so the `@Throws(IOException::class)` contract holds). + * * The returned [Response] must be closed by the caller, per the [HttpClient.execute] contract. */ @Throws(IOException::class) public fun AsyncHttpClient.asBlocking(): HttpClient = HttpClient { request -> + val future = executeAsync(request) try { - executeAsync(request).join() - } catch (ce: CompletionException) { - // `join()` wraps every exceptional completion in CompletionException; unwrap so + future.get() + } catch (ie: InterruptedException) { + // `get()` parks interruptibly (unlike `join()`). Restore the interrupt flag, abort + // the in-flight exchange, and surface an InterruptedIOException so callers' I/O + // error handling terminates cleanly. See RetryStep.awaitDelay for the same pattern. + Thread.currentThread().interrupt() + future.cancel(true) + val ioe = InterruptedIOException("Interrupted while waiting for response") + ioe.initCause(ie) + throw ioe + } catch (ee: ExecutionException) { + // `get()` wraps every exceptional completion in ExecutionException; unwrap so // callers' `catch (IOException)` blocks see the original failure rather than the // JDK wrapper. `CancellationException` from a cancelled future is unaffected — it - // is a RuntimeException, not a CompletionException, so it propagates as-is. - throw Futures.unwrap(ce) + // is a RuntimeException, not an ExecutionException, so it propagates as-is. + throw Futures.unwrap(ee) } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt index 584f3da2..85ef3af8 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/Headers.kt @@ -160,6 +160,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + validateValues(name, values) headersMap.computeIfAbsent(sanitizeName(name)) { mutableListOf() }.addAll(values) } @@ -179,6 +180,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + validateValues(name.caseInsensitiveName, values) headersMap.computeIfAbsent(name.caseInsensitiveName) { mutableListOf() }.addAll(values) } @@ -216,6 +218,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + validateValues(name, values) headersMap[sanitizeName(name)] = values.toMutableList() } @@ -243,6 +246,7 @@ public data class Headers private constructor( values: List, ): Builder = apply { + validateValues(name.caseInsensitiveName, values) headersMap[name.caseInsensitiveName] = values.toMutableList() } @@ -305,5 +309,29 @@ public data class Headers private constructor( * so locale-sensitive folding (Turkish `i`, etc.) would be incorrect here. */ private fun sanitizeName(value: String): String = value.lowercase(Locale.US).trim() + + /** + * Rejects header values that would enable request/header splitting before they reach a + * transport. A bare carriage return (`\r`) or line feed (`\n`) in a value lets an + * attacker inject a new header or even a second request once the value is serialised; + * OkHttp throws unchecked on such values and the JDK transport silently drops them, so we + * validate here at the transport-agnostic model layer to fail fast and uniformly. + * + * Policy: reject **only** CR/LF, not OkHttp's stricter printable-ASCII-only rule. CR/LF + * are the splitting vector and are illegal in every transport; tightening further would + * reject legitimate UTF-8 values that some transports (and the JDK) accept, so the + * conservative CR/LF check is the right model-layer contract. + */ + private fun validateValues( + name: String, + values: List, + ) { + values.forEach { value -> + require(value.indexOf('\r') < 0 && value.indexOf('\n') < 0) { + "Header value for '$name' must not contain a carriage return or line feed " + + "(\\r / \\n); such characters enable request/header splitting." + } + } + } } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ContextStore.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ContextStore.kt index ca9fe33a..5d8cd687 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ContextStore.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ContextStore.kt @@ -22,6 +22,25 @@ import java.util.concurrent.ConcurrentHashMap * * Entries are removed by [CallContext.close] when callers honour the close contract. * + * ## Lifecycle and leaks + * + * Eviction is **primarily close-only**: the normal path is that [CallContext.close] removes + * the entry. Only the **terminal** promotion context (the [ExchangeContext] that ends the + * chain) should be closed — closing an earlier link uses identity-conditional [remove] so it + * cannot evict the live successor that replaced it. A caller that fails to close on an + * exception path drops an entry that **pins the full Request + Response graph** — possibly an + * unread response body holding a transport connection open. + * + * To stop a missed close from leaking unboundedly, the backing map is **bounded** to + * [MAX_TRACKED_CONTEXTS] entries: after each insert ([set] / [put]) the map is drained back + * under the cap, mirroring the proven bound in + * [DigestChallengeHandler][org.dexpace.sdk.core.http.auth.DigestChallengeHandler]'s nonce + * counters. The bound is a backstop, not a substitute for closing — it caps memory but a + * leaked entry still holds its graph alive until evicted, and eviction is by arbitrary victim + * (see [drainToCap]), so a still-live call could lose its registry entry under heavy leak + * pressure. The store is the only strong reference on the live path, so a [java.lang.ref.WeakReference] + * cannot be used here: it would let an in-flight context be collected mid-call. + * * ## Thread-safety * * Thread-safe. The backing map is a [ConcurrentHashMap] so concurrent calls with distinct @@ -49,6 +68,7 @@ public object ContextStore { ) { val previous = contexts.putIfAbsent(callKey, context) require(previous == null) { "Call context key already registered: $callKey" } + drainToCap() } /** @@ -61,6 +81,7 @@ public object ContextStore { context: CallContext, ) { contexts[callKey] = context + drainToCap() } /** @@ -100,4 +121,29 @@ public object ContextStore { } return removed } + + /** + * Drains the backing map back under [MAX_TRACKED_CONTEXTS] after an insert. Mirrors + * [DigestChallengeHandler][org.dexpace.sdk.core.http.auth.DigestChallengeHandler]'s + * post-insert drain loop: rather than a single check-then-evict-then-insert (a non-atomic + * race that a burst of concurrent inserts can overshoot and then sit at the cap forever), + * each insert drains until the map is back under the cap, so the map converges to the + * bound. `ConcurrentHashMap` has no insertion order, so any single key is an acceptable + * victim — the first the iterator yields is dropped. + */ + private fun drainToCap() { + while (contexts.size > MAX_TRACKED_CONTEXTS) { + val iterator = contexts.keys.iterator() + if (!iterator.hasNext()) break + contexts.remove(iterator.next()) + } + } + + // Upper bound on call contexts tracked at once. The store is normally drained by + // close(), so this is a backstop against a missed close on an exception path leaking an + // entry that pins a full Request + Response graph. 4096 is large enough to never bite a + // realistic in-flight concurrency level (each entry is one live or recently-finished + // call), yet small enough to cap worst-case retained memory at a few thousand graphs. + // Matches the bounded-map style of DigestChallengeHandler.MAX_TRACKED_NONCES. + private const val MAX_TRACKED_CONTEXTS = 4096 } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/paging/PagedIterable.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/paging/PagedIterable.kt index 217e6891..5d739630 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/paging/PagedIterable.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/paging/PagedIterable.kt @@ -100,7 +100,8 @@ public class PagedIterable * **Page ownership.** Each yielded [PagedResponse] is owned by the consumer; this method * does not close them. Consumers MUST `close()` each page after use (or wrap iteration * in `use {}` / try-with-resources) to release the underlying response body. The - * flattening [iterator] path closes pages automatically — only direct callers of + * flattening [iterator] path closes pages automatically — each page is closed eagerly, + * as soon as its (already fully-materialized) items are taken — so only direct callers of * [byPage] need to manage page lifecycle. * * [maxPages] defaults to `Long.MAX_VALUE`. **Production callers should set a finite @@ -132,8 +133,11 @@ public class PagedIterable * Flattens all pages into an iterator of items. Each call starts a fresh iteration * (re-invokes [firstPage]). * - * The iterator owns the pages it pulls through [byPage] and closes each one once its - * items are exhausted — callers don't need to manage page lifecycle when iterating by + * The iterator owns the pages it pulls through [byPage] and closes each one eagerly, + * as soon as its items iterator is taken. Because [PagedResponse.value] is a + * fully-materialized list, the items survive the close, so a partial consume + * (`first()` / `take(n)` / `stream().findFirst()`) never strands an open response body + * or pooled connection. Callers don't need to manage page lifecycle when iterating by * item. * * **Error propagation.** If `pages.next()` throws, the exception propagates immediately @@ -146,7 +150,6 @@ public class PagedIterable object : AbstractIterator() { private val pages = byPage().iterator() private var currentItems: Iterator? = null - private var currentPage: PagedResponse? = null override fun computeNext() { // Advance through pages until we find one with items left, or pages are exhausted. @@ -156,8 +159,6 @@ public class PagedIterable setNext(items.next()) return } - currentPage?.close() - currentPage = null currentItems = null if (!pages.hasNext()) { done() @@ -167,8 +168,12 @@ public class PagedIterable // AbstractIterator transitions to FAILED state, so further hasNext() calls // raise IllegalArgumentException — this is the correct fail-fast behavior. val next = pages.next() - currentPage = next + // PagedResponse.value is a fully-materialized list, so we can take the items + // iterator and close the page immediately. Closing eagerly means a partial + // consume (first()/take(n)) never abandons an open response body or pooled + // connection waiting for a later computeNext() that may never run. currentItems = next.value.iterator() + next.close() } } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncPipelineBridges.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncPipelineBridges.kt index d0fa3b85..e567af62 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncPipelineBridges.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncPipelineBridges.kt @@ -12,8 +12,9 @@ package org.dexpace.sdk.core.http.pipeline import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.util.Futures +import java.io.InterruptedIOException import java.util.concurrent.CompletableFuture -import java.util.concurrent.CompletionException +import java.util.concurrent.ExecutionException import java.util.concurrent.Executor import java.util.concurrent.atomic.AtomicReference @@ -112,17 +113,32 @@ private fun sendInterruptibly( /** * Adapts an [AsyncHttpPipeline] into a synchronous [HttpPipeline] by blocking on - * `sendAsync(request).join()` for each `send(...)` call. The current thread blocks until the + * `sendAsync(request).get()` for each `send(...)` call. The current thread blocks until the * future completes; pair with virtual threads (JDK 21+) on the caller side to keep carrier * threads available. + * + * The blocking wait honours `Thread.interrupt()`: interrupting the calling thread restores the + * interrupt flag, cancels the in-flight future, and throws an [InterruptedIOException]. */ public fun AsyncHttpPipeline.toBlocking(): HttpPipeline { val async = this return HttpPipeline.of { request -> + val future = async.sendAsync(request) try { - async.sendAsync(request).join() - } catch (ce: CompletionException) { - throw Futures.unwrap(ce) + future.get() + } catch (ie: InterruptedException) { + // `get()` parks interruptibly (unlike `join()`). Restore the interrupt flag, abort + // the in-flight send, and surface an InterruptedIOException so the caller's I/O + // error handling terminates cleanly. + Thread.currentThread().interrupt() + future.cancel(true) + val ioe = InterruptedIOException("Interrupted while waiting for response") + ioe.initCause(ie) + throw ioe + } catch (ee: ExecutionException) { + // `get()` wraps exceptional completion in ExecutionException; unwrap so callers' + // `catch (IOException)` sees the original failure rather than the JDK wrapper. + throw Futures.unwrap(ee) } } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt index c54f486f..9d481d35 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt @@ -24,6 +24,7 @@ import org.dexpace.sdk.core.instrumentation.UrlRedactor import org.dexpace.sdk.core.instrumentation.makeCurrentWithLoggingContext import org.dexpace.sdk.core.instrumentation.metrics.DoubleHistogram import org.dexpace.sdk.core.instrumentation.metrics.LongCounter +import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.util.Clock import org.dexpace.sdk.core.util.Futures import java.util.concurrent.CompletableFuture @@ -48,7 +49,11 @@ import java.util.concurrent.CompletionException * * ## Body capture / failure semantics * - * Identical to the sync step — see its KDoc. + * Mostly identical to the sync step — see its KDoc. One difference: the response-body drain + * runs on the future-completion thread here, so an **unknown-length** (streaming) response + * body (`contentLength() < 0`) is left unwrapped — draining it could block the completion + * thread on a slow/idle producer. Known-length bodies are wrapped and bounded to the preview + * size as in the sync step. * * ## Thread-safety * @@ -153,7 +158,9 @@ public class DefaultAsyncInstrumentationStep val requestBody = request.body val wrappedRequestBody = if (shouldCaptureBody() && requestBody != null) { - LoggableRequestBody(requestBody) + // Cap the request-side tap so a multi-GB upload only mirrors a bounded preview + // into memory while still streaming the full payload to the transport. + LoggableRequestBody.bounded(requestBody, Io.provider, options.bodyPreviewMaxBytes.toLong()) } else { null } @@ -234,7 +241,15 @@ public class DefaultAsyncInstrumentationStep private fun wrapResponseForLogging(response: Response): Response { val responseBody = response.body if (!shouldCaptureBody() || responseBody == null) return response - val wrapped = LoggableResponseBody(responseBody) + // The bounded drain below runs on the future-completion thread. For an unknown-length + // (streaming) body the read could block on a slow/idle producer and stall the + // completion thread, so we skip body capture entirely for contentLength() < 0 — the + // body streams to the caller unwrapped with no preview. Known-length bodies are safe + // to drain up to the bounded preview size. + if (responseBody.contentLength() < 0L) return response + // Bound the in-memory capture to the preview size. The full body still streams to the + // caller via the wrapper's live tail; only a preview prefix is buffered. + val wrapped = LoggableResponseBody.bounded(responseBody, Io.provider, options.bodyPreviewMaxBytes.toLong()) // Force drain so we have bytes to log. Done here (not in the event emit) so a logging // failure can't mask the drain error from the caller — they still get the wrapped body // with the cached exception surfaced on source(). diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt index f8bbfbea..1dfbc908 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt @@ -21,6 +21,7 @@ import org.dexpace.sdk.core.instrumentation.UrlRedactor import org.dexpace.sdk.core.instrumentation.makeCurrentWithLoggingContext import org.dexpace.sdk.core.instrumentation.metrics.DoubleHistogram import org.dexpace.sdk.core.instrumentation.metrics.LongCounter +import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.util.Clock import java.io.IOException @@ -34,11 +35,15 @@ import java.io.IOException * ## Body capture semantics * * - **Request body**: wrapped before send; bytes are captured during the transport's `writeTo` - * via a `TeeSink`. After send returns, the captured snapshot is emitted in the - * `http.response` event. - * - **Response body**: wrapped after send. The wrapper drains lazily on first `source()` / - * `snapshot()`; this step calls `snapshot(bodyPreviewMaxBytes)` to force the drain and log - * a bounded preview. Subsequent caller reads see the cached buffer — no double-drain. + * via a `TeeSink`. The tap is capped at `bodyPreviewMaxBytes`, so a large upload mirrors only + * a bounded preview into memory while the full payload streams to the transport. After send + * returns, the captured snapshot is emitted in the `http.response` event. + * - **Response body**: wrapped after send with the in-memory capture bounded to + * `bodyPreviewMaxBytes`. The wrapper drains lazily on first `source()` / `snapshot()`; this + * step calls `snapshot(bodyPreviewMaxBytes)` to force the bounded drain and log a preview. + * A body larger than the cap still streams in full to the caller (the wrapper replays the + * captured prefix then continues from the live tail); a body within the cap stays fully + * repeatable. * * ## Failure handling * @@ -101,7 +106,9 @@ public class DefaultInstrumentationStep val requestBody = request.body val wrappedRequestBody = if (shouldCaptureBody() && requestBody != null) { - LoggableRequestBody(requestBody) + // Cap the request-side tap so a multi-GB upload only mirrors a bounded preview + // into memory while still streaming the full payload to the transport. + LoggableRequestBody.bounded(requestBody, Io.provider, options.bodyPreviewMaxBytes.toLong()) } else { null } @@ -144,7 +151,9 @@ public class DefaultInstrumentationStep private fun wrapResponseForLogging(response: Response): Response { val responseBody = response.body if (!shouldCaptureBody() || responseBody == null) return response - val wrapped = LoggableResponseBody(responseBody) + // Bound the in-memory capture to the preview size. The full body still streams to the + // caller via the wrapper's live tail; only a preview prefix is buffered. + val wrapped = LoggableResponseBody.bounded(responseBody, Io.provider, options.bodyPreviewMaxBytes.toLong()) // Force drain so we have bytes to log. Done here (not in the event emit) so a logging // failure can't mask the drain error from the caller — they still get the wrapped body // with the cached exception surfaced on source(). diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRedirectStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRedirectStep.kt index 79eb8af7..8116025a 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRedirectStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRedirectStep.kt @@ -328,18 +328,23 @@ public open class DefaultRedirectStep /** * Returns [uri] with its userinfo component cleared. If [uri] has no userinfo the * input is returned unchanged. + * + * The URI is rebuilt **textually** from its already-encoded (`raw*`) components rather + * than via the multi-argument [URI] constructor. That constructor takes *decoded* + * components and re-encodes them, which would corrupt a `Location` whose path or query + * carries percent-escaped reserved characters: `%2F` would decode to `/` and `%26` to + * `&`, silently changing the path/query structure. Reassembling from `rawPath` / + * `rawQuery` / `rawFragment` preserves the wire-exact encoding. */ private fun stripUserInfo(uri: URI): URI { if (uri.userInfo == null) return uri - return URI( - uri.scheme, - null, - uri.host, - uri.port, - uri.path, - uri.query, - uri.fragment, - ) + val sb = StringBuilder() + sb.append(uri.scheme).append("://").append(uri.host) + if (uri.port != -1) sb.append(':').append(uri.port) + uri.rawPath?.let { sb.append(it) } + uri.rawQuery?.let { sb.append('?').append(it) } + uri.rawFragment?.let { sb.append('#').append(it) } + return URI(sb.toString()) } /** diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt index d5758c11..9ecb37c3 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt @@ -46,11 +46,14 @@ import java.util.concurrent.ThreadLocalRandom * * ## Body replayability * - * A request whose method is non-idempotent (e.g. POST/PATCH) and whose body is **not** - * replayable is never retried — the loop runs exactly one attempt and returns the response - * (or rethrows the exception) as-is. Re-sending such a body would trip the body's consume-once - * guard and surface as a confusing wrapped [IllegalStateException]. Idempotent methods, and - * any method with no body or a replayable body, are retried normally. This mirrors + * Eligibility is gated on re-sendability. A body-less request is retried only when its method + * is idempotent; a body-bearing request is retried only when its body is replayable — a + * non-replayable body physically cannot be re-sent (the second `writeTo` trips the body's + * consume-once guard and surfaces as a confusing wrapped [IllegalStateException] that masks the + * real failure). A replayable body is re-sendable regardless of method; making the re-sent + * request idempotent (for a non-idempotent method, e.g. via an idempotency key) is the caller's + * responsibility. When the request is not re-sendable the loop runs exactly one attempt and + * returns the response (or rethrows the exception) as-is. This mirrors * `pipeline.step.retry.RetryStep.canRetry`. * * ## Delay precedence (highest to lowest) @@ -242,15 +245,15 @@ public open class DefaultRetryStep } /** - * Returns `true` when [request] may be re-sent. A request whose method is idempotent is - * always retry-safe; otherwise it is safe only when it has no body or a replayable one. - * A non-idempotent method (POST/PATCH) carrying a single-use body cannot be retried — - * the second `RequestBody.writeTo` would trip the body's consume-once guard. Mirrors - * `pipeline.step.retry.RetryStep.canRetry`. + * Returns `true` when [request] may be re-sent. A body-less request is retry-safe only + * when its method is idempotent; a body-bearing request is retry-safe only when its body + * is replayable — a non-replayable body cannot be re-sent (the second + * `RequestBody.writeTo` trips the body's consume-once guard and surfaces as a confusing + * wrapped [IllegalStateException]). Making a re-sent body-bearing request idempotent is + * the caller's responsibility. Mirrors `pipeline.step.retry.RetryStep.canRetry`. */ private fun isRetrySafe(request: Request): Boolean { - if (request.method in IDEMPOTENT_METHODS) return true - val body = request.body ?: return true + val body = request.body ?: return request.method in IDEMPOTENT_METHODS return body.isReplayable() } @@ -282,6 +285,8 @@ public open class DefaultRetryStep * The retryable response is closed BEFORE the backoff sleep so its body's resources * (socket / buffer) are not pinned open across a potentially long `Thread.sleep`. The * delay is computed from the response headers first, while the response is still open. + * If the should-retry predicate or [computeResponseDelay] throws, the response is closed + * before the throwable propagates so the retryable response never leaks. */ private fun decideRetryResponse( response: Response, @@ -290,11 +295,21 @@ public open class DefaultRetryStep retrySequenceStartNanos: Long, ): Boolean { val condition = HttpRetryCondition(response, null, tryCount, suppressed.orEmpty()) - if (!invokeShouldRetryResponse(condition)) return false // Compute the delay (may read Retry-After from the response) while the response is // still open, then release the response body's resources BEFORE we sleep so the - // socket/buffer is not held open across the backoff window. - val delay = computeResponseDelay(condition) + // socket/buffer is not held open across the backoff window. Both the should-retry + // predicate and computeResponseDelay (a subclass override) can throw; if either + // does we must still close the retryable response — otherwise its socket/buffer + // leaks. closeQuietly swallows IOException and Response.close is idempotent, so + // closing on the throw path is safe even though the happy path closes again. + val delay: Duration = + try { + if (!invokeShouldRetryResponse(condition)) return false + computeResponseDelay(condition) + } catch (t: Throwable) { + closeQuietly(response) + throw t + } logRetry(tryCount, delay, response.status.code, cause = null, retrySequenceStartNanos) closeQuietly(response) sleepOrAbort(delay, suppressed) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpInstrumentationOptions.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpInstrumentationOptions.kt index 7b41a82f..633a7b15 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpInstrumentationOptions.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpInstrumentationOptions.kt @@ -26,17 +26,26 @@ import org.dexpace.sdk.core.instrumentation.metrics.NoopMeter * * ## WARNING — [HttpLogLevel.BODY_AND_HEADERS] and streaming * - * When [logLevel] is [HttpLogLevel.BODY_AND_HEADERS] the response body is **fully drained - * into memory** before the caller ever sees it (the drain happens eagerly inside the - * instrumentation step). This is fundamentally incompatible with streaming responses: + * When [logLevel] is [HttpLogLevel.BODY_AND_HEADERS] both the request and response bodies are + * captured for logging, but the capture is **bounded to [bodyPreviewMaxBytes]** — only a preview + * prefix is held in memory, not the whole body: * - * - The entire response is buffered — large or unbounded payloads can exhaust the heap. - * - Backpressure is lost — the transport cannot pause the producer once the body is read. - * - First-byte latency for the caller approaches end-of-stream latency. + * - **Response body**: at most [bodyPreviewMaxBytes] bytes are buffered. A body within the cap + * is fully captured and stays repeatable; a larger body still streams in full to the caller + * (the wrapper replays the captured prefix then continues from the live tail) while only the + * preview occupies the heap. In the **sync** step the bounded drain happens eagerly inside + * the step. In the **async** step the bounded drain runs on the future-completion thread, so + * it is **skipped for unknown-length (streaming) bodies** (`contentLength() < 0`) — those + * stream to the caller unwrapped with no body preview, so a slow/idle producer never blocks + * the completion thread. + * - **Request body**: the request-side tap is likewise capped at [bodyPreviewMaxBytes], so a + * large (e.g. multi-GB file) upload mirrors only a bounded preview into memory while the full + * payload streams zero-copy to the transport. * - * Use [HttpLogLevel.HEADERS] (or [HttpLogLevel.NONE]) for endpoints that return large - * downloads, server-sent events, gRPC, or chunked encodings whose size is unknown ahead - * of time. [HttpLogLevel.BODY_AND_HEADERS] is intended for diagnostic builds against + * Even with the cap, [HttpLogLevel.BODY_AND_HEADERS] wraps known-length response bodies in a + * draining wrapper. Prefer [HttpLogLevel.HEADERS] (or [HttpLogLevel.NONE]) for endpoints that + * return large downloads, server-sent events, gRPC, or chunked encodings whose size is unknown + * ahead of time. [HttpLogLevel.BODY_AND_HEADERS] is intended for diagnostic builds against * small JSON/text payloads. */ public class HttpInstrumentationOptions diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpLogLevel.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpLogLevel.kt index e0557c1f..5e75cf26 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpLogLevel.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/HttpLogLevel.kt @@ -23,9 +23,11 @@ public enum class HttpLogLevel { /** * Headers plus the request/response body. The request body is captured during write * via [org.dexpace.sdk.core.http.request.LoggableRequestBody]; the response body is - * drained eagerly into [org.dexpace.sdk.core.http.response.LoggableResponseBody]. + * captured into [org.dexpace.sdk.core.http.response.LoggableResponseBody]. * - * Not suitable for very large streamed responses — the entire body is buffered in memory. + * Capture is **bounded** to the configured preview size (`bodyPreviewMaxBytes`), so large + * or streaming responses are not buffered whole — the caller still streams the remainder. + * See [HttpInstrumentationOptions] for the streaming and async-completion-thread caveats. */ BODY_AND_HEADERS, } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBody.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBody.kt index a252ef6f..b8589dba 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBody.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBody.kt @@ -48,6 +48,13 @@ import java.io.IOException * [toReplayable] returns a new `LoggableRequestBody` wrapping the delegate's replayable * form, so retry loops continue to see captured bytes for every attempt. * + * ## Tap cap + * + * By default the entire write is mirrored into the tap. The instrumentation steps construct a + * [bounded] wrapper that caps how many bytes are mirrored (via the [TeeSink] tap limit), so a + * multi-GB `FileRequestBody` upload mirrors only a bounded preview into memory while still + * streaming zero-copy to the transport. The full payload always reaches the transport sink. + * * ## Thread safety * * Not thread-safe. One writer at a time. @@ -57,11 +64,21 @@ import java.io.IOException * installed provider. */ public class LoggableRequestBody - @JvmOverloads - constructor( + internal constructor( private val delegate: RequestBody, - private val provider: IoProvider = Io.provider, + private val provider: IoProvider, + private val tapLimit: Long, ) : RequestBody() { + /** + * Public surface: a wrapper that mirrors the entire write into the tap (unbounded). + * Equivalent to `bounded(delegate, provider, Long.MAX_VALUE)`. + */ + @JvmOverloads + public constructor( + delegate: RequestBody, + provider: IoProvider = Io.provider, + ) : this(delegate, provider, Long.MAX_VALUE) + // Deferred so a wrapper that is constructed but never written/snapshot'd doesn't allocate // a Buffer. NONE thread-safety mode matches the documented "not thread-safe" contract. private val tap: Buffer by lazy(LazyThreadSafetyMode.NONE) { provider.buffer() } @@ -76,7 +93,8 @@ public class LoggableRequestBody if (delegate.isReplayable()) { this } else { - LoggableRequestBody(delegate.toReplayable(provider), provider) + // Preserve the configured tap cap across the replayable rewrite. + LoggableRequestBody(delegate.toReplayable(provider), provider, tapLimit) } /** @@ -88,7 +106,7 @@ public class LoggableRequestBody @Throws(IOException::class) override fun writeTo(sink: BufferedSink) { tap.clear() - val tee = TeeSink(sink, tap, provider) + val tee = TeeSink(sink, tap, provider, tapLimit) delegate.writeTo(tee) } @@ -114,4 +132,14 @@ public class LoggableRequestBody if (tap.size <= cap) return tap.snapshot() return tap.peek().readByteArray(cap.toLong()) } + + internal companion object { + /** Internal seam: a wrapper that caps the request-side tap at [tapLimit] bytes. */ + @JvmSynthetic + internal fun bounded( + delegate: RequestBody, + provider: IoProvider, + tapLimit: Long, + ): LoggableRequestBody = LoggableRequestBody(delegate, provider, tapLimit) + } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt index 518ce5fb..f5aea4f1 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt @@ -26,6 +26,13 @@ import java.net.URL * Instances are immutable and safe to share across threads. The [body], when present, may * carry single-use stream state — see [RequestBody] for that contract. * + * ## Equality + * + * [url] participates in equality by its external form ([URL.toExternalForm]) — a pure string + * comparison that performs **no** network I/O. `java.net.URL.equals`/`hashCode` resolve the host + * via DNS (blocking, and wrong for virtual hosts that share an address), so this type compares + * URLs textually instead. The remaining fields ([method], [headers], [body]) compare by value. + * * @property method HTTP method on the wire. * @property url Fully-resolved target URL. * @property headers Request headers; may be empty but never `null`. @@ -38,6 +45,31 @@ public data class Request private constructor( val headers: Headers, val body: RequestBody?, ) { + /** + * Compares by value. [url] is compared by its external form (a pure string, no DNS lookup); + * [method], [headers], and [body] compare structurally as the generated equality would. + */ + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is Request) return false + return method == other.method && + url.toExternalForm() == other.url.toExternalForm() && + headers == other.headers && + body == other.body + } + + /** + * Hash consistent with [equals]: [url] hashes by its external form (no DNS lookup), the rest + * by value. + */ + override fun hashCode(): Int { + var result = method.hashCode() + result = HASH_MULTIPLIER * result + url.toExternalForm().hashCode() + result = HASH_MULTIPLIER * result + headers.hashCode() + result = HASH_MULTIPLIER * result + (body?.hashCode() ?: 0) + return result + } + /** * Returns a new [RequestBuilder] initialized with this request's data. * @@ -227,6 +259,8 @@ public data class Request private constructor( } public companion object { + private const val HASH_MULTIPLIER = 31 + /** * Returns a fresh empty [RequestBuilder]. Java-friendly entry point matching the * `Request.builder()` idiom. diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBody.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBody.kt index 586181b7..6fbeb66b 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBody.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBody.kt @@ -12,21 +12,31 @@ import org.dexpace.sdk.core.io.Buffer import org.dexpace.sdk.core.io.BufferedSource import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.io.IoProvider +import org.dexpace.sdk.core.io.Source import java.io.IOException +import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.locks.ReentrantLock import kotlin.concurrent.withLock /** - * Wraps a [ResponseBody] to provide repeatable reads. + * Wraps a [ResponseBody] to provide repeatable reads of a bounded capture prefix. * - * On the first call to [source], [snapshot], or [captureException], the wrapped body is - * drained into an internal [Buffer] using a single `writeAll` (segment-transferring on - * Okio-backed providers). Subsequent calls to [source] return a fresh non-consuming view of - * the same bytes, and [snapshot] returns the captured bytes for logging. + * On the first call to [source], [snapshot], or [captureException], up to `maxCaptureBytes` + * bytes of the wrapped body are drained into an internal [Buffer]. Two regimes follow: * - * The wrapped body is closed after draining (success or failure). [close] on this wrapper - * is idempotent and stops further drains; the captured buffer survives close so post-mortem - * snapshots remain available. + * - **Body fits within the cap** (the default, unlimited path): the whole body is captured, + * the delegate is closed, and every subsequent [source] call returns a fresh non-consuming + * view (`peek()`) of the same bytes — fully repeatable, exactly as before. + * - **Body exceeds the cap**: only the prefix is buffered and the delegate is **left open**. + * [source] then returns a **one-shot** stream that first replays the captured prefix and + * then continues from the still-live delegate tail, so the caller receives the complete + * body. Because the tail can be read only once, calling [source] a second time on an + * over-cap body throws [IllegalStateException]. + * + * The default `maxCaptureBytes` is [Long.MAX_VALUE], so the unlimited (fully-repeatable) + * behavior is preserved for callers that construct the wrapper directly. The instrumentation + * steps construct a [bounded] wrapper capped at the configured body-preview size so logging + * never buffers more than a preview into memory. * * ## Failure semantics * @@ -42,28 +52,39 @@ import kotlin.concurrent.withLock * * ## Memory * - * The whole body is buffered in memory. Only suitable for bodies that fit comfortably in - * RAM. For multi-MB bodies whose only consumer is logging, prefer [snapshot] with a - * `maxBytes` cap. + * At most `maxCaptureBytes` bytes are buffered in memory. The default unlimited cap buffers + * the whole body, so the default path is only suitable for bodies that fit comfortably in + * RAM. For multi-MB or streaming bodies whose only consumer is logging, use [bounded] (the + * instrumentation steps do) so memory stays bounded while the caller still streams the rest. * * ## Thread safety * * The drain itself is single-fire — concurrent first calls to [source]/[snapshot] are - * serialized on an internal lock so the upstream is read exactly once. After drain, the + * serialized on an internal lock so the upstream is read exactly once. After a full drain the * captured buffer is read-only from this wrapper's perspective; concurrent readers each * receive a separate `peek()` view, which is safe as long as no external code mutates the - * captured buffer. + * captured buffer. The one-shot over-cap [source] is single-consumer by contract. * * @param delegate Body whose bytes will be captured and re-served. * @param provider I/O provider used to allocate the capture [Buffer]. Defaults to the * globally installed provider. */ public class LoggableResponseBody - @JvmOverloads - constructor( + internal constructor( private val delegate: ResponseBody, - private val provider: IoProvider = Io.provider, + private val provider: IoProvider, + private val maxCaptureBytes: Long, ) : ResponseBody() { + /** + * Public surface: an unbounded wrapper that captures the entire body (fully repeatable). + * Equivalent to `bounded(delegate, provider, Long.MAX_VALUE)`. + */ + @JvmOverloads + public constructor( + delegate: ResponseBody, + provider: IoProvider = Io.provider, + ) : this(delegate, provider, Long.MAX_VALUE) + private val lock = ReentrantLock() @Volatile @@ -75,31 +96,61 @@ public class LoggableResponseBody @Volatile private var closed = false - // Set after a successful drain: the drain path closes the source via `.use {}`, so a - // subsequent close() must not close the delegate again — some sockets / streams throw - // on double-close. + // True once the whole body fit within the cap and was drained into [captured]; the drain + // path closes the source via `.use {}` in that case, so [source] can hand out repeatable + // peek() views and close() must not double-close the delegate. + @Volatile + private var fullyCaptured = false + + // Set when the cap was hit with bytes still pending: the live delegate source is retained + // so the one-shot [source] can replay the captured prefix then continue from this tail. + @Volatile + private var liveTail: BufferedSource? = null + + // Guards the single-consumer contract for the over-cap one-shot source. + private val tailHandedOut = AtomicBoolean(false) + + // Set after the delegate (and, by ownership, its source) has been closed, so a subsequent + // close() must not close it again — some sockets / streams throw on double-close. @Volatile private var delegateClosed = false override fun mediaType(): MediaType? = delegate.mediaType() - override fun contentLength(): Long = captured?.size ?: delegate.contentLength() + /** + * Returns the captured size only when the body was fully captured within the cap; + * otherwise the delegate's reported length (the true length), since the capture is just + * a bounded prefix. + */ + override fun contentLength(): Long = + if (fullyCaptured) captured?.size ?: delegate.contentLength() else delegate.contentLength() /** - * Returns a fresh non-consuming view of the captured body. Drains the wrapped body on - * first call. If the drain failed (partial capture), this method re-throws the captured - * exception every time — callers see the failure rather than a silent empty source. + * Returns a view of the captured body. Drains (up to the cap) on first call. If the drain + * failed (partial capture), this method re-throws the captured exception every time — + * callers see the failure rather than a silent empty source. + * + * When the body fit within the cap the returned source is a fresh non-consuming `peek()` + * view and may be requested repeatedly. When the body exceeded the cap the returned source + * is a **one-shot** stream that replays the captured prefix then continues from the live + * delegate tail; a second call on an over-cap body throws [IllegalStateException]. */ override fun source(): BufferedSource { val buf = ensureCaptured() drainError?.let { throw it.asIOException() } - return buf.peek() + if (fullyCaptured) return buf.peek() + check(tailHandedOut.compareAndSet(false, true)) { + "LoggableResponseBody capture exceeded maxCaptureBytes; source() over the live tail " + + "is single-use and was already consumed." + } + val tail = liveTail ?: return buf.peek() + return provider.bufferedSource(PrefixThenTailSource(buf.peek(), tail)) } /** - * Returns the captured body bytes. Returns whatever was read before any drain failure — - * a drain error does not prevent this method from returning the partial capture. Use - * [captureException] alongside this method when you need to know whether the capture + * Returns the captured (bounded) body bytes. Returns whatever was read before any drain + * failure — a drain error does not prevent this method from returning the partial capture. + * Use [captureException] alongside this method when you need to know whether the capture * was complete. * * @throws IllegalStateException if the captured body exceeds [Buffer.MAX_BYTE_ARRAY_SIZE]. @@ -108,7 +159,7 @@ public class LoggableResponseBody public fun snapshot(): ByteArray = ensureCaptured().snapshot() /** - * Returns up to [maxBytes] bytes of the captured body. Use for log previews of large + * Returns up to [maxBytes] bytes of the captured prefix. Use for log previews of large * bodies — avoids materializing the entire body as a `ByteArray`. The captured buffer * itself remains intact. Returns the partial prefix on failure (does not throw). * @@ -134,9 +185,11 @@ public class LoggableResponseBody lock.withLock { if (closed) return closed = true - // The drain path closes the underlying source via `.use {}` on success; in that - // case we must not close the delegate again — some sockets / streams throw on - // double-close. + // The drain path closes the underlying source on a full capture; in that case we + // must not close the delegate again — some sockets / streams throw on double-close. + // On an over-cap capture the delegate is still open and must be closed here; the + // delegate owns the live-tail source, so closing the delegate releases it too (we + // must NOT close the live tail separately, or the source would be closed twice). if (!delegateClosed) { delegate.close() delegateClosed = true @@ -158,17 +211,22 @@ public class LoggableResponseBody } /** - * Drains the delegate body into an in-memory [Buffer] and caches the result. + * Drains at most [maxCaptureBytes] bytes of the delegate body into an in-memory [Buffer] + * and caches the result. + * + * The capture buffer is allocated inside the try block so that a provider failure is also + * caught and cached in [drainError]. When `delegate.source()` throws before the read loop + * the source was never obtained, so the delegate remains open and a subsequent [close] + * will still close it. * - * The capture buffer is allocated inside the try block so that a provider failure - * is also caught and cached in [drainError]. When `delegate.source()` throws before - * `.use {}` is entered the source was never obtained, so the delegate remains open - * and a subsequent [close] will still close it — `delegateClosed` is only set to - * `true` when we know the source ownership chain has already been closed via `.use {}`. + * If EOF is reached within the cap the body is fully captured: the source is closed and + * [fullyCaptured] is set, preserving the fully-repeatable behavior. If the cap is hit with + * bytes still pending the delegate is **left open** and retained as [liveTail] so the + * consumer still receives the rest of the body via [source]. * - * If `provider.buffer()` throws (rare; would indicate a misconfigured provider), - * the error is cached in [drainError] and an empty buffer is used as the fallback - * capture so [captured] is never left null. + * If `provider.buffer()` throws (rare; would indicate a misconfigured provider), the error + * is cached in [drainError] and an empty buffer is used as the fallback capture so + * [captured] is never left null. */ private fun drainAndCache(): Buffer { var buf: Buffer? = null @@ -176,14 +234,42 @@ public class LoggableResponseBody try { buf = provider.buffer() src = delegate.source() // may throw — if it does, delegate is NOT yet closed - src.use { buf.writeAll(it) } // closes src (and via ownership the delegate) on exit - delegateClosed = true + val capturedSource = src + var remaining = maxCaptureBytes + while (remaining > 0L) { + val chunk = if (remaining < READ_CHUNK_BYTES) remaining else READ_CHUNK_BYTES + val n = capturedSource.read(buf, chunk) + if (n == -1L) { + // EOF within the cap — fully captured. + fullyCaptured = true + break + } + remaining -= n + } + if (fullyCaptured) { + // Whole body captured: close the source (and via ownership the delegate). + capturedSource.close() + delegateClosed = true + } else { + // The cap was hit (or maxCaptureBytes was <= 0) with bytes still pending: retain + // the live tail so the consumer still gets them. Do NOT close the delegate. + liveTail = capturedSource + } } catch (t: Throwable) { // Keep whatever was read before the failure so logging can still inspect partial bytes. drainError = t - // If src was obtained, .use{} already closed it (and the delegate via ownership). - // If src is null, delegate.source() threw — delegate stays open; close() will close it. - if (src != null) delegateClosed = true + // If the source was obtained but a read failed, close it here so the network + // resource is not leaked, and mark the delegate closed so a later close() does not + // double-close. If `delegate.source()` itself threw, `src` is null and the delegate + // remains the caller's to close via close(). + if (src != null) { + try { + src.close() + } catch (_: Throwable) { + // Best-effort: the drain already failed; a close failure must not mask it. + } + delegateClosed = true + } } // Ensure captured is always non-null even when buf allocation itself failed. // The second provider.buffer() call here is only reached when the first one threw @@ -196,4 +282,44 @@ public class LoggableResponseBody private fun Throwable.asIOException(): IOException = if (this is IOException) this else IOException("Body capture failed", this) + + /** + * A one-shot [Source] that yields the captured [prefix] bytes first, then continues from + * the live [tail]. Used only on the over-cap path; closing it closes the tail. + */ + private class PrefixThenTailSource( + private val prefix: BufferedSource, + private val tail: BufferedSource, + ) : Source { + @Throws(IOException::class) + override fun read( + sink: Buffer, + byteCount: Long, + ): Long { + if (!prefix.exhausted()) return prefix.read(sink, byteCount) + return tail.read(sink, byteCount) + } + + @Throws(IOException::class) + override fun close() { + try { + prefix.close() + } finally { + tail.close() + } + } + } + + internal companion object { + /** Per-read pump size for the bounded drain — matches Okio's segment size. */ + private const val READ_CHUNK_BYTES: Long = 8 * 1024 + + /** Internal seam: a wrapper that caps in-memory capture at [maxCaptureBytes] bytes. */ + @JvmSynthetic + internal fun bounded( + delegate: ResponseBody, + provider: IoProvider, + maxCaptureBytes: Long, + ): LoggableResponseBody = LoggableResponseBody(delegate, provider, maxCaptureBytes) + } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/ResponseBody.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/ResponseBody.kt index ff6f62ef..e01f4244 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/ResponseBody.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/ResponseBody.kt @@ -31,7 +31,7 @@ import java.io.InputStream * * Instances are **not** thread-safe. The stream returned by [byteStream] should be read * from a single thread only. For concurrent access, wrap with - * [LoggableResponseBody][org.dexpace.sdk.core.http.logging.LoggableResponseBody] which + * [LoggableResponseBody] which * buffers the content and provides thread-safe, repeatable reads. * * ## Single-use contract @@ -40,7 +40,7 @@ import java.io.InputStream * every call, and once consumed, the bytes are gone. Use [bytes] or [string] for a * one-shot read, or wrap with `LoggableResponseBody` for repeatable access. * - * @see org.dexpace.sdk.core.http.logging.LoggableResponseBody for a buffered wrapper that + * @see LoggableResponseBody for a buffered wrapper that * supports repeatable reads and non-destructive logging. */ public abstract class ResponseBody : Closeable { diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/io/TeeSink.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/io/TeeSink.kt index ca055954..8edc66c9 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/io/TeeSink.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/io/TeeSink.kt @@ -40,15 +40,27 @@ import java.nio.charset.Charset * The result is one encoding step plus two segment-level operations, regardless of payload * size. The previous implementation encoded each string twice and allocated a fresh * `ByteArray` per `writeAll` chunk. + * + * ## Tap cap + * + * [tapLimit] bounds how many bytes are mirrored into [tap]. Once that many bytes have been + * tapped, further writes stop copying into [tap] entirely while still forwarding the **full** + * payload to [primary] — the wire body is never truncated. The default [tapLimit] is + * [Long.MAX_VALUE] (unbounded mirroring). `LoggableRequestBody` caps this so a multi-GB upload + * mirrors only a bounded preview into the tap while streaming zero-copy to the transport. */ internal class TeeSink( private val primary: BufferedSink, private val tap: Buffer, private val provider: IoProvider, + private val tapLimit: Long = Long.MAX_VALUE, ) : BufferedSink { /** Reusable staging area — keeps every typed write to a single encode + segment-move. */ private val scratch: Buffer = provider.buffer() + /** Bytes mirrored into [tap] so far; mirroring stops once this reaches [tapLimit]. */ + private var mirrored: Long = 0L + /** * Direct buffer access is unsupported on a `TeeSink`: writes into the buffer would only * reach the tap, never the primary sink, silently corrupting the wire body. Use the @@ -65,11 +77,29 @@ internal class TeeSink( source: Buffer, byteCount: Long, ) { - // Mirror the prefix into tap (non-destructive), then drain into primary (destructive). - source.copyTo(tap, 0, byteCount) + // Mirror up to the remaining tap budget (non-destructive), then drain the FULL count into + // primary (destructive) — the wire body is never truncated. + mirrorPrefix(source, byteCount) primary.write(source, byteCount) } + /** + * Copies the leading bytes of [source] into [tap] up to the remaining [tapLimit] budget, + * advancing [mirrored]. Once the budget is exhausted this is a no-op. [source] itself is + * left intact for the subsequent destructive drain into [primary]. + */ + @Throws(IOException::class) + private fun mirrorPrefix( + source: Buffer, + byteCount: Long, + ) { + val allowed = (tapLimit - mirrored).coerceAtLeast(0L) + if (allowed == 0L) return + val copy = if (byteCount < allowed) byteCount else allowed + source.copyTo(tap, 0, copy) + mirrored += copy + } + @Throws(IOException::class) override fun flush() { primary.flush() @@ -151,10 +181,13 @@ internal class TeeSink( val tapStream = tap.outputStream() return object : OutputStream() { override fun write(b: Int) { - // Mirror into the tap FIRST, then forward to the primary — same ordering as the - // typed-write path's `drainScratch`. If the primary side throws mid-write, the - // attempted byte is still captured in the tap snapshot for body logging. - tapStream.write(b) + // Mirror into the tap FIRST (within the budget), then forward the byte to the + // primary — same ordering as the typed-write path's `drainScratch`. If the primary + // side throws mid-write, the attempted byte is still captured in the tap snapshot. + if (mirrored < tapLimit) { + tapStream.write(b) + mirrored += 1L + } primaryStream.write(b) } @@ -163,9 +196,15 @@ internal class TeeSink( off: Int, len: Int, ) { - // Tap first, primary second (see single-byte overload): a primary-side failure - // leaves the failing chunk captured in the tap. - tapStream.write(b, off, len) + // Tap first (within the budget), primary second (see single-byte overload): a + // primary-side failure leaves the failing chunk captured in the tap. The FULL + // chunk is always forwarded to the primary so the wire body is never truncated. + val allowed = (tapLimit - mirrored).coerceAtLeast(0L) + if (allowed > 0L) { + val copy = if (len.toLong() < allowed) len else allowed.toInt() + tapStream.write(b, off, copy) + mirrored += copy.toLong() + } primaryStream.write(b, off, len) } @@ -201,7 +240,7 @@ internal class TeeSink( val staged = scratch.size if (staged == 0L) return try { - scratch.copyTo(tap, 0, staged) + mirrorPrefix(scratch, staged) primary.write(scratch, staged) } finally { // Ensure scratch is empty even if the copy/write threw, otherwise the next diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipeline.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipeline.kt index da2eceb4..1f10bf24 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipeline.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipeline.kt @@ -8,6 +8,7 @@ package org.dexpace.sdk.core.pipeline import org.dexpace.sdk.core.http.context.DispatchContext +import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.pipeline.step.ResponsePipelineStep import org.dexpace.sdk.core.pipeline.step.ResponseRecoveryStep import java.util.Collections @@ -90,14 +91,21 @@ public class ResponsePipeline var current = outcome for (step in responseSteps) { current = - when (current) { - is ResponseOutcome.Success -> + when (val inbound = current) { + is ResponseOutcome.Success -> { + // Capture the in-hand response before the step runs so the catch can + // release it. A throwing step would otherwise strand the open transport + // connection — mirrors the close-before-propagate discipline in + // DefaultRedirectStep / AuthStep. + val inResponse = inbound.response try { - ResponseOutcome.Success(step.execute(current.response, context)) + ResponseOutcome.Success(step.execute(inResponse, context)) } catch (t: Throwable) { + closeQuietly(inResponse, t) handleStepThrowable(t) } - is ResponseOutcome.Failure -> return current + } + is ResponseOutcome.Failure -> return inbound } } return current @@ -114,6 +122,11 @@ public class ResponsePipeline try { step.invoke(outcome) } catch (t: Throwable) { + // A recovery step that throws on a Success outcome strands the in-hand response: + // close it before wrapping the throwable so the open connection is released. + if (outcome is ResponseOutcome.Success) { + closeQuietly(outcome.response, t) + } handleStepThrowable(t) } @@ -127,4 +140,19 @@ public class ResponsePipeline } return ResponseOutcome.Failure(t) } + + /** + * Closes [response], swallowing any close error so it never masks [primary]. A failure to + * close is attached to [primary] as a suppressed throwable for diagnostics. + */ + private fun closeQuietly( + response: Response, + primary: Throwable, + ) { + try { + response.close() + } catch (closeError: Throwable) { + primary.addSuppressed(closeError) + } + } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt index f5e52803..8d50c8ec 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt @@ -23,8 +23,6 @@ import java.util.concurrent.ExecutionException import java.util.concurrent.Executors import java.util.concurrent.ScheduledExecutorService import java.util.concurrent.TimeUnit -import java.util.concurrent.locks.ReentrantLock -import kotlin.concurrent.withLock /** * Recovery-aware retry step that re-executes a failed request with exponential backoff, @@ -44,13 +42,19 @@ import kotlin.concurrent.withLock * * ## Idempotency * - * A request is eligible for retry when either: - * - `request.method ∈ retryableMethods`, **or** - * - `request.body?.isReplayable() == true` (body can be re-sent verbatim). + * Eligibility is gated on re-sendability, split by whether the request carries a body: + * - **No body** — eligible only when `request.method ∈ retryableMethods` (the method is + * idempotent). A body-less non-idempotent request (e.g. a bare POST) is not retried. + * - **Has a body** — eligible only when `request.body.isReplayable() == true`. A non-replayable + * body physically cannot be re-sent: its consume-once guard would trip on the second + * `writeTo` and mask the original failure with an [IllegalStateException]. A replayable body + * is re-sendable, so the request is retried; ensuring the re-sent request is idempotent (for + * a non-idempotent method, e.g. via an idempotency key) is the caller's responsibility. * - * Non-idempotent methods (POST/PATCH) with non-replayable bodies are NOT retried. This is a - * deliberate departure from Square's `RetryInterceptor`, which retries on status alone and - * silently double-sends POSTs on transport timeouts. + * The body-replayability gate closes the hazard where an idempotent method carrying a one-shot + * body slipped through a method-only check and tripped the consume-once guard on resend. This + * is a deliberate departure from Square's `RetryInterceptor`, which retries on status alone and + * silently double-sends a body it cannot replay on transport timeouts. * * ## Cancellation * @@ -70,8 +74,9 @@ import kotlin.concurrent.withLock * not held on the instance; it is allocated fresh inside each top-level [invoke]/[attempt] * and threaded through the retry loop as a local [AttemptState]. Two threads invoking the * same instance, or the same thread invoking it again after a prior attempt was exhausted, - * each start from a clean attempt budget. The only mutable instance field is the lazy-init - * guard for the default scheduler, which is itself lock-protected. + * each start from a clean attempt budget. The instance holds no mutable fields at all: the + * process-wide default scheduler is a companion `by lazy` (SYNCHRONIZED), created at most once + * VM-wide, so no per-instance guard is needed. * * The [httpClient] and the originating [request] are captured at construction; a single * instance therefore retries exactly that one request template. Construct a distinct @@ -93,9 +98,6 @@ public class RetryStep public val request: Request, private val clock: Clock = Clock.systemUTC(), ) : ResponseRecoveryStep { - /** Lock guards lazy-init of the default scheduler so we never create two. */ - private val lock: ReentrantLock = ReentrantLock() - /** * Per-call retry bookkeeping. Allocated fresh at the top of each [invoke]/[attempt] so * a shared/reused [RetryStep] never inherits a stale attempt budget and concurrent @@ -320,14 +322,13 @@ public class RetryStep } /** - * Returns the [settings]-supplied scheduler, or lazily creates the process-wide - * daemon scheduler. The lazy init runs at most once per RetryStep instance via the - * `lock`; the process-wide scheduler itself is initialised at most once across the - * whole VM via the companion `by lazy`. + * Returns the [settings]-supplied scheduler, or the process-wide daemon scheduler. The + * process-wide scheduler is a companion `by lazy` (SYNCHRONIZED), so it is initialised + * at most once across the whole VM — no per-instance guard is involved. */ private fun resolveScheduler(): ScheduledExecutorService { settings.scheduler?.let { return it } - return lock.withLock { DEFAULT_SCHEDULER } + return DEFAULT_SCHEDULER } /** @@ -342,13 +343,14 @@ public class RetryStep } /** - * Returns true when [request] is safe to retry — its method is in - * [RetrySettings.retryableMethods] OR its body is replayable. A non-idempotent - * method with a non-replayable body cannot be safely re-sent. + * Returns true when [request] is safe to retry. A body-less request is retry-safe only + * when its method is in [RetrySettings.retryableMethods] (idempotent); a body-bearing + * request is retry-safe only when its body is replayable — a non-replayable body cannot + * be re-sent (the second `writeTo` trips the consume-once guard). Ensuring a re-sent + * body-bearing request is idempotent is the caller's responsibility. */ private fun canRetry(request: Request): Boolean { - if (settings.retryableMethods.contains(request.method)) return true - val body = request.body ?: return true + val body = request.body ?: return settings.retryableMethods.contains(request.method) return body.isReplayable() } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/ProxyOptions.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/ProxyOptions.kt index 3fe9e6a2..d334aca1 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/ProxyOptions.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/ProxyOptions.kt @@ -18,16 +18,23 @@ import java.util.regex.Pattern * Runtime carrier for HTTP proxy configuration. * * Holds the proxy [Type] and [address], an optional list of [nonProxyHosts] glob patterns, - * and either inline [username] / [password] credentials or a pluggable [challengeHandler] for - * Digest proxy authentication. [nonProxyHosts] are compiled once at construction so per-request - * [bypassesProxy] lookups don't re-compile. + * inline [username] / [password] credentials, and a [challengeHandler] slot. [nonProxyHosts] + * are compiled once at construction so per-request [bypassesProxy] lookups don't re-compile. * * Instances are immutable. Construct directly for explicit configuration, or use * [fromConfiguration] to pull settings from JVM system properties and standard environment * variables (`HTTPS_PROXY`, `HTTP_PROXY`, `NO_PROXY`). * - * When both [username] / [password] and [challengeHandler] are supplied, [challengeHandler] - * wins — it is strictly more flexible (Digest support). + * ## Proxy authentication + * + * Proxy auth is driven by [username] / [password]. The shipped transports apply them as follows: + * - OkHttp transport: sets up **Basic** proxy authentication from [username] / [password]. + * - JDK transport: passes [username] / [password] to the `java.net.http` stack, which negotiates + * **Basic** or **Digest** with the proxy itself. + * + * [challengeHandler] is **currently not honoured by any shipped transport** — it is reserved for + * a future pluggable proxy-auth mechanism. Setting it has no effect today (the transports ignore + * it and log a warning), so supply [username] / [password] for proxy authentication. * * ## Bypass-all semantics (breaking change from pre-v2 API) * diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/client/AsyncHttpClientTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/client/AsyncHttpClientTest.kt index 0f949fab..6c19dfad 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/client/AsyncHttpClientTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/client/AsyncHttpClientTest.kt @@ -13,14 +13,19 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.http.response.Status import java.io.IOException +import java.io.InterruptedIOException import java.net.URL import java.util.concurrent.CompletableFuture +import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicReference import kotlin.test.AfterTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFails +import kotlin.test.assertTrue class AsyncHttpClientTest { private val executor = Executors.newSingleThreadExecutor() @@ -52,7 +57,7 @@ class AsyncHttpClientTest { } @Test - fun `asBlocking returns a sync client that unwraps the CompletionException`() { + fun `asBlocking returns a sync client that completes normally`() { val asyncClient = AsyncHttpClient { request -> CompletableFuture.completedFuture(mockResponse(request, 204)) @@ -63,7 +68,7 @@ class AsyncHttpClientTest { } @Test - fun `asBlocking surfaces the original exception, not the CompletionException wrapper`() { + fun `asBlocking surfaces the original exception, not the ExecutionException wrapper`() { val sentinel = IOException("network") val asyncClient = AsyncHttpClient { @@ -75,6 +80,40 @@ class AsyncHttpClientTest { assertEquals(sentinel.message, thrown.message) } + @Test + fun `asBlocking honours interruption, throwing InterruptedIOException with the flag set`() { + // A future that never completes, so asBlocking parks in get() until interrupted. + val never = CompletableFuture() + val asyncClient = AsyncHttpClient { never } + val syncClient = asyncClient.asBlocking() + + val started = CountDownLatch(1) + val thrown = AtomicReference() + val flagSet = AtomicBoolean(false) + val worker = + Thread { + started.countDown() + try { + syncClient.execute(getRequest()) + } catch (t: Throwable) { + thrown.set(t) + flagSet.set(Thread.currentThread().isInterrupted) + } + } + worker.start() + // Wait for the worker to reach the blocking call, then interrupt it. + assertTrue(started.await(2, TimeUnit.SECONDS)) + // Give it a moment to park in get() before interrupting. + Thread.sleep(50) + worker.interrupt() + worker.join(2_000) + + val failure = thrown.get() + assertTrue(failure is InterruptedIOException, "expected InterruptedIOException, got $failure") + assertTrue(flagSet.get(), "interrupt flag must be restored on the calling thread") + assertTrue(never.isCancelled, "the in-flight future must be cancelled on interruption") + } + @Test fun `round-trip via asAsync then asBlocking preserves the response`() { val syncClient = HttpClient { request -> mockResponse(request, 201) } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/auth/AuthChallengeParserTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/auth/AuthChallengeParserTest.kt index e1df3741..3d671650 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/auth/AuthChallengeParserTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/auth/AuthChallengeParserTest.kt @@ -394,6 +394,24 @@ class AuthChallengeParserTest { assertTrue(challenges[0].parameters.isEmpty()) } + @Test + fun `malformed continuation after a param does not emit a phantom challenge`() { + // Regression guard: `Digest realm="x", foo=)Bearer abc123`. After `foo=`, the value + // begins with `)`, which is not a valid token/quoted-string start, so the param read + // bails out. The bail-out leaves the cursor at a non-token char (or EOF), which the + // outer recovery absorbs WITHOUT emitting a spurious challenge — in particular, no + // phantom `bearer` challenge is produced from the trailing `Bearer abc123` text. + // We must get exactly the one well-formed Digest challenge. + val challenges = AuthChallengeParser.parse("""Digest realm="x", foo=)Bearer abc123""") + assertEquals(1, challenges.size, "only the leading Digest challenge should be emitted") + assertEquals("digest", challenges[0].scheme) + assertEquals("x", challenges[0].parameters["realm"]) + assertTrue( + challenges.none { it.scheme == "bearer" }, + "the malformed continuation must not produce a phantom bearer challenge", + ) + } + @Test fun `quote with only a backslash inside before close quote`() { // `"\"` — opens, sees backslash, advances, sees `"` (the close), appends diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt index f9b82ea4..72e5b2be 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/HeadersTest.kt @@ -158,6 +158,106 @@ class HeadersTest { assertEquals(listOf("new=1", "new=2"), headers.values(HttpHeaderName.SET_COOKIE)) } + // ---- value validation (request/header-splitting guard) ---------------------- + + @Test + fun `add rejects a value containing a line feed`() { + assertFailsWith { + Headers.builder().add("X-Evil", "ok\nInjected: 1") + } + } + + @Test + fun `add rejects a value containing a carriage return`() { + assertFailsWith { + Headers.builder().add("X-Evil", "ok\rInjected: 1") + } + } + + @Test + fun `set rejects a value containing a line feed`() { + assertFailsWith { + Headers.builder().set("X-Evil", "ok\nInjected: 1") + } + } + + @Test + fun `set rejects a value containing a carriage return`() { + assertFailsWith { + Headers.builder().set("X-Evil", "ok\rInjected: 1") + } + } + + @Test + fun `add list overload rejects any value containing CR or LF`() { + assertFailsWith { + Headers.builder().add("X-Evil", listOf("fine", "bad\nvalue")) + } + } + + @Test + fun `set list overload rejects any value containing CR or LF`() { + assertFailsWith { + Headers.builder().set("X-Evil", listOf("bad\rvalue")) + } + } + + @Test + fun `typed add rejects a value containing a line feed`() { + assertFailsWith { + Headers.builder().add(HttpHeaderName.SET_COOKIE, "a=1\nInjected: 1") + } + } + + @Test + fun `typed set rejects a value containing a line feed`() { + assertFailsWith { + Headers.builder().set(HttpHeaderName.SET_COOKIE, "a=1\nInjected: 1") + } + } + + @Test + fun `the rejection message names the offending header`() { + val thrown = + assertFailsWith { + Headers.builder().add("X-Trace-Id", "ok\nbad") + } + assertTrue( + thrown.message?.lowercase()?.contains("x-trace-id") == true, + "message should name the header, got: ${thrown.message}", + ) + } + + @Test + fun `normal values without CR or LF are accepted`() { + val headers = + Headers.builder() + .add("X-Plain", "hello world") + .set("Authorization", "Bearer abc.def-ghi") + .add(HttpHeaderName.SET_COOKIE, "id=42; Path=/") + // Tabs and UTF-8 are not CR/LF, so they must pass the conservative check. + .set("X-Unicode", "café\tvalue") + .build() + + assertEquals("hello world", headers.get("X-Plain")) + assertEquals("Bearer abc.def-ghi", headers.get("Authorization")) + assertEquals("id=42; Path=/", headers.get(HttpHeaderName.SET_COOKIE)) + assertEquals("café\tvalue", headers.get("X-Unicode")) + } + + @Test + fun `set with null still removes even though value validation is in place`() { + // Removal must not run value validation — null is the documented removal signal. + val headers = + Headers.builder() + .set("X-Trace-Id", "trace-1") + .set("X-Trace-Id", null) + .build() + + assertFalse(headers.contains("X-Trace-Id")) + assertNull(headers.get("X-Trace-Id")) + } + // ---- accessors & equality coverage ------------------------------------------ @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/ContextStoreTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/ContextStoreTest.kt index 55d94b3f..a8ab0d21 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/ContextStoreTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/ContextStoreTest.kt @@ -221,6 +221,34 @@ class ContextStoreTest { assertEquals(calls, survivors.get(), "no call should have its entry overwritten by another") } + @Test + fun `a burst of distinct keys past the cap keeps the live set bounded`() { + // The store is bounded: after each insert it drains back under MAX_TRACKED_CONTEXTS, + // so a missed close() on an exception path can leak at most the cap, not unboundedly. + // We can't read the private map size, so we assert the observable consequence: after + // inserting well past the cap, no more than `cap` of OUR keys can still be present. + // (We insert a fresh, isolated batch so no other test's entries inflate the count.) + val cap = 4096 + val burst = cap + 500 + val inserted = ArrayList(burst) + for (i in 0 until burst) { + val id = owned("burst-$i") + inserted.add(id) + ContextStore.set(id, DispatchContext(FakeInstrumentationContext(TraceId(id)), id)) + } + + val survivors = inserted.count { ContextStore.get(it) != null } + assertTrue( + survivors <= cap, + "store must stay bounded at the cap; $survivors of our keys survived (cap=$cap)", + ) + // The most-recently inserted key must still be present: draining drops arbitrary + // victims after the insert, but the just-inserted entry is the last thing written and + // the drain only runs while size exceeds the cap, so the live entry we just added + // survives its own insert. + assertTrue(survivors > 0, "the store should retain entries up to its cap") + } + @Test fun `concurrent put for the same id only lets one winner through`() { val id = owned("concurrent") diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/paging/PagedIterableTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/paging/PagedIterableTest.kt index 36ff1943..d8205dad 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/paging/PagedIterableTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/paging/PagedIterableTest.kt @@ -47,6 +47,32 @@ class PagedIterableTest { .status(Status.OK) .build() + /** + * Builds a [Response] whose body increments [closeCount] on each close, so a test can assert + * that the iterator released the page (response body + pooled connection). + */ + private fun closeRecordingResponse(closeCount: AtomicInteger): Response { + val body = + object : org.dexpace.sdk.core.http.response.ResponseBody() { + override fun mediaType() = null + + override fun contentLength() = -1L + + override fun source(): org.dexpace.sdk.core.io.BufferedSource = + throw UnsupportedOperationException("not needed for close test") + + override fun close() { + closeCount.incrementAndGet() + } + } + return Response.builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .status(Status.OK) + .body(body) + .build() + } + private fun page( value: List, nextLink: String? = null, @@ -494,6 +520,68 @@ class PagedIterableTest { assertEquals(1, closed.get(), "underlying body.close() should have been called once") } + @Test + fun `partial consume via first closes the fetched page`() { + val closeCount = AtomicInteger(0) + val iterable = + PagedIterable( + firstPage = { page(listOf(1, 2, 3), response = closeRecordingResponse(closeCount)) }, + ) + + val firstItem = iterable.iterator().next() + + assertEquals(1, firstItem) + assertEquals(1, closeCount.get(), "the fetched page must be closed eagerly once its items are taken") + } + + @Test + fun `take(1) closes the fetched page without abandoning an open response`() { + val closeCount = AtomicInteger(0) + val iterable = + PagedIterable( + firstPage = { page(listOf(10, 20), response = closeRecordingResponse(closeCount)) }, + ) + + val taken = iterable.take(1).toList() + + assertEquals(listOf(10), taken) + assertEquals(1, closeCount.get(), "a single-item consume must still close the page it materialized") + } + + @Test + fun `stream findFirst closes the fetched page`() { + val closeCount = AtomicInteger(0) + val iterable = + PagedIterable( + firstPage = { page(listOf(7, 8, 9), response = closeRecordingResponse(closeCount)) }, + ) + + val found = iterable.stream().findFirst() + + assertTrue(found.isPresent) + assertEquals(7, found.get()) + assertEquals(1, closeCount.get(), "short-circuiting via stream().findFirst() must not strand the page") + } + + @Test + fun `full iteration closes every page exactly once`() { + val firstClose = AtomicInteger(0) + val secondClose = AtomicInteger(0) + val iterable = + PagedIterable( + firstPage = { + page(listOf(1, 2), nextLink = "p2", response = closeRecordingResponse(firstClose)) + }, + nextPage = { _, _ -> + page(listOf(3, 4), response = closeRecordingResponse(secondClose)) + }, + ) + + assertEquals(listOf(1, 2, 3, 4), iterable.toList()) + assertEquals(1, firstClose.get(), "first page closed exactly once on full iteration") + assertEquals(1, secondClose.get(), "second page closed exactly once on full iteration") + } + // --------------------------------------------------------------------- // PagingOptions propagation // --------------------------------------------------------------------- diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncHttpPipelineTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncHttpPipelineTest.kt index 9b5aff65..72e1648a 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncHttpPipelineTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/AsyncHttpPipelineTest.kt @@ -15,6 +15,7 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.http.response.Status import java.io.IOException +import java.io.InterruptedIOException import java.net.URL import java.util.concurrent.CompletableFuture import java.util.concurrent.CountDownLatch @@ -236,7 +237,7 @@ class AsyncHttpPipelineTest { } @Test - fun `AsyncHttpPipeline_toBlocking unwraps CompletionException to the original cause`() { + fun `AsyncHttpPipeline_toBlocking unwraps the wrapper to the original cause`() { val async = AsyncHttpPipelineBuilder( AsyncHttpClient { @@ -249,6 +250,38 @@ class AsyncHttpPipelineTest { assertEquals("oops", thrown.message) } + @Test + fun `toBlocking honours interruption, throwing InterruptedIOException with the flag set`() { + // A future that never completes, so toBlocking parks in get() until interrupted. + val never = CompletableFuture() + val async = AsyncHttpPipelineBuilder(AsyncHttpClient { never }).build() + val sync = async.toBlocking() + + val started = CountDownLatch(1) + val thrown = AtomicReference() + val flagSet = AtomicBoolean(false) + val worker = + Thread { + started.countDown() + try { + sync.send(getRequest()) + } catch (t: Throwable) { + thrown.set(t) + flagSet.set(Thread.currentThread().isInterrupted) + } + } + worker.start() + assertTrue(started.await(2, TimeUnit.SECONDS)) + Thread.sleep(50) + worker.interrupt() + worker.join(2_000) + + val failure = thrown.get() + assertTrue(failure is InterruptedIOException, "expected InterruptedIOException, got $failure") + assertTrue(flagSet.get(), "interrupt flag must be restored on the calling thread") + assertTrue(never.isCancelled, "the in-flight future must be cancelled on interruption") + } + @Test fun `builder appendAll appends every step in iteration order`() { val a = TestStep(Stage.PRE_AUTH) { _, next -> next.processAsync() } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AsyncInstrumentationStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AsyncInstrumentationStepTest.kt index c16f6a0e..99d1dc4f 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AsyncInstrumentationStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AsyncInstrumentationStepTest.kt @@ -17,6 +17,7 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.request.RequestBody import org.dexpace.sdk.core.http.response.LoggableResponseBody import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.ResponseBody import org.dexpace.sdk.core.http.response.Status import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.instrumentation.FakeSlf4jLogger @@ -34,6 +35,7 @@ import org.dexpace.sdk.core.instrumentation.installBasicMdcAdapter import org.dexpace.sdk.core.instrumentation.metrics.DoubleHistogram import org.dexpace.sdk.core.instrumentation.metrics.LongCounter import org.dexpace.sdk.core.instrumentation.metrics.Meter +import org.dexpace.sdk.core.io.BufferedSource import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.testing.FakeHttpClient import org.dexpace.sdk.core.testing.FixedClock @@ -129,6 +131,81 @@ class AsyncInstrumentationStepTest { response.close() } + @Test + fun `known-length response body is wrapped and bounded to the preview cap`() { + // A known-length body larger than bodyPreviewMaxBytes is wrapped: the capture is bounded + // to the cap, while the caller still streams the full body via source(). + val payload = "y".repeat(100) + val fakeAsync = + AsyncHttpClient { request -> + CompletableFuture.completedFuture(okResponseWithBody(request, 200, payload)) + } + val pipeline = + AsyncHttpPipelineBuilder(fakeAsync) + .append( + DefaultAsyncInstrumentationStep( + options = + HttpInstrumentationOptions( + logLevel = HttpLogLevel.BODY_AND_HEADERS, + bodyPreviewMaxBytes = 10, + ), + ), + ) + .build() + + val response = pipeline.sendAsync(getRequest("https://api.example.com/data")).join() + val body = response.body ?: fail("expected non-null body") + assertTrue(body is LoggableResponseBody, "known-length body must be wrapped") + assertEquals(10, body.snapshot().size, "capture must be bounded to bodyPreviewMaxBytes") + assertEquals(payload, body.source().readUtf8(), "caller must still stream the full body") + response.close() + } + + @Test + fun `unknown-length response body is NOT wrapped in the async step`() { + // The async drain runs on the completion thread; an unknown-length (streaming) body + // (contentLength() < 0) is left unwrapped so a slow producer never blocks that thread. + val streamingBody = + object : ResponseBody() { + override fun mediaType(): MediaType? = MediaType.parse("text/plain") + + override fun contentLength(): Long = -1L + + override fun source(): BufferedSource = Io.provider.source("streamed".toByteArray()) + + override fun close() { /* no-op */ } + } + val fakeAsync = + AsyncHttpClient { request -> + CompletableFuture.completedFuture( + Response.builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .status(Status.OK) + .body(streamingBody) + .build(), + ) + } + val pipeline = + AsyncHttpPipelineBuilder(fakeAsync) + .append( + DefaultAsyncInstrumentationStep( + options = HttpInstrumentationOptions(logLevel = HttpLogLevel.BODY_AND_HEADERS), + ), + ) + .build() + + val response = pipeline.sendAsync(getRequest("https://api.example.com/stream")).join() + val body = response.body ?: fail("expected non-null body") + assertFalse( + body is LoggableResponseBody, + "unknown-length body must NOT be wrapped (no eager drain on the completion thread)", + ) + // The original streaming body is passed through untouched. + assertEquals("streamed", body.source().readUtf8()) + response.close() + } + @Test fun `emits failure event when downstream future completes exceptionally`() { val fakeSlf4j = FakeSlf4jLogger("test.async.instrumentation.failure") diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt index be0181b1..6658ea18 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt @@ -389,8 +389,10 @@ class InstrumentationStepTest { } @Test - fun `bodyPreviewMaxBytes truncates the response body preview to the configured cap`() { - // Cover the snapshot(maxBytes) path with maxBytes < body size. + fun `bodyPreviewMaxBytes bounds the captured preview while the caller still streams the full body`() { + // With BODY_AND_HEADERS the response is wrapped with the in-memory capture bounded to + // bodyPreviewMaxBytes. The log preview is bounded, but the caller still receives the + // entire body via source() (captured prefix + live tail). val payload = "x".repeat(100) val fake = FakeHttpClient().enqueue { status(200).body(payload, MediaType.parse("text/plain")) } val pipeline = @@ -407,13 +409,12 @@ class InstrumentationStepTest { val response = pipeline.send(getRequest("https://api.example.com/x")) val body = response.body ?: fail("body must be present") - // The wrapped body still exposes the full bytes via snapshot() — only the log preview - // is truncated. Caller-facing API stays intact. assertTrue(body is LoggableResponseBody) - val full = body.snapshot() - assertEquals(100, full.size, "wrapper must retain full body for the caller") - val truncated = body.snapshot(10) - assertEquals(10, truncated.size, "snapshot(10) must return only the cap") + // The in-memory capture is bounded to the preview cap. + assertEquals(10, body.snapshot().size, "capture must be bounded to bodyPreviewMaxBytes") + assertEquals(10, body.snapshot(10).size, "snapshot(10) must return only the cap") + // But the caller still streams the FULL body via source() (prefix + live tail). + assertEquals(payload, body.source().readUtf8(), "caller must receive the full body") response.close() } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RedirectStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RedirectStepTest.kt index 5a672ab7..de4260bd 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RedirectStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RedirectStepTest.kt @@ -939,6 +939,31 @@ class RedirectStepTest { assertEquals("api.example.com", reissued.url.host) } + @Test + fun `stripping userinfo preserves percent-encoding in path and query`() { + // L3: clearing the userinfo must not re-encode the Location. A path segment carrying + // %2F and a query value carrying %26 must survive verbatim — rebuilding via URI's + // decoded multi-arg constructor would turn %2F into '/' and %26 into '&', silently + // changing the path/query structure of the reissued request. + val fake = + FakeHttpClient() + .enqueue { status(302).header("Location", "https://user:pass@api.example.com/a%2Fb?x=%26") } + .enqueue { status(200) } + + val pipeline = + HttpPipelineBuilder(fake) + .append(DefaultRedirectStep()) + .build() + + pipeline.send(getRequest("https://api.example.com/v1")) + + val reissued = fake.requests[1] + assertNull(reissued.url.userInfo, "userinfo must be stripped") + val target = reissued.url.toString() + assertTrue(target.contains("/a%2Fb"), "encoded path segment %2F must be preserved: $target") + assertTrue(target.contains("x=%26"), "encoded query value %26 must be preserved: $target") + } + // ----------------- Other non-3xx status codes don't trigger redirect ----------------- @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt index b415b873..12fd9b1b 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt @@ -30,7 +30,9 @@ import org.dexpace.sdk.core.testing.FixedClock import org.dexpace.sdk.core.util.Clock import org.dexpace.sdk.core.util.DateTimeRfc1123 import org.dexpace.sdk.io.OkioIoProvider +import java.io.ByteArrayInputStream import java.io.IOException +import java.io.InputStream import java.io.InterruptedIOException import java.time.Duration import java.time.Instant @@ -684,13 +686,15 @@ class RetryStepTest { } @Test - fun `non-replayable PUT body IS retried because PUT is idempotent`() { - // The gate keys on method idempotency too: PUT is idempotent, so even a non-replayable - // body is retried (the body axis only blocks non-idempotent methods). + fun `non-replayable PUT body is NOT retried even though PUT is idempotent`() { + // Both gates are required: PUT is idempotent, but a non-replayable body physically + // cannot be re-sent, so the request is NOT retry-safe. The 503 is returned as-is rather + // than re-dispatched (which would trip the body's consume-once guard on the second + // writeTo). Method idempotency alone does not widen eligibility for a body-bearing + // request. val fake = FakeHttpClient() .enqueue { status(503) } - .enqueue { status(200) } val pipeline = HttpPipelineBuilder(fake) @@ -704,9 +708,77 @@ class RetryStepTest { .body(NonReplayableRequestBody()) .build() + val response = pipeline.send(request) + assertEquals(503, response.status.code) + assertEquals(1, fake.callCount, "non-replayable PUT must not be retried") + } + + @Test + fun `replayable PUT body IS retried`() { + // Control for the gate: a replayable body on an idempotent PUT is safe to re-send, so + // retry proceeds normally. + val fake = + FakeHttpClient() + .enqueue { status(503) } + .enqueue { status(200) } + + val pipeline = + HttpPipelineBuilder(fake) + .append(DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock())) + .build() + + val request = + Request.builder() + .method(Method.PUT) + .url("https://api.example.com/x") + .body(org.dexpace.sdk.core.http.request.RequestBody.create("payload".toByteArray())) + .build() + val response = pipeline.send(request) assertEquals(200, response.status.code) - assertEquals(2, fake.callCount, "idempotent PUT must retry regardless of body replayability") + assertEquals(2, fake.callCount, "replayable PUT must retry") + } + + @Test + fun `real guarded one-shot PUT body surfaces the original 503 with no IllegalStateException`() { + // End-to-end proof of the gate using a REAL consume-once body (RequestBody.create over a + // non-markable InputStream → OneShotInputStreamRequestBody). The transport actually + // drains the body on every call, so a buggy gate that retried a PUT on method-idempotency + // alone would invoke writeTo a SECOND time and trip the consume-once CAS guard, surfacing + // an IllegalStateException that masks the real failure. With both gates required the body + // is written exactly once and the original 503 is returned verbatim. + val writes = AtomicInteger(0) + val client = + object : HttpClient { + override fun execute(request: Request): Response { + // Drain the body exactly as a real transport would — this exercises the + // consume-once guard. + request.body?.writeTo(Io.provider.buffer()) + writes.incrementAndGet() + return Response.builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .status(Status.fromCode(503)) + .headers(Headers.Builder().build()) + .build() + } + } + + val pipeline = + HttpPipelineBuilder(client) + .append(DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock())) + .build() + + val request = + Request.builder() + .method(Method.PUT) + .url("https://api.example.com/x") + .body(oneShotBody()) + .build() + + val response = pipeline.send(request) + assertEquals(503, response.status.code, "original 503 must be surfaced, not an IllegalStateException") + assertEquals(1, writes.get(), "the one-shot body must be written exactly once — no replay") } // ----------------- Accumulated suppressed exceptions ----------------- @@ -902,6 +974,70 @@ class RetryStepTest { assertTrue(ex.cause?.message == "predicate boom") } + @Test + fun `throwing shouldRetryCondition closes the retryable response before propagating`() { + // M6: the should-retry predicate runs while the retryable response is still open. If it + // throws, the response must be closed before the exception propagates — otherwise the + // 5xx response's socket/buffer leaks. Assert both the close() is observed and the + // exception still surfaces. + val closes = AtomicInteger(0) + val client = + object : HttpClient { + override fun execute(request: Request): Response = + Response.builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .status(Status.fromCode(503)) + .headers(Headers.Builder().build()) + .body(CountingClosableBody(closes)) + .build() + } + val opts = + HttpRetryOptions( + shouldRetryCondition = { throw RuntimeException("predicate boom") }, + ) + val pipeline = + HttpPipelineBuilder(client) + .append(DefaultRetryStep(opts, zeroDelayClock())) + .build() + + val ex = assertFailsWith { pipeline.send(getRequest()) } + assertTrue(ex.message?.contains("shouldRetry predicate threw") == true) + assertTrue(closes.get() >= 1, "retryable response must be closed when the predicate throws") + } + + @Test + fun `throwing computeResponseDelay override closes the retryable response before propagating`() { + // M6: a subclass override of computeResponseDelay runs after the predicate approves the + // retry, while the response is still open. If it throws, the response must be closed + // before the exception propagates. + val closes = AtomicInteger(0) + val client = + object : HttpClient { + override fun execute(request: Request): Response = + Response.builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .status(Status.fromCode(503)) + .headers(Headers.Builder().build()) + .body(CountingClosableBody(closes)) + .build() + } + val step = + object : DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock()) { + override fun computeResponseDelay(condition: HttpRetryCondition): Duration = + error("delay override boom") + } + val pipeline = + HttpPipelineBuilder(client) + .append(step) + .build() + + val ex = assertFailsWith { pipeline.send(getRequest()) } + assertEquals("delay override boom", ex.message) + assertTrue(closes.get() >= 1, "retryable response must be closed when the delay override throws") + } + // ----------------- Error propagation ----------------- @Test @@ -1599,6 +1735,23 @@ class RetryStepTest { .body(NonReplayableRequestBody()) .build() + /** + * A REAL guarded one-shot body: `RequestBody.create(InputStream, length)` over a + * non-markable stream yields a `OneShotInputStreamRequestBody` whose [writeTo] enforces a + * consume-once CAS guard — a second write throws [IllegalStateException]. Used to prove a + * non-replayable PUT body is never re-sent. + */ + private fun oneShotBody(): org.dexpace.sdk.core.http.request.RequestBody { + val backing = ByteArrayInputStream(byteArrayOf(1, 2, 3)) + val nonMarkable = + object : InputStream() { + override fun read(): Int = backing.read() + + override fun markSupported(): Boolean = false + } + return org.dexpace.sdk.core.http.request.RequestBody.create(nonMarkable, 3L) + } + /** Single-use request body: [isReplayable] is false and [writeTo] streams fixed bytes. */ private class NonReplayableRequestBody : org.dexpace.sdk.core.http.request.RequestBody() { override fun mediaType(): MediaType? = MediaType.parse("text/plain") diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBodyTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBodyTest.kt index e11eb8b5..bec1bc7d 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBodyTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/LoggableRequestBodyTest.kt @@ -143,6 +143,46 @@ class LoggableRequestBodyTest { assertContentEquals("data".toByteArray(), sink2.snapshot()) } + @Test + fun `bounded wrapper caps the tap but streams the full body to the sink`() { + val payload = "0123456789".toByteArray() // 10 bytes + val delegate = RequestBody.create(payload) + val wrapper = LoggableRequestBody.bounded(delegate, Io.provider, tapLimit = 4) + val sink = Io.provider.buffer() + + wrapper.writeTo(sink) + + // The transport sink receives the FULL body. + assertContentEquals(payload, sink.snapshot()) + // The tap captured only the bounded preview. + assertContentEquals("0123".toByteArray(), wrapper.snapshot()) + } + + @Test + fun `bounded wrapper preserves the tap cap across a replayable rewrite`() { + // A single-use delegate forces toReplayable() to build a new wrapper; the cap must carry. + val delegate = RequestBody.create(Io.provider.source("0123456789".toByteArray())) + val wrapper = LoggableRequestBody.bounded(delegate, Io.provider, tapLimit = 3) + val replayable = wrapper.toReplayable(Io.provider) + assertTrue(replayable is LoggableRequestBody) + + val sink = Io.provider.buffer() + replayable.writeTo(sink) + + assertContentEquals("0123456789".toByteArray(), sink.snapshot(), "full body must reach the sink") + assertContentEquals("012".toByteArray(), replayable.snapshot(), "tap cap must survive the rewrite") + } + + @Test + fun `default wrapper mirrors the entire body unbounded`() { + val payload = "x".repeat(20_000).toByteArray() + val wrapper = LoggableRequestBody(RequestBody.create(payload)) + val sink = Io.provider.buffer() + wrapper.writeTo(sink) + assertContentEquals(payload, sink.snapshot()) + assertContentEquals(payload, wrapper.snapshot(), "default path mirrors the whole body") + } + @Test fun `snapshot captures bytes written before a delegate failure`() { val delegate = diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt index cc737f69..d7eb13ef 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt @@ -11,11 +11,14 @@ import org.dexpace.sdk.core.http.common.Headers import org.dexpace.sdk.core.http.common.HttpHeaderName import java.net.MalformedURLException import java.net.URL +import java.net.URLStreamHandler import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertNotEquals import kotlin.test.assertNotSame import kotlin.test.assertNull +import kotlin.test.assertTrue class RequestTest { @Test @@ -215,4 +218,92 @@ class RequestTest { } assertEquals("URL is required.", ex.message) } + + // --------------------------------------------------------------------- + // Equality — compares url by external form, never resolving DNS. + // --------------------------------------------------------------------- + + @Test + fun `requests with different hosts are not equal`() { + val a = + Request.builder() + .url("https://host-a.example/path") + .method(Method.GET) + .build() + val b = + Request.builder() + .url("https://host-b.example/path") + .method(Method.GET) + .build() + + assertNotEquals(a, b) + } + + @Test + fun `equal requests stay equal with consistent hashCode`() { + val a = + Request.builder() + .url("https://api.example.test/path") + .method(Method.POST) + .addHeader("X-A", "1") + .build() + val b = + Request.builder() + .url("https://api.example.test/path") + .method(Method.POST) + .addHeader("X-A", "1") + .build() + + assertEquals(a, b) + assertEquals(a.hashCode(), b.hashCode()) + // Stable across repeated invocations. + assertEquals(a.hashCode(), a.hashCode()) + } + + @Test + fun `requests differing only by url path are not equal`() { + val a = + Request.builder() + .url("https://api.example.test/one") + .method(Method.GET) + .build() + val b = + Request.builder() + .url("https://api.example.test/two") + .method(Method.GET) + .build() + + assertNotEquals(a, b) + } + + @Test + fun `equals and hashCode perform no network IO`() { + // A URLStreamHandler whose equals/hashCode/getHostAddress throw if the JDK URL machinery + // ever tries to resolve the host. java.net.URL.equals/hashCode delegate to the handler and + // would trip these; Request must compare via toExternalForm() and never touch them. + val exploding = + object : URLStreamHandler() { + override fun openConnection(u: URL) = + throw AssertionError("openConnection must not be called during equality") + + override fun equals( + u1: URL?, + u2: URL?, + ): Boolean = throw AssertionError("URLStreamHandler.equals (DNS) must not be called") + + override fun hashCode(u: URL): Int = + throw AssertionError("URLStreamHandler.hashCode (DNS) must not be called") + } + + val urlA = URL("https", "host-a.example", -1, "/path", exploding) + val urlB = URL("https", "host-a.example", -1, "/path", exploding) + + val a = Request.builder().url(urlA).method(Method.GET).build() + val b = Request.builder().url(urlB).method(Method.GET).build() + + // These calls would throw AssertionError if Request delegated to URL.equals/hashCode. + assertEquals(a, b) + assertEquals(a.hashCode(), b.hashCode()) + assertTrue(a == b) + } } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBodyTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBodyTest.kt index 2f92edc8..18e7b04b 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBodyTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/LoggableResponseBodyTest.kt @@ -269,6 +269,79 @@ class LoggableResponseBodyTest { ) } + // ----- H1: bounded capture + live-tail behavior ----- + + @Test + fun `over-cap body returns the full bytes to the consumer while snapshot is bounded`() { + val payload = "abcdefghijklmnopqrstuvwxyz" // 26 bytes + val wrapper = LoggableResponseBody.bounded(bodyFromText(payload), Io.provider, 10L) + + // The in-memory capture is bounded to 10 bytes. + assertEquals(10, wrapper.snapshot().size, "capture must be bounded to maxCaptureBytes") + assertContentEquals("abcdefghij".toByteArray(Charsets.UTF_8), wrapper.snapshot()) + + // But the consumer still receives the FULL body via source() (prefix + live tail). + assertEquals(payload, wrapper.source().readUtf8(), "consumer must receive the full body") + wrapper.close() + } + + @Test + fun `second source() on an over-cap body throws IllegalStateException`() { + val payload = "abcdefghijklmnopqrstuvwxyz" // 26 bytes > cap + val wrapper = LoggableResponseBody.bounded(bodyFromText(payload), Io.provider, 5L) + + // First source() hands out the one-shot prefix+tail stream. + val first = wrapper.source() + assertNotNull(first) + + // The live tail is single-use; a second source() must be rejected. + val ex = assertFailsWith { wrapper.source() } + assertNotNull(ex.message) + assertTrue( + ex.message!!.contains("single-use", ignoreCase = true) || + ex.message!!.contains("maxCaptureBytes", ignoreCase = true), + "Expected an over-cap one-shot message, got: ${ex.message}", + ) + wrapper.close() + } + + @Test + fun `body within the cap stays fully repeatable`() { + val payload = "small" // 5 bytes <= cap + val wrapper = LoggableResponseBody.bounded(bodyFromText(payload), Io.provider, 64L) + + // Within the cap: source() yields a fresh repeatable view each time. + assertEquals(payload, wrapper.source().readUtf8()) + assertEquals(payload, wrapper.source().readUtf8(), "within-cap body must be repeatable") + // contentLength reflects the fully-captured size. + assertEquals(payload.toByteArray(Charsets.UTF_8).size.toLong(), wrapper.contentLength()) + wrapper.close() + } + + @Test + fun `over-cap body reports the delegate content length not the captured prefix`() { + val payload = "abcdefghijklmnopqrstuvwxyz" // 26 bytes + val delegateLength = payload.toByteArray(Charsets.UTF_8).size.toLong() + val wrapper = LoggableResponseBody.bounded(bodyFromText(payload), Io.provider, 4L) + + // Trigger the bounded drain. + wrapper.snapshot() + // Over-cap: contentLength is the delegate's true length, not the 4-byte prefix. + assertEquals(delegateLength, wrapper.contentLength()) + wrapper.close() + } + + @Test + fun `default constructor captures the whole body unbounded and repeatable`() { + // The public (unlimited) path is unchanged: full capture, repeatable source(). + val payload = "x".repeat(50_000) + val wrapper = LoggableResponseBody(bodyFromText(payload)) + assertEquals(payload, wrapper.source().readUtf8()) + assertEquals(payload, wrapper.source().readUtf8(), "default path must be fully repeatable") + assertEquals(payload.toByteArray(Charsets.UTF_8).size, wrapper.snapshot().size) + wrapper.close() + } + @Test fun `concurrent source() calls serialize drain and both callers get a view`() { val payload = "shared" diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/io/TeeSinkTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/io/TeeSinkTest.kt index 41b27359..81d4142b 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/io/TeeSinkTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/io/TeeSinkTest.kt @@ -256,6 +256,93 @@ class TeeSinkTest { assertContentEquals("2345".toByteArray(), tap.snapshot()) } + // ----- M4: bounded tap (tapLimit) ----- + + @Test + fun `tapLimit caps the tap but forwards the full payload to primary - typed write`() { + val provider = OkioIoProvider + val primaryOut = ByteArrayOutputStream() + val primary = provider.sink(primaryOut) + val tap = provider.buffer() + val tee = TeeSink(primary, tap, provider, tapLimit = 5) + + tee.writeUtf8("0123456789") // 10 bytes, cap is 5 + tee.flush() + + // Primary received ALL 10 bytes — the wire body is never truncated. + assertContentEquals("0123456789".toByteArray(), primaryOut.toByteArray()) + // The tap captured only the first 5 bytes. + assertContentEquals("01234".toByteArray(), tap.snapshot()) + } + + @Test + fun `tapLimit caps the tap across multiple writes`() { + val provider = OkioIoProvider + val primaryOut = ByteArrayOutputStream() + val primary = provider.sink(primaryOut) + val tap = provider.buffer() + val tee = TeeSink(primary, tap, provider, tapLimit = 6) + + tee.write("abcd".toByteArray()) // 4 bytes -> tap has "abcd" + tee.write("efgh".toByteArray()) // 4 more -> tap fills to 6 ("abcdef"), rest forwarded only + tee.flush() + + // Primary got everything (8 bytes). + assertContentEquals("abcdefgh".toByteArray(), primaryOut.toByteArray()) + // Tap is capped at 6. + assertContentEquals("abcdef".toByteArray(), tap.snapshot()) + } + + @Test + fun `tapLimit caps the tap on the write Buffer path`() { + val provider = OkioIoProvider + val primaryOut = ByteArrayOutputStream() + val primary = provider.sink(primaryOut) + val tap = provider.buffer() + val tee = TeeSink(primary, tap, provider, tapLimit = 3) + + val src = OkioIoProvider.buffer().apply { writeUtf8("payload") } // 7 bytes + tee.write(src, 7) + tee.flush() + + assertContentEquals("payload".toByteArray(), primaryOut.toByteArray()) + assertContentEquals("pay".toByteArray(), tap.snapshot()) + assertEquals(0L, src.size, "the full source must be drained into primary") + } + + @Test + fun `tapLimit caps the tap on the outputStream path`() { + val provider = OkioIoProvider + val primaryOut = ByteArrayOutputStream() + val primary = provider.sink(primaryOut) + val tap = provider.buffer() + val tee = TeeSink(primary, tap, provider, tapLimit = 4) + + val s = tee.outputStream() + s.write("ab".toByteArray()) // 2 bytes + s.write("cdef".toByteArray(), 0, 4) // 4 more; cap reached at 4, remainder forwarded only + s.write('Z'.code) // single byte past the cap — primary only + s.flush() + + assertContentEquals("abcdefZ".toByteArray(), primaryOut.toByteArray()) + assertContentEquals("abcd".toByteArray(), tap.snapshot()) + } + + @Test + fun `zero tapLimit mirrors nothing but forwards everything`() { + val provider = OkioIoProvider + val primaryOut = ByteArrayOutputStream() + val primary = provider.sink(primaryOut) + val tap = provider.buffer() + val tee = TeeSink(primary, tap, provider, tapLimit = 0) + + tee.writeUtf8("anything") + tee.flush() + + assertContentEquals("anything".toByteArray(), primaryOut.toByteArray()) + assertEquals(0L, tap.size, "a zero tapLimit must mirror nothing") + } + @Test fun `buffer property is unsupported on TeeSink`() { // Direct buffer access would write to the tap only, never the primary sink — a diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipelineTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipelineTest.kt index ab0abfe4..933751af 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipelineTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/ResponsePipelineTest.kt @@ -13,7 +13,9 @@ import org.dexpace.sdk.core.http.context.DispatchContext import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.ResponseBody import org.dexpace.sdk.core.http.response.Status +import org.dexpace.sdk.core.io.BufferedSource import org.dexpace.sdk.core.pipeline.step.RequestPipelineStep import org.dexpace.sdk.core.pipeline.step.ResponsePipelineStep import org.dexpace.sdk.core.pipeline.step.ResponseRecoveryStep @@ -24,6 +26,7 @@ import org.junit.jupiter.api.Assertions.assertThrows import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test import java.io.IOException +import java.util.concurrent.atomic.AtomicInteger /** * Covers WU-1 acceptance criteria for the recovery-aware response pipeline. Tests are @@ -82,6 +85,34 @@ class ResponsePipelineTest { .status(Status.OK) .build() + /** + * A response whose body increments [closeCount] each time it is closed. Lets a test assert + * that a throwing step released the in-hand transport connection. + */ + private fun closeRecordingResponseFor( + request: Request, + closeCount: AtomicInteger, + ): Response { + val body = + object : ResponseBody() { + override fun mediaType() = null + + override fun contentLength() = -1L + + override fun source(): BufferedSource = throw UnsupportedOperationException("not needed for close test") + + override fun close() { + closeCount.incrementAndGet() + } + } + return Response.builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .status(Status.OK) + .body(body) + .build() + } + private fun ctx(): DispatchContext = DispatchContext.default() // endregion @@ -217,6 +248,49 @@ class ResponsePipelineTest { assertSame(responseStepError, recoverySawError) } + @Test + fun `response step that throws closes the in-hand response`() { + val req = request() + val closeCount = AtomicInteger(0) + val inHand = closeRecordingResponseFor(req, closeCount) + + val responseStepError = RuntimeException("response step failed") + val failingResponseStep = + ResponsePipelineStep { _, _ -> + throw responseStepError + } + + val pipeline = ResponsePipeline(responseSteps = listOf(failingResponseStep)) + + val out = pipeline.apply(ResponseOutcome.Success(inHand), ctx()) + + // No recovery step rescues it, so the throwable surfaces as a Failure. + assertTrue(out is ResponseOutcome.Failure) + assertSame(responseStepError, (out as ResponseOutcome.Failure).error) + assertEquals(1, closeCount.get(), "the in-hand response must be closed exactly once when a step throws") + } + + @Test + fun `recovery step that throws on a Success closes the in-hand response`() { + val req = request() + val closeCount = AtomicInteger(0) + val inHand = closeRecordingResponseFor(req, closeCount) + + val recoveryError = RuntimeException("recovery step failed") + val failingRecoveryStep = + ResponseRecoveryStep { _ -> + throw recoveryError + } + + val pipeline = ResponsePipeline(recoverySteps = listOf(failingRecoveryStep)) + + val out = pipeline.apply(ResponseOutcome.Success(inHand), ctx()) + + assertTrue(out is ResponseOutcome.Failure) + assertSame(recoveryError, (out as ResponseOutcome.Failure).error) + assertEquals(1, closeCount.get(), "the in-hand Success response must be closed when a recovery step throws") + } + @Test fun `request step throws routed through recovery chain`() { val req = request() diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt index 578553f8..0b5b3ec3 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStepTest.kt @@ -146,6 +146,27 @@ class RetryStepTest { .build() } + private fun requestPutNonReplayable(): Request { + // OneShotInputStreamRequestBody (non-markable stream) is single-use / non-replayable. + // PUT is idempotent, so this isolates the body-replayability gate from the method gate. + val body = + RequestBody.create( + java.io.ByteArrayInputStream(byteArrayOf(1, 2, 3)).let { stream -> + object : java.io.InputStream() { + override fun read(): Int = stream.read() + + override fun markSupported(): Boolean = false + } + }, + 3L, + ) + return Request.builder() + .url("https://api.example.com/resource") + .method(Method.PUT) + .body(body) + .build() + } + private fun response( status: Int, headers: Headers = Headers.builder().build(), @@ -212,6 +233,21 @@ class RetryStepTest { assertTrue(client.calls.isEmpty()) } + @Test + fun `503 with non-replayable PUT body is not retried despite PUT being idempotent`() { + // Both gates are required: PUT is in retryableMethods (idempotent), but a non-replayable + // body physically cannot be re-sent. Method idempotency alone must NOT widen eligibility + // — the failure passes through unchanged and the transport is never re-invoked (which + // would otherwise trip the body's consume-once guard). + val client = FakeClient() + val request = requestPutNonReplayable() + val step = RetryStep(client, zeroDelaySettings(InstantScheduler()), request) + val outcome = ResponseOutcome.Failure(httpException(SC_SERVICE_UNAVAILABLE)) + val out = step.invoke(outcome) + assertSame(outcome, out, "Outcome must be unchanged when an idempotent method carries a non-replayable body") + assertTrue(client.calls.isEmpty()) + } + @Test fun `404 is not retried`() { val client = FakeClient() diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/util/ProxyOptionsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/util/ProxyOptionsTest.kt index f30bad76..29fbec21 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/util/ProxyOptionsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/util/ProxyOptionsTest.kt @@ -635,9 +635,11 @@ class ProxyOptionsTest { @Test fun `challengeHandler is retained alongside username and password`() { - // When both are present we keep both — the consumer is expected to prefer the - // challengeHandler. ProxyOptions itself does not enforce one over the other; that - // is documented as the caller's responsibility (per to-implement.md §16). + // When both are present we keep both verbatim. challengeHandler is a reserved slot: + // no shipped transport reads it today (the OkHttp transport applies Basic auth from + // username/password, the JDK transport delegates Basic/Digest negotiation to the JDK + // stack from the same credentials). ProxyOptions itself stores the handler without + // enforcing any precedence; this test pins that both fields survive construction. val handler = StubChallengeHandler() val po = ProxyOptions( diff --git a/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/FakeHttpClient.kt b/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/FakeHttpClient.kt index f2c2cdd6..277b3723 100644 --- a/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/FakeHttpClient.kt +++ b/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/FakeHttpClient.kt @@ -10,6 +10,7 @@ package org.dexpace.sdk.core.testing import org.dexpace.sdk.core.client.HttpClient import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response +import java.io.InterruptedIOException import java.time.Duration /** @@ -56,6 +57,10 @@ class FakeHttpClient : HttpClient { * Records [request], pops the next canned [MockResponse], honors its simulated delay * with `Thread.sleep`, and returns the materialized [Response]. * + * If the simulating sleep is interrupted, the interrupt flag is restored and the wait is + * surfaced as an [InterruptedIOException] — matching the [HttpClient.execute] cancellation + * contract rather than letting the raw [InterruptedException] escape. + * * Throws [IllegalStateException] if no response is enqueued. */ override fun execute(request: Request): Response { @@ -66,7 +71,14 @@ class FakeHttpClient : HttpClient { "FakeHttpClient: no response enqueued for request to ${request.url}", ) if (mock.delay > Duration.ZERO) { - Thread.sleep(mock.delay.toMillis()) + try { + Thread.sleep(mock.delay.toMillis()) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + val ioe = InterruptedIOException("FakeHttpClient: interrupted while simulating response delay") + ioe.initCause(e) + throw ioe + } } return mock.toResponse(request) } diff --git a/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/RequestRecorder.kt b/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/RequestRecorder.kt index dc60007d..da0beca8 100644 --- a/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/RequestRecorder.kt +++ b/sdk-core/src/testFixtures/kotlin/org/dexpace/sdk/core/testing/RequestRecorder.kt @@ -8,26 +8,29 @@ package org.dexpace.sdk.core.testing import org.dexpace.sdk.core.http.request.Request +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock /** * Thread-safe captured-request log used by [FakeHttpClient] and any other test fixture * that needs to observe the exact request a pipeline produced. * - * Backed by a `mutableListOf` guarded with `synchronized` — adequate for the modest + * Backed by a `mutableListOf` guarded with a [ReentrantLock] — adequate for the modest * concurrency seen in test scenarios. [snapshot] returns an immutable copy so callers * can iterate without worrying about concurrent mutation. */ class RequestRecorder { + private val lock: ReentrantLock = ReentrantLock() private val list: MutableList = mutableListOf() /** Appends [request] to the log. Safe to call concurrently. */ fun record(request: Request) { - synchronized(list) { list.add(request) } + lock.withLock { list.add(request) } } /** Returns an immutable snapshot of the captured requests in insertion order. */ - fun snapshot(): List = synchronized(list) { list.toList() } + fun snapshot(): List = lock.withLock { list.toList() } /** Number of requests captured so far. */ - val callCount: Int get() = synchronized(list) { list.size } + val callCount: Int get() = lock.withLock { list.size } } diff --git a/sdk-io-okio3/src/main/kotlin/org/dexpace/sdk/io/internal/WriteAllInto.kt b/sdk-io-okio3/src/main/kotlin/org/dexpace/sdk/io/internal/WriteAllInto.kt index 53ad519d..15f72e92 100644 --- a/sdk-io-okio3/src/main/kotlin/org/dexpace/sdk/io/internal/WriteAllInto.kt +++ b/sdk-io-okio3/src/main/kotlin/org/dexpace/sdk/io/internal/WriteAllInto.kt @@ -8,6 +8,7 @@ package org.dexpace.sdk.io.internal import org.dexpace.sdk.core.io.Source +import java.io.IOException /** * Shared `writeAll` implementation used by both [OkioBufferedSink] and the buffer-side @@ -15,11 +16,15 @@ import org.dexpace.sdk.core.io.Source * known to be Okio-backed; otherwise falls back to a pump loop through an intermediate * [okio.Buffer]. * - * `LoggableResponseBody.drainAndCache()` is the main beneficiary of the fast path: it does - * `buf.writeAll(src)` to suck a wrapped response body into an in-memory cache; when `src` is - * an [OkioBufferedSource] (the common case), Okio moves segments by ownership transfer - * rather than copying bytes. + * Source-to-sink body bridging is the main beneficiary of the fast path: when `source` is an + * [OkioBufferedSource] (the common case), Okio moves segments by ownership transfer rather + * than copying bytes. + * + * On the fallback path a `read` of `0` for a non-zero `byteCount` violates the + * [Source.read] contract and is **rejected** with an [IOException] rather than tolerated as + * a silent EOF — mirroring `TeeSink.writeAll`. Only a `read` of `-1` terminates the loop. */ +@Throws(IOException::class) internal fun writeAllInto( sink: okio.BufferedSink, source: Source, @@ -32,14 +37,16 @@ internal fun writeAllInto( var total = 0L while (true) { val read = source.read(tmp, SEGMENT_SIZE) - // The Source contract forbids returning 0 for a non-zero byteCount in blocking - // mode. We treat 0 as exhaustion conservatively to avoid an infinite loop. - // A well-behaved blocking Source should return -1 (EOF) instead of 0; the - // defensive break here exists to handle misbehaving foreign implementations - // without spinning forever. - if (read <= 0L) break - sink.write(tmp.delegate, tmp.delegate.size) - total += read + when { + read == -1L -> break // EOF — normal termination + read == 0L -> throw IOException( + "Source returned 0 for byteCount=$SEGMENT_SIZE which violates the Source.read contract", + ) + else -> { + sink.write(tmp.delegate, tmp.delegate.size) + total += read + } + } } total } diff --git a/sdk-io-okio3/src/test/kotlin/org/dexpace/sdk/io/WriteAllIntoTest.kt b/sdk-io-okio3/src/test/kotlin/org/dexpace/sdk/io/WriteAllIntoTest.kt index e1d66abe..15ae77e1 100644 --- a/sdk-io-okio3/src/test/kotlin/org/dexpace/sdk/io/WriteAllIntoTest.kt +++ b/sdk-io-okio3/src/test/kotlin/org/dexpace/sdk/io/WriteAllIntoTest.kt @@ -13,11 +13,13 @@ import org.dexpace.sdk.core.io.BufferedSource import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.io.Source import java.io.ByteArrayOutputStream +import java.io.IOException import java.nio.charset.Charset import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertContentEquals import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertTrue /** @@ -26,7 +28,7 @@ import kotlin.test.assertTrue * * The Okio-fast paths are already covered by `OkioIoProviderTest`. Here we focus on the * `else` branch that does a pump loop through an intermediate `OkioBuffer`, and on the - * `read <= 0` guard against a foreign Source that returns 0. + * contract guard that rejects a foreign Source returning 0 for a non-zero byteCount. */ class WriteAllIntoTest { @BeforeTest @@ -76,7 +78,7 @@ class WriteAllIntoTest { ): BufferedSource = throw UnsupportedOperationException() } - /** Foreign Source that returns 0 on first read — ensures the slow path's guard short-circuits. */ + /** Foreign Source that returns 0 on first read — ensures the slow path's guard rejects it. */ private class ZeroSource : Source { override fun read( sink: Buffer, @@ -98,13 +100,10 @@ class WriteAllIntoTest { } @Test - fun `writeAll terminates on a Source that returns zero`() { + fun `writeAll rejects a Source that returns zero for a non-zero byteCount`() { val out = ByteArrayOutputStream() val sink: BufferedSink = OkioIoProvider.sink(out) - val n = sink.writeAll(ZeroSource()) - sink.flush() - assertEquals(0L, n) - assertContentEquals(ByteArray(0), out.toByteArray()) + assertFailsWith { sink.writeAll(ZeroSource()) } } @Test diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt index da4485a7..8df42865 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt @@ -265,6 +265,7 @@ public class JdkHttpTransport private constructor( * - `httpVersion`: [HttpVersion.HTTP_2]. */ public class Builder internal constructor() : SdkBuilder { + private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.jdkhttp.JdkHttpTransport.Builder") private var connectTimeout: Duration? = null private var responseTimeout: Duration? = Duration.ofSeconds(DEFAULT_RESPONSE_TIMEOUT_SECONDS) private var proxy: ProxyOptions? = null @@ -364,12 +365,29 @@ public class JdkHttpTransport private constructor( * configured proxy, returning `null` for origin-server (401) challenges so the * proxy credentials never leak to an origin host. * + * A configured [ProxyOptions.challengeHandler] is **not** honoured by this transport: + * `java.net.http.HttpClient` exposes no per-407 hook through which a custom + * `ChallengeHandler` (e.g. Digest) could be invoked, so the handler is dropped with a + * loud warning rather than silently ignored. Proxy authentication falls back to the + * JDK's own username/password negotiation via [ProxyAuthenticator]. Consumers needing + * Digest proxy auth should use the OkHttp transport. + * * Credentials are deliberately never logged. */ private fun applyProxy( clientBuilder: java.net.http.HttpClient.Builder, options: ProxyOptions, ) { + if (options.challengeHandler != null) { + log.atWarning() + .event("proxy.auth.challenge_handler.unsupported") + .log( + "ProxyOptions.challengeHandler is set but the JDK transport cannot invoke a " + + "custom ChallengeHandler: java.net.http.HttpClient exposes no per-407 hook. " + + "The handler is ignored; proxy auth falls back to username/password via the " + + "JDK's own auth negotiation. Use the OkHttp transport for Digest proxy auth.", + ) + } if (options.type != ProxyOptions.Type.HTTP) { // SOCKS via the JDK client builder is not supported; silently return without // applying. See KDoc above for the system-wide workaround. diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt index e9320fe1..29871b05 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/BodyPublishers.kt @@ -9,11 +9,16 @@ package org.dexpace.sdk.transport.jdkhttp.internal import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.io.Io +import java.io.Closeable +import java.io.IOException +import java.io.InputStream +import java.io.InterruptedIOException import java.io.PipedInputStream import java.io.PipedOutputStream import java.net.http.HttpRequest import java.util.concurrent.ExecutorService import java.util.concurrent.Executors +import java.util.concurrent.Future import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody /** @@ -31,28 +36,44 @@ import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody * because the body would have to be materialised in the publisher anyway and small * bodies do not benefit from streaming. * - * - **Streaming path (`contentLength > 64 KiB` or `-1L` (unknown))** — create a - * [PipedOutputStream] / [PipedInputStream] pair. A daemon writer thread drains - * `body.writeTo(...)` into the OutputStream; the publisher reads from the InputStream - * via `BodyPublishers.ofInputStream`. Exceptions on the writer side propagate to the - * reader as `IOException` (the standard JDK PipedStream contract), which the JDK client - * surfaces back to the calling future. + * - **Streaming path (`contentLength > 64 KiB` or `-1L` (unknown))** — return a + * `BodyPublishers.ofInputStream` whose supplier creates a **fresh** + * [PipedOutputStream] / [PipedInputStream] pair and a **fresh** writer task on every + * invocation. A daemon writer thread drains `body.writeTo(...)` into the OutputStream; + * the publisher reads from the InputStream. See "Per-subscription pipes" below for why + * the pipe and writer are created lazily, per-subscription, rather than once. * * The 64 KiB threshold matches the spec; not exposed via the builder. Justification: the * pipe coordination overhead measurably exceeds the buffer cost below this size, and * holding 64 KiB in heap is uncontroversial. * + * ## Per-subscription pipes + * + * The JDK invokes the `ofInputStream` supplier **once per subscription**, and it re-subscribes + * the same publisher on internal resends — notably the 407 proxy-auth retry driven by the + * `ProxyAuthenticator` this transport installs, and HTTP/2 `GOAWAY` replays. A supplier that + * returned one shared, already-draining stream would hand the second subscription an exhausted + * pipe, so the authenticated retry would carry a truncated or empty body. The supplier therefore + * constructs a new pipe + writer each time, and the streaming path requires the body to be + * **replayable** (see [streamingPublisher]) so a second `writeTo` produces the same bytes. + * * ## Writer thread lifecycle * * [bodyWriterExecutor] is process-wide and daemon-threaded — JVM shutdown reaps any * stranded writers without an explicit close hook. Threads are named * `dexpace-jdkhttp-body-writer` so they are identifiable in thread dumps. * + * A subscription that is acquired but never drained (connect failure, cancellation before + * the body is sent) would otherwise strand its writer thread blocked in + * `PipedOutputStream.write`. The returned [PipedInputStream] is wrapped so that closing it + * — which the JDK does when it abandons a subscription — cancels the writer [Future] and + * closes the pipe's output end, unblocking the stuck writer. + * * ## Thread-safety * - * Each call to [adaptBody] creates a fresh `BodyPublisher` over a fresh pipe (for the - * streaming path) or a fresh byte array (for the eager path); no state is shared between - * concurrent invocations. + * Each call to [adaptBody] creates a fresh `BodyPublisher`; each subscription to a streaming + * publisher creates a fresh pipe + writer. No state is shared between concurrent invocations + * or between subscriptions. */ internal object BodyPublishers { /** @@ -109,44 +130,158 @@ internal object BodyPublishers { * memory is released when the array is handed off. */ private fun eagerPublisher(body: SdkRequestBody): HttpRequest.BodyPublisher { - val buffer = Io.provider.buffer() - body.writeTo(buffer) - val bytes = buffer.snapshot() + val bytes = bufferToByteArray(body) return HttpRequest.BodyPublishers.ofByteArray(bytes) } /** - * Creates a [PipedOutputStream] / [PipedInputStream] pair, submits a daemon task that - * drives `body.writeTo(...)` onto the OutputStream side, and returns - * [HttpRequest.BodyPublishers.ofInputStream] over the InputStream side. + * Streaming publisher for bodies larger than the eager threshold (or of unknown length). * - * The `ofInputStream` factory expects a `Supplier` — the JDK calls it once - * to acquire the stream. The stream returned here is constructed eagerly so the writer - * task can start producing bytes immediately; the JDK reader picks them up as it - * consumes the publisher. + * The JDK calls the `ofInputStream` supplier **once per subscription** and re-subscribes + * on internal resends (407 proxy-auth retry, HTTP/2 `GOAWAY` replay). Each subscription + * therefore re-reads the body, so the body must be replayable for the resent request to + * carry the correct bytes. If [body] is not already replayable it is buffered once into an + * in-memory copy via [SdkRequestBody.toReplayable]. A body that cannot be made replayable + * (its `toReplayable` throws) falls back to a single eager byte-array publisher — correct + * for the first send and harmless on a resend — rather than a one-shot pipe that would + * corrupt the body on resubscribe. * - * Errors thrown from `body.writeTo` close the OutputStream prematurely (because the - * `use { }` block exits abnormally); the JDK reader then observes an `IOException` on - * its next read and the surrounding future completes exceptionally. The DEBUG log - * records the writer-side failure so it's discoverable in tests / production. + * For each subscription the supplier: + * 1. creates a fresh [PipedOutputStream] / [PipedInputStream] pair; + * 2. submits a writer task that drives `body.writeTo(...)` onto the OutputStream side and + * captures its [Future]; and + * 3. returns the InputStream wrapped so its `close()` cancels the writer [Future] and + * closes the OutputStream — unblocking a writer stranded in `PipedOutputStream.write` + * when the JDK abandons the subscription without draining it. + * + * Errors thrown from `body.writeTo` close the OutputStream prematurely (the `use { }` block + * exits abnormally); the JDK reader then observes an `IOException` on its next read and the + * surrounding future completes exceptionally. Thread interruption in the writer is honoured + * per the repo convention: the flag is restored and an [InterruptedIOException] is surfaced + * so the reader side fails loudly. The DEBUG log records the writer-side failure so it is + * discoverable in tests / production. */ private fun streamingPublisher(body: SdkRequestBody): HttpRequest.BodyPublisher { + val replayable: SdkRequestBody = + if (body.isReplayable()) { + body + } else { + try { + body.toReplayable() + } catch (e: IOException) { + // The body could not be buffered into a replayable copy. A one-shot pipe + // would corrupt the body on the JDK's resubscribe, so fall back to a single + // eager byte-array publisher: the body has already been partially consumed + // by the failed buffering attempt, but emitting whatever was captured is + // strictly better than handing back a stream that is wrong on resend. + log.atVerbose() + .event("transport.jdkhttp.body.replayable.failed") + .field("error.message", e.message ?: "") + .log("could not buffer streaming body as replayable; falling back to eager publisher") + return HttpRequest.BodyPublishers.ofByteArray(bufferToByteArray(body)) + } + } + return HttpRequest.BodyPublishers.ofInputStream { newSubscriptionStream(replayable) } + } + + /** + * Test seam: opens a single streaming subscription's InputStream directly, bypassing the + * `ofInputStream` publisher's own reader thread so a test can hold the stream un-drained and + * close it to exercise the [KillSwitchInputStream] writer-unblock path. The [body] must be + * replayable (the production path guarantees this before reaching [newSubscriptionStream]). + */ + @JvmSynthetic + internal fun openSubscriptionStreamForTest(body: SdkRequestBody): InputStream = newSubscriptionStream(body) + + /** + * Builds one subscription's InputStream: a fresh pipe pair plus a writer task draining + * [body] into it. Returns the read end wrapped with a kill-switch `close()` (see + * [KillSwitchInputStream]). + */ + private fun newSubscriptionStream(body: SdkRequestBody): InputStream { val pipeIn = PipedInputStream(PIPE_BUFFER_BYTES) val pipeOut = PipedOutputStream(pipeIn) - bodyWriterExecutor.execute { - try { - pipeOut.use { os -> - Io.provider.sink(os).use { sdkSink -> - body.writeTo(sdkSink) + val task = + Runnable { + try { + pipeOut.use { os -> + Io.provider.sink(os).use { sdkSink -> + body.writeTo(sdkSink) + } } + } catch (e: InterruptedIOException) { + // The writer was interrupted (e.g. its Future was cancelled when the + // subscription was abandoned). Restore the flag per repo convention; the + // pipe is already closing, so the reader observes EOF / IOException. + Thread.currentThread().interrupt() + logWriterFailure(e) + } catch (e: InterruptedException) { + Thread.currentThread().interrupt() + logWriterFailure(e) + } catch (t: Throwable) { + logWriterFailure(t) } - } catch (t: Throwable) { - log.atVerbose() - .event("transport.jdkhttp.body.write.failed") - .field("error.message", t.message ?: "") - .log("piped body writer failed; reader side will see IOException") + } + val writer: Future<*> = bodyWriterExecutor.submit(task) + return KillSwitchInputStream(pipeIn, pipeOut, writer) + } + + private fun logWriterFailure(t: Throwable) { + log.atVerbose() + .event("transport.jdkhttp.body.write.failed") + .field("error.message", t.message ?: "") + .log("piped body writer failed; reader side will see IOException") + } + + /** + * Drains [body] into a fresh in-memory [org.dexpace.sdk.core.io.Buffer] from the active + * [Io.provider] and snapshots it to a byte array. + */ + private fun bufferToByteArray(body: SdkRequestBody): ByteArray { + val buffer = Io.provider.buffer() + body.writeTo(buffer) + return buffer.snapshot() + } + + /** + * Wraps a subscription's [PipedInputStream] so that closing the read end also cancels the + * writer [Future] and closes the [PipedOutputStream] write end. Without this, a JDK + * subscription that is acquired and then abandoned (connect failure, cancellation before + * the body is sent) would leave the writer thread blocked indefinitely in + * `PipedOutputStream.write`, pinning the writer thread and the body's resources. + * + * `close()` is idempotent: the JDK may close the supplied stream more than once. + */ + private class KillSwitchInputStream( + private val pipeIn: PipedInputStream, + private val pipeOut: PipedOutputStream, + private val writer: Future<*>, + ) : InputStream() { + override fun read(): Int = pipeIn.read() + + override fun read( + b: ByteArray, + off: Int, + len: Int, + ): Int = pipeIn.read(b, off, len) + + override fun available(): Int = pipeIn.available() + + override fun close() { + // Interrupt/cancel the writer first so a thread parked in PipedOutputStream.write + // wakes up, then close both pipe ends. Order matters: closing pipeOut alone does + // not unblock a writer mid-write, so the cancel(true) is what frees the thread. + writer.cancel(true) + closeQuietly(pipeOut) + closeQuietly(pipeIn) + } + + private fun closeQuietly(closeable: Closeable) { + try { + closeable.close() + } catch (_: IOException) { + // Best-effort teardown — the peer end may already be closed. } } - return HttpRequest.BodyPublishers.ofInputStream { pipeIn } } } diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt index 55ff42a6..032886d2 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt @@ -101,6 +101,11 @@ internal class RequestAdapter( * Catching it here drops the offending header with a DEBUG log rather than letting the * `IllegalArgumentException` escape [adapt] (and therefore `execute`, declared * `@Throws(IOException)`) where a caller's `catch(IOException)` would not observe it. + * + * Note this catch guards against the JDK's restricted *name* set only. Illegal header + * *values* (CR/LF and similar) are now rejected upstream by `Headers.Builder`, so a value + * with control characters never reaches this point — the `IllegalArgumentException` handled + * here is the JDK refusing a restricted name, not a malformed value. */ private fun attachHeaders( builder: HttpRequest.Builder, diff --git a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt index 6581def7..42e2e4c6 100644 --- a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt +++ b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt @@ -12,20 +12,30 @@ import mockwebserver3.MockWebServer import mockwebserver3.junit5.StartStop import okio.Buffer import org.dexpace.sdk.core.http.common.CommonMediaTypes +import org.dexpace.sdk.core.http.common.MediaType import org.dexpace.sdk.core.http.common.Protocol import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.request.RequestBody +import org.dexpace.sdk.core.io.BufferedSink import org.dexpace.sdk.core.io.Io +import org.dexpace.sdk.core.util.ProxyOptions import org.dexpace.sdk.io.OkioIoProvider +import org.dexpace.sdk.transport.jdkhttp.internal.BodyPublishers +import java.io.ByteArrayOutputStream import java.io.IOException +import java.net.InetSocketAddress import java.net.ServerSocket import java.net.URL import java.net.http.HttpClient +import java.net.http.HttpRequest +import java.nio.ByteBuffer import java.security.MessageDigest import java.time.Duration import java.util.concurrent.CancellationException import java.util.concurrent.CompletionException +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Flow import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicReference @@ -291,6 +301,106 @@ class JdkHttpTransportTest { } } + // -------- streaming publisher re-subscription (H4) -------- + + @Test + fun `streamingPublisherSurvivesResubscription`() { + // The JDK re-acquires the BodyPublisher's InputStream once per subscription and + // re-subscribes on internal resends (407 proxy-auth retry, HTTP/2 GOAWAY replay). The + // supplier must therefore mint a fresh pipe + writer per subscription so a replayable + // body produces the full bytes every time — not an exhausted/empty stream on resend. + // A true proxy 407 flow is awkward to drive over plaintext MockWebServer, so we drive + // the publisher directly: a > 64 KiB replayable body, drained twice. + val bytes = randomBytes(128 * 1024, seed = 4242L) + val body = RequestBody.create(bytes, CommonMediaTypes.APPLICATION_OCTET_STREAM) + val publisher = BodyPublishers.adaptBody(body) + + val first = drainPublisher(publisher) + assertContentEquals(bytes, first, "first subscription must yield the full streaming body") + + // Re-subscribe: the same publisher must mint a fresh stream and replay the full body. + val second = drainPublisher(publisher) + assertContentEquals(bytes, second, "re-subscription must yield the full streaming body again") + } + + @Test + fun `streamingBodyRoundTripsThroughTransport`() { + // End-to-end confirmation that the per-subscription streaming path delivers a > 64 KiB + // replayable body intact over the real transport, not just in the direct-publisher test. + val bytes = randomBytes(256 * 1024, seed = -7L) + server.enqueue(MockResponse.Builder().code(200).build()) + val request = + Request.builder() + .method(Method.POST) + .url(server.url("/stream-resub").toUrl()) + .body(RequestBody.create(bytes, CommonMediaTypes.APPLICATION_OCTET_STREAM)) + .build() + transport.execute(request).use { it.body?.source()?.readByteArray() } + val recorded = server.takeRequest() + val received = recorded.body?.toByteArray() ?: ByteArray(0) + assertEquals(md5(bytes), md5(received), "streaming body must round-trip intact") + } + + @Test + fun `abandonedStreamingPublisherDoesNotStrandWriter`() { + // A subscription that is acquired but never drained (connect failure, cancellation + // before body send) would otherwise leave the writer thread blocked forever in + // PipedOutputStream.write once the 8 KiB pipe fills. Closing the supplied InputStream — + // which the JDK does when it abandons a subscription — must cancel/interrupt the writer. + // We open one subscription's stream via the test seam, let the writer fill the pipe and + // park in write(), then close the stream and assert the writer is released. + val writerExited = CountDownLatch(1) + val bytes = randomBytes(512 * 1024, seed = 99L) + val body = + LatchedReplayableBody(bytes, CommonMediaTypes.APPLICATION_OCTET_STREAM, writerExited) + + val stream = BodyPublishers.openSubscriptionStreamForTest(body) + // Give the writer time to fill the 8 KiB pipe and park in PipedOutputStream.write. + Thread.sleep(200) + assertEquals(1L, writerExited.count, "writer should still be parked, not yet exited") + // The kill switch: closing the stream must unblock/cancel the parked writer. + stream.close() + + assertTrue( + writerExited.await(3, TimeUnit.SECONDS), + "closing the abandoned subscription stream must unblock/cancel the writer thread", + ) + } + + // -------- proxy challengeHandler (M7) -------- + + @Test + fun `proxyChallengeHandlerIsAcceptedAndSurfacedAsUnsupported`() { + // ProxyOptions.challengeHandler cannot be honoured by java.net.http (no per-407 hook). + // The builder must accept it, log the unsupported warning, and fall back to + // username/password negotiation rather than throwing. The slf4j-nop test binding has no + // appender, so we assert the accepted-and-functional contract: the transport builds and + // dispatches a request without error through the challengeHandler branch of applyProxy. + server.enqueue(MockResponse.Builder().code(200).body("ok").build()) + val proxyAddress = InetSocketAddress("127.0.0.1", freePort()) + val options = + ProxyOptions( + type = ProxyOptions.Type.HTTP, + address = proxyAddress, + username = "u", + password = "p", + challengeHandler = NoopChallengeHandler, + bypassAllHosts = true, + ) + val proxiedTransport = + JdkHttpTransport.builder() + .httpVersion(JdkHttpTransport.HttpVersion.HTTP_1_1) + .proxy(options) + .build() + // bypassAllHosts routes every host directly, so the request reaches MockWebServer + // without touching the (dead) proxy — proving the builder accepted the + // challengeHandler-bearing ProxyOptions and produced a working transport. + proxiedTransport.execute(simpleGet("/proxy-challenge")).use { response -> + assertEquals(200, response.status.code) + assertEquals("ok", response.body?.source()?.readUtf8()) + } + } + // -------- response body streaming -------- @Test @@ -555,4 +665,77 @@ class JdkHttpTransportTest { private fun freePort(): Int { ServerSocket(0).use { socket -> return socket.localPort } } + + /** + * Drains a [HttpRequest.BodyPublisher] (a `Flow.Publisher`) to completion, + * returning the concatenated bytes. Subscribes fresh each call, so a second invocation on + * the same publisher exercises the JDK's per-subscription re-acquisition. + */ + private fun drainPublisher(publisher: HttpRequest.BodyPublisher): ByteArray { + val out = ByteArrayOutputStream() + val done = CountDownLatch(1) + val error = AtomicReference() + publisher.subscribe( + object : Flow.Subscriber { + override fun onSubscribe(subscription: Flow.Subscription) { + subscription.request(Long.MAX_VALUE) + } + + override fun onNext(item: ByteBuffer) { + val chunk = ByteArray(item.remaining()) + item.get(chunk) + out.write(chunk) + } + + override fun onError(throwable: Throwable) { + error.set(throwable) + done.countDown() + } + + override fun onComplete() { + done.countDown() + } + }, + ) + assertTrue(done.await(5, TimeUnit.SECONDS), "publisher did not complete in time") + error.get()?.let { throw AssertionError("publisher errored", it) } + return out.toByteArray() + } + + /** + * Replayable body that trips [exited] when its [writeTo] returns or aborts — used to detect + * that an abandoned-subscription writer was actually unblocked rather than left parked. + */ + private class LatchedReplayableBody( + private val bytes: ByteArray, + private val mediaType: MediaType, + private val exited: CountDownLatch, + ) : RequestBody() { + override fun mediaType(): MediaType = mediaType + + override fun contentLength(): Long = bytes.size.toLong() + + override fun isReplayable(): Boolean = true + + override fun writeTo(sink: BufferedSink) { + try { + sink.write(bytes) + sink.flush() + } finally { + exited.countDown() + } + } + } + + /** Minimal [org.dexpace.sdk.core.http.auth.ChallengeHandler] stub for the M7 acceptance test. */ + private object NoopChallengeHandler : org.dexpace.sdk.core.http.auth.ChallengeHandler { + override fun handleChallenges( + method: Method, + uri: java.net.URI, + challenges: List, + isProxy: Boolean, + ): org.dexpace.sdk.core.http.auth.AuthorizationHeader? = null + + override fun canHandle(challenges: List): Boolean = false + } } diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt index 096a58b0..9f0950ad 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransport.kt @@ -115,12 +115,20 @@ public class OkHttpTransport private constructor( call: Call, response: OkResponse, ) { - try { - future.complete(responseAdapter.adapt(request, response)) - } catch (t: Throwable) { - // ResponseAdapter.adapt already closed the response on failure; we only - // need to surface the error to the future. - future.completeExceptionally(t) + val adapted = + try { + responseAdapter.adapt(request, response) + } catch (t: Throwable) { + // ResponseAdapter.adapt already closed the response on failure; we only + // need to surface the error to the future. + future.completeExceptionally(t) + return + } + // The consumer may have cancelled `future` while the exchange was being + // adapted. In that race complete() returns false and nothing else will ever + // close `adapted` — a live connection — so close it here to release it. + if (!future.complete(adapted)) { + closeQuietly(adapted) } } @@ -140,6 +148,19 @@ public class OkHttpTransport private constructor( return future } + /** + * Best-effort close on a discard path. Used when an adapted [Response] loses the race to a + * cancelled future: the response is already being discarded, so a failure to close it has + * nothing actionable to surface. + */ + private fun closeQuietly(response: Response) { + try { + response.close() + } catch (ignored: Throwable) { + // Intentionally ignored: the response is already being discarded. + } + } + /** * Releases SDK-owned OkHttp resources. When this transport was built via [builder] the * underlying [OkHttpClient] is closed by: @@ -223,6 +244,7 @@ public class OkHttpTransport private constructor( * — letting OkHttp follow redirects underneath would double-handle them. */ public class Builder internal constructor() : SdkBuilder { + private val log: ClientLogger = ClientLogger("org.dexpace.sdk.transport.okhttp.OkHttpTransport") private var connectTimeout: Duration? = null private var readTimeout: Duration? = null private var writeTimeout: Duration? = null @@ -304,6 +326,13 @@ public class OkHttpTransport private constructor( * 3. Credentials (when present) are wired through [Authenticator] — * a `Proxy-Authorization: Basic …` is added on 407 challenges. * + * This transport does **not** honour [ProxyOptions.challengeHandler]: a custom proxy + * challenge handler (e.g. Digest) is not wired into OkHttp's `proxyAuthenticator`, which + * only ever emits Basic auth from [ProxyOptions.username] / [ProxyOptions.password]. A + * caller who configured a `challengeHandler` gets a loud WARNING here so the limitation is + * discoverable rather than surfacing as unexplained `407 Proxy Authentication Required` + * responses. + * * Credentials are deliberately never logged; only the proxy host/port appears in * the DEBUG log. */ @@ -311,6 +340,16 @@ public class OkHttpTransport private constructor( okBuilder: OkHttpClient.Builder, options: ProxyOptions, ) { + if (options.challengeHandler != null) { + log.atWarning() + .event("proxy.auth.challenge_handler.unsupported") + .log( + "The OkHttp transport does not honour ProxyOptions.challengeHandler; it is " + + "ignored. Proxy authentication falls back to Basic auth derived from " + + "ProxyOptions.username / ProxyOptions.password. Supply those credentials, " + + "or use a transport that supports a custom proxy challenge handler.", + ) + } val javaType = when (options.type) { ProxyOptions.Type.HTTP -> Proxy.Type.HTTP diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index dc09c964..e9bf5131 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -46,6 +46,9 @@ internal class RequestAdapter( continue } for (value in values) { + // Header names/values are validated upstream by Headers.Builder (CR/LF and other + // illegal characters are rejected at construction), so addHeader receives only + // well-formed values here. builder.addHeader(rawName, value) } } diff --git a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt index 4818a096..c466801f 100644 --- a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt +++ b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt @@ -13,13 +13,17 @@ import mockwebserver3.junit5.StartStop import okhttp3.Interceptor import okhttp3.OkHttpClient import okio.Buffer +import org.dexpace.sdk.core.http.auth.BasicChallengeHandler import org.dexpace.sdk.core.http.common.CommonMediaTypes import org.dexpace.sdk.core.http.common.Protocol import org.dexpace.sdk.core.http.request.FileRequestBody import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.request.RequestBody +import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.Status import org.dexpace.sdk.core.io.Io +import org.dexpace.sdk.core.util.ProxyOptions import org.dexpace.sdk.io.OkioIoProvider import org.dexpace.sdk.transport.okhttp.internal.SdkRequestBodyAdapter import org.junit.jupiter.api.io.TempDir @@ -32,6 +36,7 @@ import java.security.MessageDigest import java.time.Duration import java.util.concurrent.CancellationException import java.util.concurrent.CompletionException +import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicInteger @@ -361,6 +366,127 @@ class OkHttpTransportTest { Thread.sleep(900) } + @Test + fun asyncResponseThatLosesTheRaceIsClosed() { + // L1: the consumer can complete/cancel the returned future in the window *between* OkHttp + // finishing the exchange and Callback.onResponse adapting it. In that race + // `future.complete(adapted)` returns false and nothing else will ever close `adapted` — a + // live connection — so the transport must close it. We reproduce the race deterministically + // without tripping OkHttp's own cancel handling (which, once the call is canceled, routes a + // completed exchange to onFailure rather than onResponse): a BYO interceptor parks the + // already-received response on the dispatcher thread while the test completes the future + // with a decoy value. When the interceptor is released, onResponse runs, observes a + // future it cannot complete, and must close the adapted response. + val responseReady = CountDownLatch(1) + val releaseInterceptor = CountDownLatch(1) + val racingClient = + OkHttpClient.Builder() + .followRedirects(false) + .retryOnConnectionFailure(false) + .addInterceptor( + Interceptor { chain -> + val response = chain.proceed(chain.request()) + // The exchange is complete; signal the test, then block on the dispatcher + // thread so the future is already settled by the time onResponse adapts. + responseReady.countDown() + releaseInterceptor.await(2, TimeUnit.SECONDS) + response + }, + ) + .build() + val racingTransport = OkHttpTransport.create(racingClient) + + server.enqueue(MockResponse.Builder().code(200).body("dropped").build()) + val future = racingTransport.executeAsync(simpleGet("/race")) + + assertTrue(responseReady.await(2, TimeUnit.SECONDS), "interceptor should have received the response") + // Settle the future from the test thread so the in-flight onResponse will lose the + // `future.complete(adapted)` race. Completing (rather than cancelling) avoids triggering + // `call.cancel()`, which would otherwise re-route the completed exchange to onFailure. + val decoy = + Request.builder() + .method(Method.GET) + .url(server.url("/decoy").toUrl()) + .build() + .let { req -> + Response.builder() + .request(req) + .protocol(Protocol.HTTP_1_1) + .status(Status.OK) + .build() + } + assertTrue(future.complete(decoy), "test must win the race so onResponse is the loser") + // Release the parked interceptor; onResponse now adapts and must close the response it + // cannot publish. get() returns the decoy immediately and does not wait for onResponse. + releaseInterceptor.countDown() + future.get(2, TimeUnit.SECONDS).close() + + // onResponse runs on the dispatcher thread; wait until its closeQuietly has returned the + // connection to the pool (it becomes idle) before issuing the follow-up. A leaked response + // would never produce an idle connection here. + assertTrue( + awaitCondition { racingClient.connectionPool.idleConnectionCount() >= 1 }, + "the dropped response's connection must be released back to the pool", + ) + + // Proof the connection was released cleanly: the follow-up reuses the same physical + // connection rather than opening a fresh one. + server.enqueue(MockResponse.Builder().code(200).body("reused").build()) + val firstIndex = server.takeRequest().connectionIndex + val followUp = racingTransport.executeAsync(simpleGet("/after-race")).get(5, TimeUnit.SECONDS) + followUp.use { assertEquals(200, it.status.code) } + val secondIndex = server.takeRequest().connectionIndex + assertEquals( + firstIndex, + secondIndex, + "the dropped response's connection must be released so the follow-up reuses it", + ) + } + + // -------- proxy: unsupported challenge handler -------- + + @Test + fun proxyChallengeHandlerIsAcceptedAndIgnored() { + // M7: ProxyOptions.challengeHandler is not honoured by the OkHttp transport. Building a + // transport with one set must be accepted (it logs a loud WARNING and falls back to Basic + // auth from username/password) rather than throwing. We point the proxy at the + // MockWebServer so the built transport still routes a normal request through it, proving + // the proxy wiring survived the challenge-handler branch in applyProxy. + val proxyOptions = + ProxyOptions( + type = ProxyOptions.Type.HTTP, + address = server.socketAddress, + username = "proxy-user", + password = "proxy-pass", + challengeHandler = BasicChallengeHandler("ignored-user", "ignored-pass"), + ) + val proxiedTransport = + OkHttpTransport.builder() + .proxy(proxyOptions) + .build() + + server.enqueue(MockResponse.Builder().code(200).body("via-proxy").build()) + // An HTTP forward proxy receives the request directly; MockWebServer records it. + val request = + Request.builder() + .method(Method.GET) + .url(URL("http://downstream.example/resource")) + .build() + proxiedTransport.execute(request).use { response -> + assertEquals(200, response.status.code, "request routed through the configured proxy must succeed") + assertEquals("via-proxy", response.body?.source()?.readUtf8()) + } + val recorded = server.takeRequest() + // An HTTP forward proxy receives the absolute-form request target; its presence proves the + // request was routed through the configured proxy (so the proxy wiring survived the + // challenge-handler branch). + assertEquals( + "http://downstream.example/resource", + recorded.target, + "request must have been forwarded via the proxy in absolute form", + ) + } + // -------- redirect behaviour -------- @Test @@ -591,4 +717,23 @@ class OkHttpTransportTest { private fun freePort(): Int { ServerSocket(0).use { socket -> return socket.localPort } } + + /** + * Polls [condition] until it is `true` or [timeoutMillis] elapses. Returns whether the + * condition was met. Used to wait deterministically for asynchronous dispatcher-thread work + * (e.g. a connection being returned to the pool) without a fixed sleep. + */ + private fun awaitCondition( + timeoutMillis: Long = 2_000, + condition: () -> Boolean, + ): Boolean { + val deadline = System.nanoTime() + timeoutMillis * 1_000_000 + while (System.nanoTime() < deadline) { + if (condition()) { + return true + } + Thread.sleep(10) + } + return condition() + } }