Add retries to Help proxy request #489
Conversation
|
|
||
| // Conversion helper between reqwest and actix-web's `HeaderValue` | ||
| // | ||
| // Both point to a re-exported `HeaderValue` from the http crate, but they come from | ||
| // different versions of the http crate, so they look different to Rust and we need a | ||
| // small conversion helper. | ||
| // - reqwest re-exports http 1.0.0's `HeaderValue` | ||
| // - actix-web re-exports http 0.2.7's `HeaderValue` | ||
| fn convert_header_value(x: &reqwest::header::HeaderValue) -> actix_web::http::header::HeaderValue { | ||
| let out = actix_web::http::header::HeaderValue::from_bytes(x.as_bytes()); | ||
|
|
||
| // We've checked these are the same underlying structure, so assert that this works. | ||
| let mut out = out.unwrap(); | ||
|
|
||
| // Set the one other field that defaults to `false`, just in case. | ||
| out.set_sensitive(x.is_sensitive()); | ||
|
|
||
| out | ||
| } |
There was a problem hiding this comment.
Joint work with @DavisVaughan who discovered that:
- actix-web re-exports http 0.2.7's
HeaderValue - reqwest re-exports http 1.0.0's
HeaderValue
Here is the commit where reqwest changed this:
seanmonstar/reqwest@eb94f26
crates/ark/src/help_proxy.rs
Outdated
| // Set up reqwest client with 3 retries. | ||
| let retry_policy = ExponentialBackoff::builder().build_with_max_retries(3); | ||
| let client = ClientBuilder::new(Client::new()) | ||
| .with(RetryTransientMiddleware::new_with_policy(retry_policy)) | ||
| .build(); | ||
|
|
There was a problem hiding this comment.
Shall we start with 3 retries?
| use reqwest::Client; | ||
| use reqwest_middleware::ClientBuilder; | ||
| use reqwest_retry::policies::ExponentialBackoff; | ||
| use reqwest_retry::RetryTransientMiddleware; |
There was a problem hiding this comment.
The reqwest crate doesn't have any retry mechanism, but we can use these related crates to set up middleware and retries.
| reqwest-retry = "0.6.1" | ||
| reqwest-middleware = "0.3.3" |
There was a problem hiding this comment.
Good news is that both reqwest-retry and reqwest-middleware are smart enough to set default-features = false for their reqwest dependency
https://github.com/TrueLayer/reqwest-middleware/blob/6cd4eb93f8567bece4526f050cd6f7c2bde7371e/reqwest-middleware/Cargo.toml#L24
https://github.com/TrueLayer/reqwest-middleware/blob/6cd4eb93f8567bece4526f050cd6f7c2bde7371e/reqwest-retry/Cargo.toml#L19
Making #487 still viable even after this
| @@ -128,8 +132,14 @@ async fn proxy_request(req: HttpRequest, app_state: web::Data<AppState>) -> Http | |||
| // Add query from original request back to URL. | |||
| target_url.set_query(Some(query)); | |||
|
|
|||
| // Set up reqwest client with 3 retries. | |||
| let retry_policy = ExponentialBackoff::builder().build_with_max_retries(3); | |||
There was a problem hiding this comment.
For the curious, here are the other default options that get set for the backoff:
https://docs.rs/retry-policies/0.4.0/src/retry_policies/policies/exponential_backoff.rs.html#148-151
Definitions
https://docs.rs/reqwest-retry/latest/reqwest_retry/policies/struct.ExponentialBackoff.html#fields
Co-authored-by: Davis Vaughan <davis@rstudio.com>
Addresses posit-dev/positron#3753
Folks in that issue⤴️ are experiencing problems seeing R Help pages in the Help pane. Some things we can notice:
Given all that, seems like we may just need to retry the request, or at the least that is a first possible step as outlined by @DavisVaughan in posit-dev/positron#3753 (comment).