On Thu, 12 Feb 2026 14:54:18 GMT, Volkan Yazici <[email protected]> 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
