Summary
Both transports adapt the request synchronously at the top of executeAsync, before returning the CompletableFuture<Response>:
override fun executeAsync(request: Request): CompletableFuture<Response> {
val okRequest = requestAdapter.adapt(request) // may throw
...
}
Any failure raised while adapting the request is therefore thrown to the caller of executeAsync, not delivered through the returned future. Affected adaptation failures include:
An async caller composing on the returned future will not observe these via .exceptionally / .handle:
transport.executeAsync(req)
.exceptionally(t -> recover(t)); // never runs — the exception is thrown at the call site
Expected
Decide and document the contract:
- If fail-fast-synchronous is intended for setup/validation errors, document it on
executeAsync so callers know some failures arrive synchronously and must be wrapped in their own try/catch; or
- For consistency with the async contract, catch adaptation failures and return a future completed exceptionally so all post-dispatch errors flow through a single channel. (Note: the OkHttp module targets Java 8, so
CompletableFuture.failedFuture is unavailable there — use an explicitly completeExceptionally-d future.)
The latter seems preferable at least for I/O-shaped failures such as the streaming-body buffering failure, which an async caller would reasonably expect to receive via the future rather than as a synchronous throw.
Code
sdk-transport-okhttp/.../OkHttpTransport.kt (executeAsync)
sdk-transport-jdkhttp/.../JdkHttpTransport.kt (executeAsync)
Pre-existing for the CONNECT / header-rejection cases; more relevant now that the adaptation path can raise additional failures (#82). Low severity.
Summary
Both transports adapt the request synchronously at the top of
executeAsync, before returning theCompletableFuture<Response>:Any failure raised while adapting the request is therefore thrown to the caller of
executeAsync, not delivered through the returned future. Affected adaptation failures include:IllegalArgumentExceptionfor aCONNECTrequest; theUncheckedIOExceptionfor an unbufferable streaming body (added in fix: correct transport request-body handling for body-less and unbufferable streaming bodies #82).IllegalArgumentExceptionfromRequest.Builder.methodfor a method/body mismatch (see the companion GET-with-body issue).An async caller composing on the returned future will not observe these via
.exceptionally/.handle:Expected
Decide and document the contract:
executeAsyncso callers know some failures arrive synchronously and must be wrapped in their own try/catch; orCompletableFuture.failedFutureis unavailable there — use an explicitlycompleteExceptionally-d future.)The latter seems preferable at least for I/O-shaped failures such as the streaming-body buffering failure, which an async caller would reasonably expect to receive via the future rather than as a synchronous throw.
Code
sdk-transport-okhttp/.../OkHttpTransport.kt(executeAsync)sdk-transport-jdkhttp/.../JdkHttpTransport.kt(executeAsync)Pre-existing for the CONNECT / header-rejection cases; more relevant now that the adaptation path can raise additional failures (#82). Low severity.