-
Notifications
You must be signed in to change notification settings - Fork 523
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
feat(core): abstract HttpFetch trait for raw http client #5184
Changes from 9 commits
bc83fdc
efac5a3
b9f886f
5c6d065
bc21b1d
5c2d3c9
2281a2b
371fd0f
bb92562
bcaa642
86e2d62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,22 +19,36 @@ use std::fmt::Debug; | |
use std::fmt::Formatter; | ||
use std::future; | ||
use std::mem; | ||
use std::ops::Deref; | ||
use std::str::FromStr; | ||
use std::sync::Arc; | ||
|
||
use futures::Future; | ||
use futures::TryStreamExt; | ||
use http::Request; | ||
use http::Response; | ||
use once_cell::sync::Lazy; | ||
use raw::oio::Read; | ||
|
||
use super::parse_content_encoding; | ||
use super::parse_content_length; | ||
use super::HttpBody; | ||
use crate::raw::*; | ||
use crate::*; | ||
|
||
/// Http client used across opendal for loading credentials. | ||
/// This is merely a temporary solution because reqsign requires a reqwest client to be passed. | ||
/// We will remove it after the next major version of reqsign, which will enable users to provide their own client. | ||
#[allow(dead_code)] | ||
pub(crate) static GLOBAL_REQWEST_CLIENT: Lazy<reqwest::Client> = Lazy::new(reqwest::Client::new); | ||
|
||
/// HttpFetcher is a type erased [`HttpFetch`]. | ||
pub type HttpFetcher = Box<dyn HttpFetchDyn>; | ||
|
||
/// HttpClient that used across opendal. | ||
#[derive(Clone)] | ||
pub struct HttpClient { | ||
client: reqwest::Client, | ||
fetcher: Arc<HttpFetcher>, | ||
} | ||
|
||
/// We don't want users to know details about our clients. | ||
|
@@ -47,26 +61,24 @@ impl Debug for HttpClient { | |
impl HttpClient { | ||
/// Create a new http client in async context. | ||
pub fn new() -> Result<Self> { | ||
Self::build(reqwest::ClientBuilder::new()) | ||
let fetcher = Arc::new(Box::new(reqwest::Client::new()) as HttpFetcher); | ||
Ok(Self { fetcher }) | ||
} | ||
|
||
/// Construct `Self` with given [`reqwest::Client`] | ||
pub fn with(client: reqwest::Client) -> Self { | ||
Self { client } | ||
pub fn with(client: impl HttpFetch) -> Self { | ||
let fetcher = Arc::new(Box::new(client) as HttpFetcher); | ||
Self { fetcher } | ||
} | ||
|
||
/// Build a new http client in async context. | ||
#[deprecated] | ||
pub fn build(builder: reqwest::ClientBuilder) -> Result<Self> { | ||
Ok(Self { | ||
client: builder.build().map_err(|err| { | ||
Error::new(ErrorKind::Unexpected, "http client build failed").set_source(err) | ||
})?, | ||
}) | ||
} | ||
|
||
/// Get the async client from http client. | ||
pub fn client(&self) -> reqwest::Client { | ||
self.client.clone() | ||
let client = builder.build().map_err(|err| { | ||
Error::new(ErrorKind::Unexpected, "http client build failed").set_source(err) | ||
})?; | ||
let fetcher = Arc::new(Box::new(client) as HttpFetcher); | ||
Ok(Self { fetcher }) | ||
} | ||
|
||
/// Send a request in async way. | ||
|
@@ -78,6 +90,41 @@ impl HttpClient { | |
|
||
/// Fetch a request in async way. | ||
pub async fn fetch(&self, req: Request<Buffer>) -> Result<Response<HttpBody>> { | ||
self.fetcher.fetch(req).await | ||
} | ||
} | ||
|
||
pub trait HttpFetch: Send + Sync + Unpin + 'static { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add comments to this trait. This trait is exposed to users, and we expect that users should never interact with |
||
/// Fetch a request in async way. | ||
fn fetch( | ||
&self, | ||
req: Request<Buffer>, | ||
) -> impl Future<Output = Result<Response<HttpBody>>> + MaybeSend; | ||
} | ||
|
||
/// HttpFetchDyn is the dyn version of [`HttpFetch`] | ||
/// which make it possible to use as `Box<dyn HttpFetchDyn>` | ||
pub trait HttpFetchDyn: Send + Sync + Unpin + 'static { | ||
/// The dyn version of [`HttpFetch::fetch`]. | ||
/// | ||
/// This function returns a boxed future to make it object safe. | ||
fn fetch_dyn(&self, req: Request<Buffer>) -> BoxedFuture<Result<Response<HttpBody>>>; | ||
} | ||
|
||
impl<T: HttpFetch + ?Sized> HttpFetchDyn for T { | ||
fn fetch_dyn(&self, req: Request<Buffer>) -> BoxedFuture<Result<Response<HttpBody>>> { | ||
Box::pin(self.fetch(req)) | ||
} | ||
} | ||
|
||
impl<T: HttpFetchDyn + ?Sized> HttpFetch for Box<T> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implement for |
||
async fn fetch(&self, req: Request<Buffer>) -> Result<Response<HttpBody>> { | ||
self.deref().fetch_dyn(req).await | ||
} | ||
} | ||
|
||
impl HttpFetch for reqwest::Client { | ||
async fn fetch(&self, req: Request<Buffer>) -> Result<Response<HttpBody>> { | ||
// Uri stores all string alike data in `Bytes` which means | ||
// the clone here is cheap. | ||
let uri = req.uri().clone(); | ||
|
@@ -86,7 +133,6 @@ impl HttpClient { | |
let (parts, body) = req.into_parts(); | ||
|
||
let mut req_builder = self | ||
.client | ||
.request( | ||
parts.method, | ||
reqwest::Url::from_str(&uri.to_string()).expect("input request url must be valid"), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,10 @@ | |
mod client; | ||
pub use client::HttpClient; | ||
|
||
/// temporary client used by several features | ||
#[allow(unused_imports)] | ||
pub(crate) use client::GLOBAL_REQWEST_CLIENT; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make |
||
|
||
mod body; | ||
pub use body::HttpBody; | ||
|
||
|
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.
We can use
Arc<dyn HttpFetchDyn>
here to eliminate the need for an extraBox
.