On Mon, 20 Jun 2022 14:09:27 GMT, Daniel Fuchs <dfu...@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!NVra2H7buQekgeaxLPm4KqgwnuWUqR2SEuUI7sLXSBHPqMEPUncMbU2cLFyRIJqMBjXmPVdIpIxBISM$ . 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
