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

[Jaikiran Pai]

On Mon, 20 Mar 2023 10:57:05 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> Please find here an RFE that makes the `java.net.HttpClient` auto-closeable.
>> 
>> The API has been modeled on `ExecutorService`.
>> 
>> HttpClient::close() is a graceful shutdown and will wait until all operations are terminated before returning.
>> If a request is in progress, and the caller doesn't pull the corresponding data (for instance, the request was sent with a BodyHandler.ofInputStream(), but the caller stopped reading the input stream) then close() may never return.
>> 
>> Therefore, additional methods similar to those present in `ExecutorService` are also proposed. In summary:
>> 
>> - `shutdown()`: initiate a graceful shutdown, but doesn't wait for termination. 
>> - `shutdownNow()`:  initiate an immediate shutdown, attempting to cancel all operations in progress. Doesn't wait for termination.
>> - `awaitTermination(Duration)`: await for termination within the given delay
>> - `isTerminated()` tells whether the client is terminated
>> 
>> New tests have been added to test the proposed behaviors.
>> 
>> HttpClient tests (new and old) are still stable.
>
> 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 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 let's 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"?

src/java.net.http/share/classes/java/net/http/HttpClient.java line 769:

> 767:     /**
> 768:      * Blocks until all operations have completed execution after a shutdown
> 769:      * request, or the timeout occurs, or the current thread is

If I understand this correctly, this method is expected to be called after `shutdown()` or `shutdownNow()` has been called. If so, then should this method throw an `IllegalStateException` if it gets called before any of those shutdown methods are called?

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

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