-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add retry policies #2763
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
Add retry policies #2763
Conversation
b69e1da to
db25a40
Compare
dd59862 to
aaedda8
Compare
1933bba to
9e41df0
Compare
Bumps reqwest from 0.12.22 to 0.12.23. Release notes Sourced from reqwest's releases. v0.12.23 tl;dr 🇺🇩🇸 Add ClientBuilder::unix_socket(path) option that will force all requests over that Unix Domain Socket. 🔁 Add ClientBuilder::retries(policy) and reqwest::retry::Builder to configure automatic retries. Add ClientBuilder::dns_resolver2() with more ergonomic argument bounds, allowing more resolver implementations. Add http3_* options to blocking::ClientBuilder. Fix default TCP timeout values to enabled and faster. Fix SOCKS proxies to default to port 1080 (wasm) Add cache methods to RequestBuilder. What's Changed Minimize package size by @weiznich in seanmonstar/reqwest#2759 chore(dev-dependencies): bump brotli by @seanmonstar in seanmonstar/reqwest#2760 upgrade hickory-dns to 0.25 by @seanmonstar in seanmonstar/reqwest#2761 Re-expose http3 options in blocking::clientBuilder by @ducaale in seanmonstar/reqwest#2770 fix(proxy): restore default port 1080 for SOCKS proxies without explicit port by @0x676e67 in seanmonstar/reqwest#2771 ci: use msrv-aware cargo in msrv job by @seanmonstar in seanmonstar/reqwest#2779 feat: add request cache option for wasm by @Spxg in seanmonstar/reqwest#2775 style(client): use std::task::ready! macro to simplify Poll branch match by @0x676e67 in seanmonstar/reqwest#2781 fix: add default tcp keepalive and user_timeout values by @seanmonstar in seanmonstar/reqwest#2780 feat: add unix_socket() option to client builder by @seanmonstar in seanmonstar/reqwest#2624 Add retry policies by @seanmonstar in seanmonstar/reqwest#2763 refactor: loosen retry for_host parameter bounds by @Enduriel in seanmonstar/reqwest#2792 feat: add dns_resolver2 that is more ergonomic and flexible by @seanmonstar in seanmonstar/reqwest#2793 Prepare v0.12.23 by @seanmonstar in seanmonstar/reqwest#2795 New Contributors @weiznich made their first contribution in seanmonstar/reqwest#2759 @Spxg made their first contribution in seanmonstar/reqwest#2775 @Enduriel made their first contribution in seanmonstar/reqwest#2792 Full Changelog: seanmonstar/[email protected] Changelog Sourced from reqwest's changelog. v0.12.23 Add ClientBuilder::unix_socket(path) option that will force all requests over that Unix Domain Socket. Add ClientBuilder::retries(policy) and reqwest::retry::Builder to configure automatic retries. Add ClientBuilder::dns_resolver2() with more ergonomic argument bounds, allowing more resolver implementations. Add http3_* options to blocking::ClientBuilder. Fix default TCP timeout values to enabled and faster. Fix SOCKS proxies to default to port 1080 (wasm) Add cache methods to RequestBuilder. Commits ae7375b v0.12.23 9aacdc1 feat: add dns_resolver2 that is more ergonomic and flexible (#2793) 221be11 refactor: loosen retry for_host parameter bounds (#2792) acd1b05 feat: add reqwest::retry policies (#2763) 54b6022 feat: add ClientBuilder::unix_socket() option (#2624) 6358cef fix: add default tcp keepalive and user_timeout values (#2780) 21226a5 style(client): use std::task::ready! macro to simplify Poll branch matching... 82086e7 feat: add request cache options for wasm (#2775) 2a0f7a3 ci: use msrv-aware cargo in msrv job (#2779) f186803 fix(proxy): restore default port 1080 for SOCKS proxies without explicit port... 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)
Do I understand correctly with 0.12.22 retries were not possible at all, and now the default policy is to enable some retries? In that case it's a breaking change. I alone have two cases affected by this, one is API client that maintains fixed request rate and will be banned as soon as it exceeds the maximum, which becomes possible after suddenly introduced retries. Another case is generic link checker which a) maintains strict politeness policy of no more than 1/3RPS (it implements its own retry and redirect handling, and each request must as well be subject to throttling) and b) must strictly report each failure as-is. I suggest to yank this release and re-release as 0.13. And if I may express an opinion, I'd strongly prefer any extra logic on top of base HTTP request functionality to be opt-in. That includes redirect handling, but since you already have these enabled by default, the statement may be limited to "any new extra logic". Otherwise, each library user would have to go through |
Nope. This is behavior that has been inside the reqwest client for a long while, and now you can finally customize it. It's never automatically interpreted any status code, but in something like HTTP/2 where a signal can indicate "I'm closing this connection and everything above stream id 123 has been ignored", then reqwest would retry those on a new connection. That's the default. Now, you can disable that by providing a |
|
Thank you for the explanation! This way it looks good wrt. breaking changes, and I'm no longer sure whether it's a good or bad default. From the point of strict rate limiting it is probably not, but at the same time it's a kind of failure you don't expect without having complete understanding of how persistent HTTP connections work. I may only suggest now to add a few words explaining what |
reqwest 0.1.23 now allows to customize retry policy. The default has not changed and is to only retry "low level protocol NACKs", which, as explained in [1], implies recreating presistent HTTP/2 connections transparently when needed. That mostly does not affect us as we create a new Client for each request, and thus do not have any persistent connections, but still - be explicit that we don't expect any retries. - handle the rare cases that the request gets such low level protocol NACK right away - report an error, do not retry [1] seanmonstar/reqwest#2763 (comment)
This adds retry policies, somewhat kind of like redirect policies. This includes a new
ClientBuilder::retry()method, and areqwest::retrymodule to build a retry policy.Example
Closes #316
Closes #2577