On Sun, 26 Jun 2022 06:08:07 GMT, Jaikiran Pai <j...@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!JL9hqB4ORwRVy6IHKdvtBmdyVOqqyxumiBnrW7bOxTAJzd_Cjew0bUQwyIgLhB3Q5r4JFXtMG1MXI3ZqfohFozk$ > . 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!JL9hqB4ORwRVy6IHKdvtBmdyVOqqyxumiBnrW7bOxTAJzd_Cjew0bUQwyIgLhB3Q5r4JFXtMG1MXI3ZqATA6QVk$ So we will re-check `isReferenced()` again before calling select. I believe we are fine and there's no need to change anything here. ------------- PR: https://git.openjdk.org/jdk/pull/9217
