On Wed, 10 Dec 2025 23:34:02 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: Use Utils.adjustTimeout() in GoAwayWithErrorTest test/jdk/java/net/httpclient/http2/GoAwayWithErrorTest.java line 113: > 111: request, HttpResponse.BodyHandlers.ofString()); > 112: > 113: if (!goAwaySentLatch.await(Utils.adjustTimeout(5), > TimeUnit.SECONDS)) { I suggest that we replace this with just `goAwaySentLatch.await()`. Our experience with arbitrary timeouts in tests has been that they fail intermittently when those timeouts aren't met. In this case there's no reason for the await() to complete in a specific amount of time. If it doesn't complete for the entire test duration (which by default is 2 minutes), then that very likely is a bug and will need investigation. jtreg test failure handler has the infrastructure to collect the thread dumps when such timeouts happen. So leaving out these arbitrary timeouts helps in such cases. So I think we should change this line to just: goAwaySentLatch.await(); test/jdk/java/net/httpclient/http2/GoAwayWithErrorTest.java line 124: > 122: > 123: CompletableFuture.allOf(first, second, third) > 124: .orTimeout(Utils.adjustTimeout(20), TimeUnit.SECONDS) Similar to the review comment above, we shouldn't use any timeout here and instead should just wait for the requests to complete. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28632#discussion_r2610414348 PR Review Comment: https://git.openjdk.org/jdk/pull/28632#discussion_r2610416068
