On Tue, 24 Oct 2023 09:46:40 GMT, Conor Cleary <[email protected]> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16340#discussion_r1371437292
