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

[Daniel Fuchs]

On Wed, 25 Oct 2023 09:26:29 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> If Http2ClientImpl::stop() is called, it will loop through the connections map and call the close() method on each connection. The changes in this PR make sure that the connection thread (where close is called from) removes the `Http2Connection` from the connection pool to prevent inactive connections from remaining in there rather than just closing it. 
>> 
>> Even though the connection can be and usually is removed from the pool in other places (reader or writer thread), this change ensures removal from the pool in cases where the connection is shutdown by an exception, reset, etc. The issue was first spotted when a RST_STREAM triggered repeated attempts to remove idle connections from the pool before a regular shutdown could occur.
>
> 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.

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

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