On Tue, 14 Oct 2025 07:21:21 GMT, Volkan Yazici <[email protected]> wrote:
> Ensure `HttpClient::sendAsync` translates every operational failure to an > `IOException` as per specification. > > `tier1-2` passes with the proposed changes. > > **Context:** The parent issue, [JDK-8364733], reports that > `HttpClient::sendAsync` leaks exceptions which do not extend from > `IOException`, and this violates the method's specification. The > [JDK-8367067] (#26876) sub-task improved issues around exceptions thrown by > request body publishers – which triggered the first encounter with this > problem. This PR (and its associated sub-task) is aimed to end this saga. > > [JDK-8364733]: https://bugs.openjdk.org/browse/JDK-8364733 > [JDK-8367067]: https://bugs.openjdk.org/browse/JDK-8367067 src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java line 1147: > 1145: var translatedException = unwrappedException > instanceof Error > 1146: ? unwrappedException > 1147: : Utils.toIOException(exception); `toIOException()`, which is _only_ used in `Http3PushManager::cancelPendingPushPromise`, performs unwrapping of `CompletionException` and `ExecutionException` too, though it doesn't exclude `Error`. I considered following approaches: 1. Don't use `toIOException` and hard-code all the translation logic to here 2. Update `toIOException()` to not wrap `Error` – This implies I need to change method's return type from `IOException` to `Throwable`, and this creates problems at the `cancelPendingPushPromise()` call site, since it uses `Http3PushPromiseStream#cancel(IOException)` (note the required `IOException` argument!), which overrides `ExchangeImpl#cancel(IOException)`. In short, this is a dead end. 3. Specialize on `Error` here, but still use `toIOException()` for the rest. I've implemented the 3rd option. But as time passes, I'm getting inclined towards hard-coding all the translation logic (1st option) *and* move `Utils::toIOException` to `Http3PushManager` as a `private` method. I'd appreciate your thoughts on the matter. test/jdk/java/net/httpclient/HttpClientSendAsyncExceptionTest.java line 173: > 171: testCases.add(new ExceptionTestCase( > 172: "IOException", > 173: _ -> { sneakyThrow(ioException); throw new > AssertionError(); }, Sneaky-throw is necessary to throw a checked exception (i.e., `IOException`) from a `HttpResponse.BodyHandler`. test/jdk/java/net/httpclient/InvalidInputStreamSubscriptionRequest.java line 476: > 474: } > 475: > 476: private static boolean throwableCausalChainContainsInstanceOf( `HttpClientImpl::send` wraps `IOException` (wrapping `IllegalArgumentException`) thrown by `sendAsync` in an `IOException` again. As a result, the existing checks performing _"unwrapping of `IOException` once"_ do not work anymore. Since the existing unwrapping was not strict – that is, they're not enforced, but optionally unwrapping if types match in the causal chain – I've further relaxed it with scanning the causal chain. test/jdk/java/net/httpclient/http3/StopSendingTest.java line 184: > 182: } > 183: > 184: private static boolean throwableCausalChainContainsInstanceOf( `HttpClientImpl::send` wraps `IOException` (wrapping `CancellationException`) thrown by `sendAsync` in an `IOException` again. As a result, the existing checks performing _"unwrapping of `IOException` once"_ do not work anymore. Since the existing unwrapping was not strict – that is, they're not enforced, but optionally unwrapping if types match in the causal chain – I've further relaxed it with scanning the causal chain. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27787#discussion_r2428184830 PR Review Comment: https://git.openjdk.org/jdk/pull/27787#discussion_r2428188838 PR Review Comment: https://git.openjdk.org/jdk/pull/27787#discussion_r2428445942 PR Review Comment: https://git.openjdk.org/jdk/pull/27787#discussion_r2428460756
