RFR: 8347597: HttpClient: improve exception reporting when closing connection [v2]

[Daniel Fuchs]

On Tue, 14 Jan 2025 09:52:59 GMT, Jaikiran Pai <jpai at 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