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

[Jaikiran Pai]

On Sun, 26 Jun 2022 06:08:07 GMT, Jaikiran Pai <jpai 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!Mo9E5YBNsuLorRTNluRjIsiee0ZUYmDHUX7hP8ontSxNORFwcNB0ZHVYt_HbmTu3Vw7xM19I2R-mTY4KAmkg$ . 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).

> @jaikiran there is already an escape hatch at line 1196:
> 
> https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/7905788e969727c81eea4397f0d9b918cdb5286a/src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java*L1196__;Iw!!ACWV5N9M2RV99hQ!Mo9E5YBNsuLorRTNluRjIsiee0ZUYmDHUX7hP8ontSxNORFwcNB0ZHVYt_HbmTu3Vw7xM19I2R-mTbinCsJn$ 
> 
> So we will re-check `isReferenced()` again before calling select. I believe we are fine and there's no need to change anything here.

Ah! Indeed. I completely missed that. That check answers my question.

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

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