On Thu, 27 Aug 2026 09:46:19 GMT, Daniel Fuchs <[email protected]> wrote:
>> Volkan Yazici has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Copy while accessing the `idleConnectionTimeoutEvent`, which is `volatile` > > src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java > line 1564: > >> 1562: setFinalStream(); >> 1563: return false; >> 1564: } > > This does not look completely right - maybe it should do: > > if (timedOut) idleConnectionTimeoutEvent.handle()? `Http2Connection.IdleConnectionTimeoutEvent#handle()` re-acquires the state lock, marks the idle connection for termination, releases its inner lock acquisition, and tries to terminate the connection using `connTerminator.idleTimedOut()`, yet the lock is still held by the outer caller. That is, it will result in terminating the connection while the lock is still held. This doesn't match what `handle()` is trying to do. Are you sure about this change? FWIW, when I change the code in the way you suggested, the newly added tests still pass. > src/java.net.http/share/classes/jdk/internal/net/http/Http3Connection.java > line 907: > >> 905: setFinalStream(); >> 906: return false; >> 907: } > > In Http3Connection the idleConnectionTimeoutEvent is volatile - so please add > a local variable to not read it more than once (or investigate whether the > `volatile` keyword could/should be removed). > Also it seems here we should also simply call handle() on the event if the > timeout has expired and let the rest of the method unfold. Fixed the `volatile` issue in a8162a4e930. Using `Http3Connection.IdleConnectionTimeoutEvent#handle()` has the same problem I've shared earlier for H2: connection gets terminated while the lock is still held. Are you sure about this change? FWIW, when I change the code in the way you suggested, the newly added tests still pass. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3879032406 PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3879052002
