On Thu, 12 Feb 2026 16:16:50 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8377796? >> >> The change here asserts that if `BodyHandler.apply()` returns `null`, then >> the HTTP request completes exceptionally. A new test has been introduced to >> reproduce the issue and verify the fix. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - split the test method arguments into individual parts > - no need for the duplicate null check > - use HttpTestEchoHandler in the new test LGTM - only one minor remark on the test, and something to investigate further in handleNoBody - but that can be done as a followup. src/java.net.http/share/classes/jdk/internal/net/http/MultiExchange.java line 387: > 385: BodySubscriber<T> bs = responseHandler.apply(new > ResponseInfoImpl(r.statusCode(), > 386: r.headers(), r.version())); > 387: Objects.requireNonNull(bs, "BodyHandler returned a null > BodySubscriber"); We might want to revisit that at some point. Instead of throwing would it make more sense to return a `CompletableFuture` that is completed with a `NullPointerException`? Either way we'll need to see how it behaves WRT operation ref count handling... test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 204: > 202: if (version == Version.HTTP_3) { > 203: builder.setOption(HttpOption.H3_DISCOVERY, > HTTP_3_URI_ONLY); > 204: } Should we assert that version == HTTP_2 instead? Since we only use warmup for HTTP/2. ------------- PR Comment: https://git.openjdk.org/jdk/pull/29691#issuecomment-3895945059 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2803125782 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2803158410
