RFR: 8371903: HttpClient: improve handling of HTTP/2 GOAWAY frames with error code [v5]

[Daniel Fuchs]

On Wed, 10 Dec 2025 14:32:05 GMT, EunHyunsu <duke at openjdk.org> 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