RFR: JDK-8288746: HttpClient resources could be reclaimed more eagerly

[Jaikiran Pai]

On Mon, 20 Jun 2022 14:09:27 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

> Hi,
> 
> Please find here a patch that should help the HttpClient's SelectorManager thread to terminate more timely, allowing the resources consumed by the client to be released earlier.
> 
> The idea is to use a Cleaner registered with the HttpClientFacade to wakeup the Selector when the facade is being gc'ed.
> Some tests have been modified to wait for the selector manager thread to shutdown, and some of them have been observed to timeout when the fix is not in place.

Hello Daniel, the source code changes look OK to me. The test code changes I haven't finished reviewing yet. A few questions related to the `SelectorManager`:

I realize that the goal of this change is to wakeup the `SelectorManager` from its select operation so that it can then go ahead and exit from its `run()` and in the process shut itself down.

In the `SelectorManager`, I see that when it is potentially woken up, it checks if the `HttpClientImpl` (via its facade) is still referenced and if not, it will trigger the shutdown. That happens here https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/master/src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java*L1240__;Iw!!ACWV5N9M2RV99hQ!LPXB1B3ZR48hZnBurd3IAPpjL1C9nZu1HeWDVics4nlcZ-NTctHmDdJAydTTm_jRwUtadK2C8vZy_pfZKa4F$ . However, that check is only done if the `select` operation (which was potentially woken up) returns `0` as the number of ready ops. The `Selector.wakeup()` javadoc notes that it's possible that when the selector is woken up, the select operation may return a non-zero value. With the change in this PR where the `HttpClientImpl` has realized that the facade is no longer in use and thus has triggered the `wakeup`, do you think we should shutdown the `SelectorManager` irrespective of what the return value of the `select` operation was? In other words, should that `if (!owner.isReferenced()) {` check in the `SelectorManager` be outside of the `if (n == 0) {` block?

Continuing further, if we do decide that the code in the `SelectorManager` is fine in its current form, do you think we should have an additional `if (!owner.isReferenced()) {` check right at the start of each loop iteration too, something like:

while (!Thread.currentThread().isInterrupted() && !closed) {
    // Check whether client is still alive, and if not,
    // gracefully stop this thread
    if (!owner.isReferenced()) {
        Log.logTrace("{0}: {1}",
                getName(),
                "HttpClient no longer referenced. Exiting...");
        return;
    }
    synchronized (this) {
        ...
    

}

because in its current form, as far as I can see, even if we wakeup the selector eagerly, it's possible that the `select` operation that it was waiting on, might return a non-zero value and we then end up back in that while loop and do another `select` which could then end up waiting for some more seconds (3 seconds by default).

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

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