On Tue, 14 Jan 2025 09:52:59 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review feedback from @japai
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line
> 334:
>
>> 332: if (cf.isCompletedExceptionally()) {
>> 333: // if an error occurs during subscription
>> 334: connection.close(cf.exceptionNow());
>
> `Future.exceptionNow()` (among other things) states that it will throw a
> `IllegalStateException` if the Future was cancelled. We do have a check for
> `cf.isCompletedExceptionally()` before invoking this method. However, the
> `CompletableFuture.isCompletedExceptionally()` states that it returns `true`
> even in the case of cancellation:
>
>> Returns {@code true} if this CompletableFuture completed exceptionally, in
>> any way. Possible causes include cancellation ...
>
> So I suspect this call to `exceptionNow()` might need a rethink or some
> additional checks?
Good catch. I suspect the case where the CF is cancelled should not occur, but
I have added a Utils.exceptionNow method to take care of this case just in case.
> test/jdk/java/net/httpclient/http2/ExpectContinueResetTest.java line 104:
>
>> 102: testThrowable = e.getCause();
>> 103: }
>> 104: assertNotNull(exception, "Request should have completed
>> exceptionally but exception is null");
>
> Unlike the `assertNotNull` on the next line, I think it might be better to
> replace this new `assertNotNull` with a `fail(...)` inside the try block
> immediately after the `performRequest()` call.
I see what you mean. I reworked that part.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23080#discussion_r1915543009
PR Review Comment: https://git.openjdk.org/jdk/pull/23080#discussion_r1915543383
