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

[Daniel Fuchs]

On Mon, 20 Mar 2023 12:44:17 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into HttpClient-close-8267140
>>  - Update test/jdk/java/net/httpclient/HttpClientShutdown.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - Update test/jdk/java/net/httpclient/ShutdownNow.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - Update test/jdk/java/net/httpclient/HttpClientShutdown.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - Update test/jdk/java/net/httpclient/AsyncShutdownNow.java
>>    
>>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>>  - Review feedback
>>  - typo
>>  - Merge branch 'master' into HttpClient-close-8267140
>>  - Throw NPE if duration is null
>>  - 8267140
>
> src/java.net.http/share/classes/java/net/http/HttpClient.java line 142:
> 
>> 140:  * through the same client. In the JDK implementation, connection pools
>> 141:  * are not shared between {@code HttpClient} instances. Creating a new client
>> 142:  * for each operation, though possible, will prevent reusing such connections.
> 
> I think, the reason why this sentence about creating a new connection for each operation, is here is to discourage applications from doing that. However, I found it a bit odd to have it in the middle of a section named "closing". 
> The existing javadoc of this `HttpClient` has this sentence:
>> Once built, an {@code HttpClient} is immutable, and can be used to send multiple requests.
> 
> Perhaps we should add this sentence there and emphasise that creating a new client for each operation is discouraged since that prevents reuse of existing connections?

Good suggestion. I'm moving that in the paragraph following the one you quoted, since that paragraph already talks about sharing resources. I have softened the wording though, since the text is moved from a non-normative section to a normative section.

> src/java.net.http/share/classes/java/net/http/HttpClient.java line 751:
> 
>> 749:     /**
>> 750:      * Initiates an orderly shutdown in which previously submitted
>> 751:      * operations are completed, but no new request will be accepted.
> 
> I think the word "completed" can be confusing, given its usage in `java.util.concurrent.Future` APIs. I haven't yet reviewed the code, but as far as I understand, the shutdown implementation merely marks a state which won't allow any new requests to be created, but lets the current submitted operations run to completion. The implementation however doesn't (mark such operations) as completed. Do you think we should word this differently? Maybe "Initiates an orderly shutdown in which previously submitted operations are allowed to run to completion, but no new request will be accepted"?

Good idea. Done.

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

PR: https://git.openjdk.org/jdk/pull/13019