RFR: 8267140: Support closing the HttpClient by making it auto-closable [v11]
Daniel Fuchs
dfuchs at openjdk.org
Thu Mar 23 09:33:49 UTC 2023
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
More information about the net-dev
mailing list