On Wed, 10 Dec 2025 14:32:05 GMT, EunHyunsu <[email protected]> wrote:
>> ### Problem >> >> When the HTTP/2 client receives a GOAWAY frame with a non-zero error code, >> the current implementation discards both the error code and debug data. >> Users only see generic "Connection closed by peer" errors without any >> information about why the server terminated the connection. >> >> ### Solution >> >> Per [RFC 9113 >> §5.4.1](https://www.rfc-editor.org/rfc/rfc9113.html#section-5.4.1), a GOAWAY >> frame with a non-zero error code indicates a connection error requiring >> immediate closure. This fix: >> >> 1. **Distinguishes** graceful shutdown (NO_ERROR) from connection errors >> 2. **Preserves** error code and debug data in exception messages >> 3. **Categorizes** streams based on `lastStreamId`: >> - Streams with ID > `lastStreamId`: Marked as unprocessed for automatic >> retry >> - Streams with ID ≤ `lastStreamId`: Failed with detailed error >> information >> >> ### Changes >> >> **Core Implementation** (`Http2Connection.java`): >> - Modified `handleGoAway()` to check error code and route appropriately >> - Added `handleGoAwayWithError()` method that: >> - Extracts error code and debug data from GOAWAY frame >> - Creates meaningful error messages with error name, hex code, and debug >> data >> - Properly categorizes streams for retry or failure >> >> **Test Infrastructure**: >> - `Http2TestServerConnection.sendGoAway(int, int, byte[])`: Supports >> custom error codes >> - `Http2TestExchangeImpl.getServerConnection()`: Accessor for test handlers >> - `GoAwayWithErrorTest`: Verifies proper error propagation >> >> ### Example >> >> **Before:** >> IOException: Connection closed by peer >> >> **After:** >> IOException: Received GOAWAY with error code Protocol error (0x1): Invalid >> HEADERS frame >> >> ### Testing >> >> - New `GoAwayWithErrorTest` passes >> - Existing HTTP/2 tests unaffected (NO_ERROR path unchanged) >> - Backward compatible (no public API changes) > > EunHyunsu has updated the pull request incrementally with one additional > commit since the last revision: > > 8371903: Move goAwaySentLatch.await() back to original position > > Move the await call to after the first request but before the second > and third requests, ensuring only one initial connection is created > and the other requests are properly retried. Marked as reviewed by djelinski (Reviewer). Right, we only create one connection initially, and requests 2 and 3 are usually sent over the first connection before they are retried on new connections. This can be observed in the server logs as `resetting stream 3 as REFUSED_STREAM`. Ideally the server should not send anything after sending the GOAWAY frame with error, but I think what we have is good enough, ------------- PR Review: https://git.openjdk.org/jdk/pull/28632#pullrequestreview-3562866791 PR Comment: https://git.openjdk.org/jdk/pull/28632#issuecomment-3637384654
