On Wed, 25 Oct 2023 10:18:55 GMT, Daniel Fuchs <[email protected]> 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
