RFR: 8267140: Support closing the HttpClient by making it auto-closable [v11]

[Daniel Fuchs]

On Thu, 23 Mar 2023 07:26:43 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Minor updates. Added some links
>>  - Define operations. Clarify some of the things that may stall an orderly shutdown
>
> test/jdk/java/net/httpclient/HttpClientClose.java line 178:
> 
>> 176:         try {
>> 177:             readerService.shutdown();
>> 178:             readerService.awaitTermination(2000, TimeUnit.MILLISECONDS);
> 
> Same comment about the timeout value.

Same answer: there should be no operation in progress at this point - since we have waited for all the completable futures to complete, and we're not relying on the GC to garbage things.

> test/jdk/java/net/httpclient/HttpClientClose.java line 298:
> 
>> 296:                     String msg = "Incorrect uuid header values:[" + uuids + "]";
>> 297:                     (new RuntimeException(msg)).printStackTrace();
>> 298:                     t.sendResponseHeaders(500, -1);
> 
> Same comment about the `-1` value.

Right - same answer: there's no harm in using chunked mode.

> test/jdk/java/net/httpclient/offline/FixedResponseHttpClient.java line 302:
> 
>> 300:         long timeout = duration.compareTo(max) > 0 ? Long.MAX_VALUE : duration.toMillis();
>> 301:         try {
>> 302:             all.exceptionally((t) -> null).get(timeout, TimeUnit.MILLISECONDS);
> 
> This implementation of `awaitTermination` only waits for the current (snapshot) of requests that are currently in progress. If a new request happens to be initiated (and is in-progress) after this snapshot of `responses.toArray()` was taken, then this implementation of `awaitTermination` can possibly return an incorrect `true` (indicating the client has terminated) while there currently might be some requests in progress. This `FixedResponseHttpClient` is just a test specific implementation which appears to be used in one single test class (`OfflineTesting`) and as far as I can see, that test won't be impacted with the implementation of `awaitTermination()`, so I think this implementation might be OK for now.

Good point.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13019#discussion_r1145907963
PR Review Comment: https://git.openjdk.org/jdk/pull/13019#discussion_r1145908782
PR Review Comment: https://git.openjdk.org/jdk/pull/13019#discussion_r1145910744