Decouple ObjectStore from Reqwest #7183
Conversation
|
|
||
| /// The [`Body`] of an [`HttpRequest`] | ||
| #[derive(Debug, Clone)] | ||
| pub struct HttpRequestBody(Inner); |
There was a problem hiding this comment.
We define fixed body types as we need HttpClient to be dyn-compatible (it also makes things slightly easier to follow)
| } | ||
| } | ||
|
|
||
| pub(crate) struct HttpRequestBuilder { |
There was a problem hiding this comment.
This is a crate-private builder that provides an interface similar to reqwest but which can then be used with the HttpClient. Unfortunately reqwest::Request is largely opaque and I couldn't see an obvious way to make use of it.
| } | ||
| } | ||
|
|
||
| pub(crate) fn add_query_pairs<I, K, V>(uri: &mut Uri, query_pairs: I) |
There was a problem hiding this comment.
It is a shame that http::Request uses http::Uri which lacks the ergonomic niceities of Url, so we require some additional shenanigans
|
|
||
| #[derive(Debug, Copy, Clone, PartialEq, Eq)] | ||
| #[non_exhaustive] | ||
| pub enum HttpErrorKind { |
There was a problem hiding this comment.
This allows the retry logic to work on top of arbitrary HttpClients
| inner: RequestError, | ||
| } | ||
|
|
||
| impl std::fmt::Display for RetryError { |
There was a problem hiding this comment.
This somewhat lays the ground work for #6377
| } | ||
|
|
||
| impl RetryExt for reqwest::RequestBuilder { | ||
| impl RetryExt for HttpRequestBuilder { |
There was a problem hiding this comment.
We could theoretically remove the extension trait, as HttpRequestBuilder, is not a foreign type. I opted to keep it for now though.
| }; | ||
|
|
||
| // Reqwest error variants aren't great, attempt to refine them | ||
| let mut source = e.source(); |
There was a problem hiding this comment.
This logic is moved out of retry.rs and reframed in terms of HttpErrorKind
|
Is it right to say that instead of
? I wonder if we need the custom service type though or if we could use something like |
|
Also I think the keyword that is used by other libs for this kind of refactor is called "sans-io", i.e. to make no assumption about the underlying IO layer or framework. |
I think we just depend on
I went back and forth on this, the reason for preferring a custom service was the following:
Ultimately defining a very simple async_trait in terms of |
| /// An HTTP protocol error | ||
| /// | ||
| /// Clients should return this when an HTTP request fails to be completed, e.g. because | ||
| /// of a connection issue. This does **not** include HTTP requests that are return |
There was a problem hiding this comment.
FWIW the way reqwest conflates these two scenarios I have always found immensely confusing
| HttpErrorKind::Connect | HttpErrorKind::Request => true, // Request not sent, can retry | ||
| HttpErrorKind::Timeout => is_idempotent, | ||
| HttpErrorKind::Unknown | ||
| | HttpErrorKind::Interrupted |
There was a problem hiding this comment.
Note: I think we should retry Interrupted if request is_idempotent
Which issue does this PR close?
Closes #6056
Related to #6818
Related to #7171
Rationale for this change
See tickets
What changes are included in this PR?
Are there any user-facing changes?