RFR: 8377796: java.net.http.HttpClient.send() and sendAsync() never complete when BodyHandler.apply() returns null

[Volkan Yazici]

On Thu, 12 Feb 2026 13:42:33 GMT, Jaikiran Pai <jpai at openjdk.org> 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