feat: implement shared reqwest::Client for improved retry policies and DOS protection #1285
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| use reqwest::Client; | ||
| use std::sync::OnceLock; | ||
|
|
||
| /// Global shared HTTP client instance with retry policies | ||
| static SHARED_CLIENT: OnceLock<Client> = OnceLock::new(); | ||
|
|
||
| /// Get the shared HTTP client instance | ||
| /// | ||
| /// This client is configured with retry policies and is shared across the entire application. | ||
| /// This allows for global tracking of requests to each domain for DOS protection. | ||
| pub fn get_shared_client() -> &'static Client { | ||
| SHARED_CLIENT.get_or_init(|| { | ||
| Client::builder() | ||
| .retry( | ||
| reqwest::retry::for_all() | ||
| .classify_fn(|req_rep| { | ||
| match req_rep.status() { | ||
| // Server errors and rate limiting | ||
| Some(s) | ||
| if s.is_server_error() | ||
| || s == reqwest::StatusCode::TOO_MANY_REQUESTS => | ||
| { | ||
| req_rep.retryable() | ||
| } | ||
| // Network errors | ||
| None => req_rep.retryable(), | ||
| _ => req_rep.success(), | ||
| } | ||
| }) | ||
| .max_retries_per_request(5) | ||
| .max_extra_load(5.0), | ||
| ) | ||
| .timeout(std::time::Duration::from_secs(30)) | ||
| .build() | ||
| .expect("Failed to create shared HTTP client") | ||
| }) | ||
| } | ||
|
|
||
| /// Get a retryable client for specific hosts | ||
| /// | ||
| /// This function returns the shared client which has global retry tracking. | ||
| /// All requests use the same client instance for consistent DOS protection. | ||
| pub fn get_retryable_client(_host: String) -> Result<&'static Client, reqwest::Error> { | ||
| Ok(get_shared_client()) | ||
| } | ||
|
Comment on lines
+39
to
+45
There was a problem hiding this comment. Misleading API: unused parameter and infallible The function signature has two issues that make the API confusing:
Consider one of these approaches: Option 1 (Recommended): Simplify the API If the host parameter truly isn't needed, remove it and change the return type: -pub fn get_retryable_client(_host: String) -> Result<&'static Client, reqwest::Error> {
- Ok(get_shared_client())
+pub fn get_retryable_client() -> &'static Client {
+ get_shared_client()
}Then update callers in Option 2: Remove the wrapper entirely If Option 3: Document the intent If the signature is intentionally kept for future extensibility or API stability, add a comment explaining why the parameter is unused and why a
🤖 Prompt for AI Agents |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify the
reqwest::retry::for_all()API and consider error handling.The retry configuration looks reasonable, but I'd like to verify the API usage:
The
reqwest::retry::for_all()call on line 15 should be confirmed to exist in the version ofreqwestbeing used. Standardreqwestdoesn't include a built-in retry module by default—this typically requires thereqwest-retrycrate or similar middleware.The
.expect()on line 35 will panic if client creation fails during initialization. While this may be acceptable for a critical shared resource, consider whether a more graceful startup failure would be appropriate.Run the following script to verify the reqwest retry API:
Also, please verify the retry API documentation:
🏁 Script executed:
Length of output: 361
🌐 Web query:
💡 Result:
Do you mean the built-in reqwest retry API (reqwest::retry) or the reqwest-retry middleware (crate reqwest-retry)? Which crate and version are you using?
For reference:
Reply with which one (and the version) and I’ll find the exact for_all documentation or show how to use it.
Excellent! I found critical information. The retry module was added to reqwest via PR #2763, which includes a new ClientBuilder::retry() method and a reqwest::retry module to build a retry policy, with a for_host() function and classify_fn() method. However, I need to verify when this was released and whether it's in reqwest 0.12.7.Perfect! I found it. As of version 0.12.23, reqwest includes automatic retry functionality - which means reqwest 0.12.7 does NOT have the built-in retry module. The PR #2763 added it, but it was only released in 0.12.23 or later.
Let me verify the exact API by checking if
for_all()is the correct method name:Looking at the search results, I found a critical issue. The code uses
reqwest::retry::for_all(), but the built-in retry functionality was only added to reqwest starting in version 0.12.23, and the version specified in Cargo.toml is 0.12.7. Additionally, the API examples showfor_host(), notfor_all(). Let me check the actual imports in the source file to be certain:Based on my research, I now have sufficient evidence to write the rewritten review comment. The critical issue is that reqwest's automatic retry functionality was only added in version 0.12.23, but the codebase uses reqwest 0.12.7. The code callsreqwest::retry::for_all(), which does not exist in that version and would not compile.Code will not compile with reqwest 0.12.7—retry module is unavailable.
The code uses
reqwest::retry::for_all()on line 15, but automatic retry functionality was added to reqwest only in version 0.12.23. The Cargo.toml specifies reqwest 0.12.7, which does not include thereqwest::retrymodule.Upgrade reqwest to at least 0.12.23 to enable the built-in retry API. Additionally, verify that the
for_all()method is the correct API (the PR documentation referencesfor_host()for host-specific retry policies).🤖 Prompt for AI Agents