On Fri, 19 Dec 2025 09:15:31 GMT, Daniel Jeliński <[email protected]> wrote:
>> EunHyunsu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8371903: Add printStackTrace() to GoAwayWithErrorTest > > Option 1: we already do that > [here](https://github.com/openjdk/jdk/blob/574efae488865a22d551b834ee82e5dcd8b31022/src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java#L1388); > once finalStream is set, the connection will not accept more streams. But > there's a short window after the stream is accepted, but before it's added to > the map. > Option 2 won't work either. For all we know, the other thread might not be > actively running on the CPU, so the second forEach would not be enough. > > We need to cooperate with the other thread that adds the stream to the map; > maybe we could put stateLock around `streams.forEach` and `close`, assuming > it won't deadlock anywhere. @djelinski I've added `stateLock` around the `streams.forEach()` and `close()` in `handleGoAwayWithError()`. This should prevent the race condition where streams get added to the map while forEach is running. Since `putStream()` already uses the same `stateLock`, any concurrent stream additions will now block until forEach and close() complete. The test passes locally, but I can't reproduce the intermittent failure (only happens 3/2500 times). Hopefully this fixes it. Just to confirm my understanding: the issue was that streams 3 and 5 could be added to the streams map during forEach execution, so they would miss the `closeAsUnprocessed()` call and get closed with the GOAWAY cause instead. By holding stateLock during forEach and close(), any concurrent `putStream()` calls will block until the connection is closed, at which point they'll fail the `isOpen()` check and the streams will automatically use a new connection. Is that correct? ------------- PR Comment: https://git.openjdk.org/jdk/pull/28632#issuecomment-3674828661
