RFR: 8318492: Http2ClientImpl should attempt to close and remove connection in stop()

[Jaikiran Pai]

On Wed, 25 Oct 2023 10:18:55 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

>> src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java line 231:
>> 
>>> 229:         }
>>> 230:         do {
>>> 231:             connections.values().removeIf(this::close);
>> 
>> Hello Conor, I'm just wondering if doing this will have some unexpected race or some such similar issue in the `connections` management.
>> 
>> Right now, before this change, to remove a connection from a pool we call the `Http2ClientImpl.removeFromPool(conn)` which then obtains a `connectionPoolLock` lock before removing it from the pool. In this proposed change, we are bypassing the `connectionPoolLock` lock. The `connections` itself is a `ConcurrentHashMap` but its usages are currently guarded through the `connectionPoolLock`, so I wonder if we should be calling `removeFromPool` method here instead of `removeIf`, something like:
>> 
>> 
>>         do {
>>             connections.values().forEach((c) -> {
>>                 if (close(c)) {
>>                     removeFromPool(c);
>>                 }
>>             });
>>         } while (!connections.isEmpty());
>> 
>> I haven't done any kind of testing on this use of `removeFromPool` - it could be that it might have issues of its own.
>
> @jaikiran we are stopping the client at that point. The main reason for the connectionPoolLock is to avoid adding connections to the pool or handing off connections from the pool if `stopping` has been set to true. `removeFromPool` does nothing more than logging and removing the connection from the pool. Locking is necessary here because we don't want a connection to be handed off if it's going to be removed, but that logic is not necessary at client stop. Of course this only works because our connections map is a ConcurrentHashMap, but so is removing from within a loop. So I believe using removeIf here is appropriate.

Thank you for that additional detail Daniel.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16340#discussion_r1371634222