Skip to content

fix: route JDK transport async dispatch failures through the returned future#134

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/async-adaptation-failure-delivery
Open

fix: route JDK transport async dispatch failures through the returned future#134
OmarAlJarrah wants to merge 1 commit into
mainfrom
fix/async-adaptation-failure-delivery

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Problem

JdkHttpTransport.executeAsync is documented to deliver every error through the returned CompletableFuture, never by throwing on the caller's thread. Today the try/catch wraps only the requestAdapter.adapt(...) call. The dispatch kickoff that runs immediately after — client.sendAsync(...) plus the bridgeAsyncResponse(...) wiring — runs unguarded on the caller's thread before the future is handed back.

sendAsync does not guarantee 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, so the exception escapes on the caller's thread and bypasses the future entirely. A future-composing caller's .exceptionally/.handle would never observe it, breaking the method's contract.

The OkHttp transport is already correct here: newCall(...) does no throwing work, and a dispatch failure (including a RejectedExecutionException from a shut-down dispatcher) is delivered through Callback.onFailure, so it already reaches the future.

Change

  • JDK transport: widen the executeAsync try/catch to enclose the whole post-adaptation dispatch path (sendAsync + bridgeAsyncResponse), so any synchronous throw becomes a failed future rather than escaping on the caller's thread.
  • OkHttp transport: no behavioral change; add a comment recording why its post-adapt path is already future-safe.
  • Both transports: keep the catch at Exception (not RuntimeException). The intent is that any non-fatal failure, including an unexpected adapter bug such as an NPE, surfaces via the future; only Error / JVM-fatal conditions propagate. The inline comments now state that breadth is deliberate.
  • Tests: both transports' async adaptation-failure tests now assert future.isCompletedExceptionally() is already true on return (completion is synchronous, not merely eventual) and assert a substring of the adapter's message so an unrelated IllegalArgumentException cannot satisfy the test. A new JDK test closes an AutoCloseable client and asserts the synchronous sendAsync throw is delivered through the future (skipped on JDK < 21, where the client has no close hook).

No public-API change.

Gated build commands run

./gradlew :sdk-transport-jdkhttp:test :sdk-transport-jdkhttp:ktlintCheck :sdk-transport-jdkhttp:apiCheck \
          :sdk-transport-okhttp:test :sdk-transport-okhttp:ktlintCheck :sdk-transport-okhttp:detekt :sdk-transport-okhttp:apiCheck --no-daemon

Result: BUILD SUCCESSFUL. (detekt is skipped on sdk-transport-jdkhttp per the module's toolchain exclusion.)

Closes #120
Closes #121
Closes #122

JdkHttpTransport.executeAsync guarded only the request-adaptation call.
The dispatch kickoff that follows on the caller's thread —
HttpClient.sendAsync plus the bridgeAsyncResponse wiring — ran
unguarded. sendAsync does not guarantee that every failure reaches its
returned future: on a closed java.net.http.HttpClient (JDK 21+, where
the client is AutoCloseable) it throws synchronously, so the exception
escaped on the caller's thread and bypassed the future. That breaks the
method's documented contract that all errors arrive through the returned
CompletableFuture, and a future-composing caller's .exceptionally/.handle
would never observe such a throw.

Widen the try/catch to enclose the whole post-adaptation dispatch path so
any synchronous throw becomes a failed future. The OkHttp transport is
already correct here — newCall does no throwing work and dispatch
failures arrive via Callback.onFailure — so it is left unchanged apart
from a comment recording why its post-adapt path is future-safe.

The catch stays at Exception (not RuntimeException) in both transports:
the intent is that any non-fatal failure, including an unexpected adapter
bug such as an NPE, surfaces via the future; only Error / JVM-fatal
conditions propagate. The inline comments now state that breadth is
deliberate.

Both transports' async adaptation-failure tests now assert
future.isCompletedExceptionally() is already true on return (completion
is synchronous, not merely eventual) and assert a substring of the
adapter's message so an unrelated IllegalArgumentException cannot satisfy
the test. A new JDK test closes an AutoCloseable client and asserts the
synchronous sendAsync throw is delivered through the future.

Closes #120
Closes #121
Closes #122
@OmarAlJarrah OmarAlJarrah changed the title Route JDK transport async dispatch failures through the returned future fix: route JDK transport async dispatch failures through the returned future Jun 17, 2026
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This widens executeAsync's guard so both requestAdapter.adapt(...) and client.sendAsync(...) run inside the try, routing any synchronous failure into a failed CompletableFuture instead of throwing on the caller's thread. The production change is safe, compiles clean, and carries no public API impact. Approving, but a couple of things around the new test and the comment are off and worth tightening first.

Issues

New dispatch-failure test doesn't actually exercise the post-adapt guardsdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt:185-208. The test is meant to prove that wrapping sendAsync in the try routes a synchronous throw on a closed client through the future. But on JDK 21 and 25, sendAsync on a closed HttpClient does not throw synchronously — it returns an already-failed CompletableFuture. That failed future flows through bridgeAsyncResponse, whose inFlight.whenComplete runs inline (upstream is already complete) and calls result.completeExceptionally(error), so isCompletedExceptionally is already true on return without the new guard. Reverting executeAsync to the pre-PR version (sendAsync outside the try) and running this exact test still passes, so it doesn't guard against removing the change — it only re-asserts the existing bridge behavior. To actually pin the new guard you'd need a client/adapter that throws synchronously from the dispatch call.

Production comment overstates the risk it's fixingsdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt:143-156. The rationale (and the PR description) states that on a closed AutoCloseable HttpClient (JDK 21+), sendAsync "throws synchronously on the caller's thread, which would bypass the future." On JDK 21.0.4 and 25.0.2 it doesn't — sendAsync on a closed client returns an already-completed-exceptionally future. The defensive wrapping is still reasonable (it guards against an unexpected synchronous NPE/IllegalStateException or a future JDK behavior change), but the comment should be reworded to describe it as defense-in-depth rather than citing a concrete throw path that doesn't occur on current JDKs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant