On Fri, 28 Aug 2026 09:16:19 GMT, Daniel Fuchs <[email protected]> wrote:
>> `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. > > It's a good point. My worry here is that the connection may be removed from > the list of connections without being properly terminated. Also we don't seem > to check whether the idleTimeoutEvent has been cancelled. Are we sure that > the event cannot be cancelled? The actions performed by the event when it is > fired and finds that the connection should be terminated are much more > involved than just calling finalStream. And the caller seems to only remove > the connection from the list. b4dc8c03a6c introduces further checks for idleness. >> 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. > > Good point again. Holding the lock while doing this might not be a good idea. > Now that I think about it, I guess that your reasoning was that the event > would eventually fire and terminate the connection later? So your concern was > just to prevent it from being reused? > > Then maybe we need to double check that the connection is still idle before > marking it finalStream. Then just let the event alone and return false and > the event will eventually fire and do its job. b4dc8c03a6c introduces further checks for idleness. I've reverted the `volatile` copy, since it is only read & written while the lock is held. > I guess that your reasoning was that the event would eventually fire and > terminate the connection later? So your concern was just to prevent it from > being reused? Correct. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3880387475 PR Review Comment: https://git.openjdk.org/jdk/pull/32545#discussion_r3880392660
