On Thu, 12 Feb 2026 13:42:33 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. src/java.net.http/share/classes/jdk/internal/net/http/Http1Exchange.java line 477: > 475: Http1ResponseBodySubscriber<T> > createResponseSubscriber(BodyHandler<T> handler, ResponseInfo response) { > 476: BodySubscriber<T> subscriber = handler.apply(response); > 477: Objects.requireNonNull(subscriber, "BodyHandler returned a null > BodySubscriber"); Is this necessary? `Http1ResponseBodySubscriber::new` on line 481 will invoke `HttpBodySubscriberWrapper::new` anyway. test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 175: > 173: .build(); > 174: // test for HTTP/2 upgrade when there are no already established > connections > 175: args.add(Arguments.of(new Request(h2HttpReq, Version.HTTP_2))); `test()` issues two requests one after another using `send()` and `sendAsync()`. Effectively, `sendAsync()` will never observe an empty connection pool. I don't think this is something bad, but it makes me think if we add any value by trying with an empty connection pool. test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 177: > 175: args.add(Arguments.of(new Request(h2HttpReq, Version.HTTP_2))); > 176: // test for HTTP/2 upgrade when there is an established > connection > 177: args.add(Arguments.of(new Request(h2HttpReq, Version.HTTP_2, > true))); The test case added 1 line above will already admit a HTTP/2 clear-text connection to the pool. Why do we need `requiresWarmupHEADRequest=true` here? test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 231: > 229: final ExecutionException ee = > assertThrows(ExecutionException.class, f::get); > 230: if (!(ee.getCause() instanceof IOException cause) > 231: || !(cause.getCause() instanceof NullPointerException)) { Would you mind giving this a good shake in the CI, please? Asynchronous nature of things can disrupt assumptions on the causal chain of raised exceptions. test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 236: > 234: } > 235: > 236: private record Request(URI reqURI, Version version, boolean > requiresWarmupHEADRequest) { I doubt if you need this carrier DTO. All you do is `Arguments.of(new Request(requestURI, version, ...))` — instead you can simply do `Arguments.of(requestURI, version, ...)` and change `test(final Request request)` to `test(String requestURI, ...)`. test/jdk/java/net/httpclient/NullReturningBodyHandlerTest.java line 249: > 247: } > 248: > 249: private static final class Handler implements HttpTestHandler { You can consider using `EchoHandler` instead. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799350111 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799272934 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799247226 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799312721 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799209189 PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799188626
