fix: route JDK transport async dispatch failures through the returned future#134
Merged
Merged
Conversation
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
changed the title
…uard The async dispatch-failure test closed a java.net.http.HttpClient to provoke a synchronous sendAsync throw, but two things undercut it: the JDK client returns an already-failed future for a closed client rather than throwing (so the bridge, not executeAsync's guard, handled it), and on this module's JDK 11 test runtime HttpClient is not AutoCloseable, so the test took its early-return skip path and asserted nothing on every run. Replace it with an injected HttpClient whose sendAsync throws on the caller's thread. That drives the post-adaptation guard directly and deterministically on every JDK; narrowing the guard back to wrap only request adaptation makes the test fail, confirming it pins the behaviour. Also correct the executeAsync comments, which stated as fact that sendAsync throws synchronously on a closed client. The guard is defence-in-depth for a client that throws on dispatch — not a description of the stock JDK client, which delivers such failures through the returned future.
JdkHttpTransport.executeAsync guards the whole post-adaptation dispatch path so a synchronous throw becomes a failed future. But if sendAsync had already returned a live in-flight exchange when a later step threw, that exchange kept running on a future nothing would await, leaking its connection. Hoist the in-flight handle out of the try and cancel it from the catch so the exchange is aborted on the failure path; the cancel is a no-op when the throw happened at or before dispatch (handle still null). Also document on the test's HttpClient double why its sslContext() and send() stubs are inert: executeAsync only ever calls sendAsync, so neither is reached on the path under test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
JdkHttpTransport.executeAsyncis documented to deliver every error through the returnedCompletableFuture, never by throwing on the caller's thread. Today the try/catch wraps only therequestAdapter.adapt(...)call. The dispatch kickoff that runs immediately after —client.sendAsync(...)plus thebridgeAsyncResponse(...)wiring — runs unguarded on the caller's thread before the future is handed back.sendAsyncdoes not guarantee that every failure arrives through its returned future. Its Javadoc permits a synchronousIllegalArgumentExceptionfor a request it rejects, and a custom or futureHttpClientimplementation is free to throw on the caller's thread rather than completing the future exceptionally. Any such synchronous throw escapesexecuteAsync, so a future-composing caller's.exceptionally/.handlewould never observe it, breaking the method's contract. (The stock JDK client today packages dispatch failures — e.g. on a closed client — into an already-failed future, whichbridgeAsyncResponsealready propagates; the guard exists for the throwing case.)The OkHttp transport is already correct here:
newCall(...)does no throwing work, and a dispatch failure (including aRejectedExecutionExceptionfrom a shut-down dispatcher) is delivered throughCallback.onFailure, so it already reaches the future.Change
executeAsynctry/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.Exception(notRuntimeException). The intent is that any non-fatal failure, including an unexpected adapter bug such as an NPE, surfaces via the future; onlyError/ JVM-fatal conditions propagate. The inline comments state that breadth is deliberate.future.isCompletedExceptionally()is already true on return (completion is synchronous, not merely eventual) and assert a substring of the adapter's message so an unrelatedIllegalArgumentExceptioncannot satisfy the test. The JDK transport additionally injects a stubHttpClientwhosesendAsyncthrows synchronously and asserts the throw is routed verbatim through the returned future — exercising the post-adaptation guard deterministically on every JDK (the module's tests run on the Java 11 toolchain, where the real client is notAutoCloseable). Narrowing the guard back to wrap only request adaptation makes this test fail, confirming it pins the behaviour.No public-API change.
Gated build commands run
Result: BUILD SUCCESSFUL. (detekt is skipped on
sdk-transport-jdkhttpper the module's toolchain exclusion.)Closes #120
Closes #121
Closes #122