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

[Jaikiran Pai]

On Thu, 12 Feb 2026 14:54:18 GMT, Volkan Yazici <vyazici at openjdk.org> wrote:

>> 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
>
> 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.

You are right, this is no longer needed. In my initial experiments this is the place I started the fix with, but then noticed the missing check in `HttpBodySubscriberWrapper` and added it there. I've now updated the PR to remove this duplicate check.

> 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, ...)`.

It was an effort to keep the method arguments a bit more concise. But I don't have a strong preference. I have updated the test to split those arguments into individual parts.

> 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.

Hello Volkan, I've updated the test to use `HttpTestEchoHandler`. None of the other `EchoHandler`(s) seemed appropriate for this test.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799760382
PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799751434
PR Review Comment: https://git.openjdk.org/jdk/pull/29691#discussion_r2799746316