8335181: Incorrect handling of HTTP/2 GOAWAY frames in HttpClient #20442
Conversation
|
👋 Welcome back jpai! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
I'm investigating an intermittent failure that happened once with this change. I will update this PR once I have additional details. |
…ntroller 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 The test server has also been enhanced to send out Finally, with the support for GOAWAY handling in the client, it's now possible that a stream might be unregisters from the 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. |
| // A REFUSED_STREAM error code implies that the stream wasn't processed by the | ||
| // peer and the client is free to retry the request afresh. | ||
| // Here we arrange for the request to be retried. | ||
| markUnprocessedByPeer(); | ||
| errorRef.compareAndSet(null, new IOException("request not processed by peer")); | ||
| if (debug.on()) { | ||
| debug.log("request unprocessed by peer (REFUSED_STREAM) " + this.request); | ||
| } |
There was a problem hiding this comment.
Hmm.... Should we just call:
closeAsUnprocessed():
return;
here?
There was a problem hiding this comment.
I've now updated this PR to follow this suggestion. The finally block which does connection.decrementStreamsCount(streamid); is needed (otherwise the HttpClient close() doesn't complete), so I haven't changed that part.
With the latest state of this PR, a test-repeat 50 of java/net/httpclient tests continues to pass without any failures.
dfuch
left a comment
dfuch
left a comment
There was a problem hiding this comment.
Other than the previous comment about calling closeAsUnprocessed() this looks good to me.
Reviewed-by: tschatzl
Reviewed-by: dholmes, stefank
…buteSet Reviewed-by: achung, honkar, prr
Reviewed-by: mgronlun
Reviewed-by: hannesw
Co-authored-by: Fei Yang <fyang@openjdk.org> Reviewed-by: vlivanov, adinn
Reviewed-by: rgiulietti
Reviewed-by: alanb, liach
Reviewed-by: aivanov, stuefe, prr
Reviewed-by: clanger, stuefe
…o separate class Reviewed-by: kvn, epeter
…ethod complexity Reviewed-by: jpai
Reviewed-by: mgronlun
Reviewed-by: iwalulya, tschatzl
….java fails with release build Reviewed-by: chagedorn, thartmann
…after JDK-8338058 Reviewed-by: mdoerr, clanger
Reviewed-by: vlivanov, shade
Reviewed-by: stefank, kbarrett
Reviewed-by: prr, lbourges
…ptorLookupError Reviewed-by: jlahoda
…ssing Reviewed-by: jwaters, erikj, shade
…g binary Reviewed-by: vromero
Reviewed-by: stuefe, shade, ccheung
Reviewed-by: vromero
…xception_entry() Reviewed-by: thartmann
Reviewed-by: kbarrett, tschatzl, shade
Reviewed-by: dholmes, shade
Reviewed-by: never
Reviewed-by: cjplummer
Reviewed-by: dholmes
Reviewed-by: kbarrett, shade
Reviewed-by: ihse, dholmes, jwaters
…version Reviewed-by: chagedorn, kvn
…oken after JDK-8333840 Reviewed-by: chagedorn, thartmann, kvn
…AD_MUTEX_INITIALIZER Reviewed-by: dholmes, dlong
…on_failure) failed: Has low-order bits set Reviewed-by: stefank, eosterlund, aboldtch
Reviewed-by: rkennke, ysr, wkemper
…n failing due to a LinkageError or other errors Reviewed-by: alanb
|
Looks like I did some mistake with a "git merge" command locally when trying to refresh this PR with latest mainline changes. I'll close this one and open a new one with only the relevant commits. |
20 participants
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.HttpClientdoesn'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.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20442/head:pull/20442$ git checkout pull/20442Update a local copy of the PR:
$ git checkout pull/20442$ git pull https://git.openjdk.org/jdk.git pull/20442/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20442View PR using the GUI difftool:
$ git pr show -t 20442Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20442.diff
Webrev
Link to Webrev Comment