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. Changes requested by dfuchs (Reviewer). test/jdk/java/net/httpclient/http2/GoAwayWithErrorTest.java line 114: > 112: if (!goAwaySentLatch.await(5, TimeUnit.SECONDS)) { > 113: throw new AssertionError("GOAWAY not sent in time"); > 114: } You should at least use Utils.adjustTimeout() here to make sure the timeout is adjusted when the timeout factor increases. Some configurations - like running with debug builds, can increase latency times significantly. test/jdk/java/net/httpclient/http2/GoAwayWithErrorTest.java line 123: > 121: > 122: CompletableFuture.allOf(first, second, third) > 123: .orTimeout(20, TimeUnit.SECONDS) Same here. test/jdk/java/net/httpclient/http2/GoAwayWithErrorTest.java line 188: > 186: secondRequestHandled.set(true); > 187: System.out.println("Handler: successfully handling retried > request"); > 188: exchange.sendResponseHeaders(200, 0); This is an Http2Handler - so IIRC 0 means size unknown (not that it matters much in the case of HTTP/2 - it means the size will be known when response body is closed, and no content-length header will be sent in the headers). Typically - if some data was being sent, it means that an additional data frame of size 0 and carrying the END_STREAM flag would be sent when the body is closed. We're sending no body - so we're only going to send the empty DATA_FRAME. IIRC - if you had passed -1 (meaning 0 bytes in reply) - the END_STREAM would be carried by the HEADER frame instead and no empty DATA_FRAME would be sent. This is just for the record. I think what you have is good. ------------- PR Review: https://git.openjdk.org/jdk/pull/28632#pullrequestreview-3563260020 PR Review Comment: https://git.openjdk.org/jdk/pull/28632#discussion_r2607222458 PR Review Comment: https://git.openjdk.org/jdk/pull/28632#discussion_r2607224602 PR Review Comment: https://git.openjdk.org/jdk/pull/28632#discussion_r2607248080
