On Tue, 6 Aug 2024 09:10:13 GMT, Jaikiran Pai <j...@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
