RFR: 8335181: Incorrect handling of HTTP/2 GOAWAY frames in HttpClient [v3]

[Jaikiran Pai]

On Tue, 6 Aug 2024 09:10:13 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which fixes the issue noted in https://bugs.openjdk.org/browse/JDK-8335181?
>> 
>> As noted in that issue, the current implementation in the `java.net.http.HttpClient` doesn't correctly handle an incoming GOAWAY frame. The HTTP3 RFC https://www.rfc-editor.org/rfc/rfc9113#name-goaway notes the specifics on what the expectations are when an endpoint receives a GOAWAY frame from the peer.
>> 
>> Before the changes proposed in this PR, the HttpClient implementation would (incorrectly) shutdown the connection and abort requests when a GOAWAY frame was received. The changes in this PR fixes that by retrying relevant unprocessed requests (if any) and not initiating any new streams on the connection.
>> 
>> A new test has been introduced to exercise this detail. The test continues to pass along with other existing tests. tier testing as well as a repeated testing (with test-repeat 50) is currently in progress with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   simplify test, handle REFUSED_STREAM on the client side, fix WindowController assertion

I've updated the PR to address the intermittent failure in the new test. The test now consistently passes in several hundred repeat runs.

In the updated PR, I have updated the client to even handle `REFUSED_STREAM` error code in a `RST_FRAME`. The RFC states that `REFUSED_STREAM` indicates that the server hasn't processed the request and the client is free to retry the request afresh. The HttpClient code has been updated to handle `REFUSED_STREAM` in a similar manner to `GOAWAY`, except that in the `REFUSED_STREAM` case we only mark (and retry) that current stream as unprocessed.

The test server has also been enhanced to send out `REFUSED_STREAM` errors in `RST_FRAME` to exercise this change.

Finally, with the support for GOAWAY handling in the client, it's now possible that a stream might be unregisters from the `WindowController` before request headers are sent on that stream and thus the `WindowController` required an update to not "assert" the presence of the stream being removed in the `streams` map.

With all these changes in the latest PR, this test (and all existing tests in java/net/httpclient) continue to pass in several test repeat runs.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/20442#issuecomment-2270784420