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 fdb0e83a..5b3858f4 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 @@ -138,21 +138,26 @@ public class JdkHttpTransport private constructor( * completions to release the body's connection back to the pool. */ override fun executeAsync(request: Request): CompletableFuture { - val jdkRequest = - try { - requestAdapter.adapt(request, responseTimeout) - } catch (e: Exception) { - // The async contract is that errors arrive through the returned future. Request - // adaptation runs on the caller's thread and can throw (e.g. a CONNECT request the - // JDK client rejects), so route the failure into a failed future instead of - // throwing synchronously where a future-composing caller's .exceptionally/.handle - // would never observe it. Errors (OOM and other JVM-fatal conditions) are left to - // propagate up the caller's stack rather than be packaged into a future that may - // never be awaited. - return CompletableFuture.failedFuture(e) - } - val inFlight = client.sendAsync(jdkRequest, HttpResponse.BodyHandlers.ofInputStream()) - return bridgeAsyncResponse(inFlight) { jdkResponse -> responseAdapter.adapt(request, jdkResponse) } + return try { + val jdkRequest = requestAdapter.adapt(request, responseTimeout) + // `sendAsync` is inside the guard too: it does not promise that every failure arrives + // through its returned future. On a closed `java.net.http.HttpClient` (JDK 21+, where + // the client is `AutoCloseable`) it throws synchronously on the caller's thread, which + // would bypass the future and break the error-delivery contract documented above. + val inFlight = client.sendAsync(jdkRequest, HttpResponse.BodyHandlers.ofInputStream()) + bridgeAsyncResponse(inFlight) { jdkResponse -> responseAdapter.adapt(request, jdkResponse) } + } catch (e: Exception) { + // The async contract is that errors arrive through the returned future. The dispatch + // path above runs on the caller's thread and can throw — request adaptation rejecting a + // CONNECT request, `sendAsync` on a closed client, or an unexpected adapter bug such as + // an NPE — so route any of these into a failed future instead of throwing synchronously + // where a future-composing caller's .exceptionally/.handle would never observe it. The + // breadth is intentional: catching `Exception` (not `RuntimeException`) keeps a future + // adapter step's checked exception on the future too. Only `Error` (OOM and other + // JVM-fatal conditions) is left to propagate up the caller's stack rather than be + // packaged into a future that may never be awaited. + CompletableFuture.failedFuture(e) + } } /** 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 3e5ec734..8646f183 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 @@ -162,11 +162,49 @@ class JdkHttpTransportTest { .build() // Must return a future rather than throwing on the caller's thread. val future = transport.executeAsync(request) + // Completion is synchronous, not merely eventual: the future is already completed + // exceptionally on return, before anything is awaited. + assertTrue( + future.isCompletedExceptionally, + "adaptation failure must complete the future exceptionally synchronously on return", + ) val ex = assertFailsWith { future.get(5, TimeUnit.SECONDS) } assertTrue( ex.cause is IllegalArgumentException, "adaptation failure must surface as the future's cause, was: ${ex.cause?.let { it::class }}", ) + // Assert the message so an unrelated IllegalArgumentException cannot satisfy the test. + // RequestAdapter rejects CONNECT with "java.net.http.HttpClient does not support + // user-issued CONNECT requests." + assertTrue( + ex.cause?.message?.contains("does not support user-issued CONNECT requests") == true, + "expected the JDK CONNECT-rejection message, was: ${ex.cause?.message}", + ) + } + + @Test + fun `executeAsyncDeliversDispatchFailureThroughFuture`() { + // `sendAsync` does not promise that every failure arrives through its returned future: + // on a closed `java.net.http.HttpClient` it throws synchronously on the caller's thread. + // The dispatch path runs after a successful adaptation, so this exercises the post-adapt + // guard specifically — the failure must still arrive through the returned future, not be + // thrown on the caller's thread. `HttpClient` became `AutoCloseable` in JDK 21 (JEP 461); + // on older runtimes there is no close hook to trigger this, so the case is skipped. + val client = HttpClient.newHttpClient() + val closeable = client as? AutoCloseable ?: return // JDK 21+ only; no close hook earlier. + closeable.close() + val closedTransport = JdkHttpTransport.create(client) + + val request = simpleGet("/async-dispatch-fail") + // Must return a future rather than throwing on the caller's thread. + val future = closedTransport.executeAsync(request) + assertTrue( + future.isCompletedExceptionally, + "a synchronous sendAsync throw must complete the future exceptionally synchronously", + ) + // The closed-client throw is an unchecked exception; surface it as the future's cause + // rather than letting it escape executeAsync on the caller's thread. + assertFailsWith { future.get(5, TimeUnit.SECONDS) } } // -------- headers round-trip -------- 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 c2a30777..db5f1c13 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 @@ -116,11 +116,16 @@ public class OkHttpTransport private constructor( // adaptation runs on the caller's thread and can throw (e.g. a method/body // mismatch OkHttp rejects), so route the failure into a completed-exceptionally // future instead of throwing synchronously where a future-composing caller's - // .exceptionally/.handle would never observe it. Errors (OOM and other JVM-fatal - // conditions) are left to propagate up the caller's stack rather than be packaged - // into a future that may never be awaited. + // .exceptionally/.handle would never observe it. The breadth is intentional: + // catching `Exception` (not `RuntimeException`) also funnels an unexpected adapter + // bug such as an NPE through the future so the caller can observe it. Only `Error` + // (OOM and other JVM-fatal conditions) is left to propagate up the caller's stack + // rather than be packaged into a future that may never be awaited. return failedFuture(e) } + // The post-adaptation dispatch needs no guard: `newCall(...)` does no throwing work, and a + // dispatch failure (including a `RejectedExecutionException` from a shut-down dispatcher) + // is delivered through `Callback.onFailure` below, so it already reaches the future. val call = client.newCall(okRequest) val future = CompletableFuture() call.enqueue( 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 c4cc9dbb..77b88bd3 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 @@ -207,11 +207,24 @@ class OkHttpTransportTest { .build() // Must return a future rather than throwing on the caller's thread. val future = transport.executeAsync(request) + // Completion is synchronous, not merely eventual: the future is already completed + // exceptionally on return, before anything is awaited. + assertTrue( + future.isCompletedExceptionally, + "adaptation failure must complete the future exceptionally synchronously on return", + ) val ex = assertFailsWith { future.get(5, TimeUnit.SECONDS) } assertTrue( ex.cause is IllegalArgumentException, "adaptation failure must surface as the future's cause, was: ${ex.cause?.let { it::class }}", ) + // Assert the message so an unrelated IllegalArgumentException cannot satisfy the test. + // OkHttp's Request.Builder.method rejects a body on GET with " must not have a + // request body." + assertTrue( + ex.cause?.message?.contains("must not have a request body") == true, + "expected OkHttp's body-on-GET rejection message, was: ${ex.cause?.message}", + ) } // -------- headers round-trip --------