Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HTTP/2: retry requests rejected with REFUSED_STREAM #2081

Merged
merged 7 commits into from
Jan 15, 2024

Conversation

magurotuna
Copy link
Contributor

This patch gets the client to retry HTTP/2 requests rejected with REFUSED_STREAM by inspecting the error detail.

I found the issue with highly concurrent requests in HTTP/2 made to the server that is configured to allow the smaller number of concurrent streams. So for example, a client makes, say, 500 concurrent requests to an HTTP/2 server while the server's max_concurrent_streams is set to 100. In this case the client is allowed to initiate 500 concurrent streams until the initial SETTINGS frame from the server reaches to the client, though the server will reject 400 streams by responding REFUSED_STREAM.
From the standpoint of users of reqwest crate, I would expect the crate to deal with this kind of situation behind the scenes. Actually, as far as I searched, Go and libcurl provides the retry logic when they encounter REFUSED_STREAM. Considering the "ergonomic, batteries-included" characteristic of reqwest, I believe automatic retry would be very nice to have.

Here's a repo containing reproducible code written in a couple different languages. It also has rust-patched code which demonstrates that this patch does fix the issue.
https://github.com/magurotuna/deno-fetch-h2-stream-error-repro

@seanmonstar
Copy link
Owner

Thanks for this! I think this might be something we can do, I'll have to go carefully read the spec about REFUSED_STREAM again. But something I was wondering: assuming that this is a safe thing to retry, could we have a unit test for it? Probably just creating a server with a low max concurrent stream limit, and send a bunch of simple requests. It might mean the support test server might need to pass some options to hyper...

@magurotuna
Copy link
Contributor Author

Sure, I'll add a unit test!

@magurotuna
Copy link
Contributor Author

Added two test cases in f6ad3c6.
I added two, because I found that if we use the existing test server, it just passed this test without the retry logic - after some investigation, it turned out that the hyper server is so fast that we can't reproduce a scenario that I saw when using nginx as a server, that is, server's initial SETTINGS frame reaches the client later than the client having already opened more streams than is allowed.
In order to get a test case that fails without the retry, the new server has been added that can simulate a "slow" server. I confirmed that the test case using this "slow" server does fail without the retry logic.

@magurotuna
Copy link
Contributor Author

A related topic; thinking a bit further about the fact that we get REFUSED_STREAM errors when making too many concurrent requests, doing retry is certainly a mitigation, but another approach would be that we start with the relatively small number of concurrent streams until the server's initial SETTINGS frame arrives. This should reduce the chance of getting rejected with REFUSED_STREAM in the first place.
I've opened a proposal issue in h2 (hyperium/h2#731). But I believe retry is still good to have also.

@magurotuna
Copy link
Contributor Author

Saw CI failed due to missing h2::Error::is_reset method. To address it I bumped h2 crate to 0.3.14 as it was supported since this version.

@magurotuna
Copy link
Contributor Author

Sorry for making many commits to pass the CI. I made a small fix in 895f282 (the port 19999 was introduced mistakenly in f6ad3c6 for me to easily debug it).
I locally confirmed that cargo nextest --locked --workspace have passed.

@seanmonstar seanmonstar merged commit 4ab5fb0 into seanmonstar:master Jan 15, 2024
31 checks passed
@magurotuna magurotuna deleted the refused-stream-retryable branch January 16, 2024 00:29
@magurotuna
Copy link
Contributor Author

Thank you for starting the CI many times! I now understand how the test suite works against many feature flags.

kodiakhq bot pushed a commit to pdylanross/fatigue that referenced this pull request Feb 1, 2024
Bumps reqwest from 0.11.23 to 0.11.24.

Release notes
Sourced from reqwest's releases.

v0.11.24
What's Changed

Add Certificate::from_pem_bundle() to add a bundle.
Add http3_prior_knowledge() to blocking client builder.
Remove Sync bounds requirement for Body::wrap_stream().
Fix HTTP/2 to retry REFUSED_STREAM requests.
Fix instances of converting Url to Uri that could panic.

New Contributors

@​magurotuna made their first contribution in seanmonstar/reqwest#2081
@​michaelciraci made their first contribution in seanmonstar/reqwest#2102
@​basic-bgnr made their first contribution in seanmonstar/reqwest#2110
@​jgraef made their first contribution in seanmonstar/reqwest#2114
@​LucasPickering made their first contribution in seanmonstar/reqwest#2040
@​gibbz00 made their first contribution in seanmonstar/reqwest#2032

Full Changelog: seanmonstar/[email protected]



Changelog
Sourced from reqwest's changelog.

v0.11.24

Add Certificate::from_pem_bundle() to add a bundle.
Add http3_prior_knowledge() to blocking client builder.
Remove Sync bounds requirement for Body::wrap_stream().
Fix HTTP/2 to retry REFUSED_STREAM requests.
Fix instances of converting Url to Uri that could panic.




Commits

c9b4b65 v0.11.24
ddf7f24 Add Certificate::from_pem_bundle() (#2032)
5d8443d fix panic when parsing invalid Url to Uri (#2040)
e3bf090 docs: explain TLS backend features better (#2117)
1bd939b update wasm-streams dependency (#2114)
87cdf12 doc: clarify Cookie API usage (#2111)
b3feff6 Add http3 feature to blocking client (#2110)
2332936 Update cookie crates (#2089)
ef2c8ee Upgrading env_logger dep (#2102)
4ab5fb0 Fix HTTP/2: retry requests rejected with REFUSED_STREAM (#2081)
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Nutomic pushed a commit to Nutomic/reqwest that referenced this pull request Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants