Skip to content

Commit

Permalink
chore(volo-http): do not put meta config in service (#484)
Browse files Browse the repository at this point in the history
Putting request related config in `MetaServiceConfig` will be
difficult to maintain, so in this commit we put them in context
and when client creates a request, it will clone the config into
context.

In addition, use `Option<Duration>` for timeout interfaces is
unnecessary, but it will introduce many breaking changes. This commit
also revert those changes.

Signed-off-by: Yu Li <[email protected]>
  • Loading branch information
yukiiiteru authored Aug 14, 2024
1 parent fe68dcd commit b7b1884
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 42 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion volo-http/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "volo-http"
version = "0.2.12"
version = "0.2.13"
edition.workspace = true
homepage.workspace = true
repository.workspace = true
Expand Down
27 changes: 8 additions & 19 deletions volo-http/src/client/meta.rs
Original file line number Diff line number Diff line change
@@ -1,31 +1,24 @@
use std::{error::Error, time::Duration};
use std::error::Error;

use http_body::Body;
use motore::service::Service;
use volo::context::Context;

use crate::{
context::ClientContext,
error::client::{status_error, timeout, ClientError},
error::client::{status_error, ClientError},
request::ClientRequest,
response::ClientResponse,
};

#[derive(Clone)]
pub struct MetaService<S> {
inner: S,
config: MetaServiceConfig,
}

#[derive(Clone)]
pub(super) struct MetaServiceConfig {
pub default_timeout: Option<Duration>,
pub fail_on_error_status: bool,
}

impl<S> MetaService<S> {
pub(super) fn new(inner: S, config: MetaServiceConfig) -> Self {
Self { inner, config }
pub(super) fn new(inner: S) -> Self {
Self { inner }
}
}

Expand All @@ -47,27 +40,23 @@ where
cx: &mut ClientContext,
req: ClientRequest<B>,
) -> Result<Self::Response, Self::Error> {
let request_timeout = cx
.rpc_info()
.config()
.timeout
.or(self.config.default_timeout);
let config = cx.rpc_info().config().to_owned();
let fut = self.inner.call(cx, req);
let res = match request_timeout {
let res = match config.timeout {
Some(duration) => {
let sleep = tokio::time::sleep(duration);
tokio::select! {
res = fut => res,
_ = sleep => {
tracing::error!("[Volo-HTTP]: request timeout.");
return Err(timeout());
return Err(crate::error::client::timeout());
}
}
}
None => fut.await,
};

if !self.config.fail_on_error_status {
if !config.fail_on_error_status {
return res;
}

Expand Down
44 changes: 23 additions & 21 deletions volo-http/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use self::{
callopt::CallOpt,
dns::parse_target,
loadbalance::{DefaultLB, DefaultLBService, LbConfig},
meta::{MetaService, MetaServiceConfig},
meta::MetaService,
target::TargetParser,
transport::{ClientConfig, ClientTransport, ClientTransportConfig},
};
Expand Down Expand Up @@ -94,11 +94,11 @@ pub struct ClientBuilder<IL, OL, C, LB> {
/// This is unstable now and may be changed in the future.
#[doc(hidden)]
pub struct BuilderConfig {
timeout: Option<Duration>,
stat_enable: bool,
fail_on_error_status: bool,
pub timeout: Option<Duration>,
pub stat_enable: bool,
pub fail_on_error_status: bool,
#[cfg(feature = "__tls")]
disable_tls: bool,
pub disable_tls: bool,
}

impl Default for BuilderConfig {
Expand Down Expand Up @@ -605,29 +605,29 @@ impl<IL, OL, C, LB> ClientBuilder<IL, OL, C, LB> {
}

/// Set the maximum idle time for a connection.
pub fn set_connect_timeout(&mut self, timeout: Option<Duration>) -> &mut Self {
self.connector.set_connect_timeout(timeout);
pub fn set_connect_timeout(&mut self, timeout: Duration) -> &mut Self {
self.connector.set_connect_timeout(Some(timeout));
self
}

/// Set the maximum idle time for reading data from the connection.
pub fn set_read_timeout(&mut self, timeout: Option<Duration>) -> &mut Self {
self.connector.set_read_timeout(timeout);
pub fn set_read_timeout(&mut self, timeout: Duration) -> &mut Self {
self.connector.set_read_timeout(Some(timeout));
self
}

/// Set the maximum idle time for writing data to the connection.
pub fn set_write_timeout(&mut self, timeout: Option<Duration>) -> &mut Self {
self.connector.set_write_timeout(timeout);
pub fn set_write_timeout(&mut self, timeout: Duration) -> &mut Self {
self.connector.set_write_timeout(Some(timeout));
self
}

/// Set the maximin idle time for the request.
///
/// The whole request includes connecting, writting, and reading the whole HTTP protocol
/// headers (without reading response body).
pub fn set_request_timeout(&mut self, timeout: Option<Duration>) -> &mut Self {
self.builder_config.timeout = timeout;
pub fn set_request_timeout(&mut self, timeout: Duration) -> &mut Self {
self.builder_config.timeout = Some(timeout);
self
}

Expand All @@ -648,18 +648,14 @@ impl<IL, OL, C, LB> ClientBuilder<IL, OL, C, LB> {
#[cfg(feature = "__tls")]
disable_tls: self.builder_config.disable_tls,
};
let meta_config = MetaServiceConfig {
default_timeout: self.builder_config.timeout,
fail_on_error_status: self.builder_config.fail_on_error_status,
};
let transport = ClientTransport::new(
self.http_config,
transport_config,
self.connector,
#[cfg(feature = "__tls")]
self.tls_config.unwrap_or_default(),
);
let meta_service = MetaService::new(transport, meta_config);
let meta_service = MetaService::new(transport);
let service = self.outer_layer.layer(
self.mk_lb
.make()
Expand All @@ -671,18 +667,22 @@ impl<IL, OL, C, LB> ClientBuilder<IL, OL, C, LB> {
} else {
self.caller_name
};

if !caller_name.is_empty() && self.headers.get(header::USER_AGENT).is_none() {
self.headers.insert(
header::USER_AGENT,
HeaderValue::from_str(caller_name.as_str()).expect("Invalid caller name"),
);
}
let config = Config {
timeout: self.builder_config.timeout,
fail_on_error_status: self.builder_config.fail_on_error_status,
};

let client_inner = ClientInner {
caller_name,
callee_name: self.callee_name,
default_target: self.target,
default_config: config,
default_call_opt: self.call_opt,
target_parser: self.target_parser,
headers: self.headers,
Expand All @@ -699,6 +699,7 @@ struct ClientInner {
caller_name: FastStr,
callee_name: FastStr,
default_target: Target,
default_config: Config,
default_call_opt: Option<CallOpt>,
target_parser: TargetParser,
headers: HeaderMap,
Expand Down Expand Up @@ -876,8 +877,9 @@ impl<S> Client<S> {
cx.rpc_info_mut().callee_mut().set_service_name(callee_name);
(self.inner.target_parser)(target, call_opt, cx.rpc_info_mut().callee_mut());

let config = Config { timeout };
cx.rpc_info_mut().set_config(config);
let config = cx.rpc_info_mut().config_mut();
config.clone_from(&self.inner.default_config);
config.timeout = timeout.or(config.timeout);

self.call(&mut cx, request).await
}
Expand Down
2 changes: 2 additions & 0 deletions volo-http/src/context/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ impl ClientStats {
pub struct Config {
/// Timeout of the whole request
pub timeout: Option<Duration>,
/// Return `Err` if status code of response is 4XX or 5XX
pub fail_on_error_status: bool,
}

impl Reusable for Config {
Expand Down

0 comments on commit b7b1884

Please sign in to comment.