Skip to content

Commit

Permalink
Default connection and request timeouts of 5 seconds (#4928)
Browse files Browse the repository at this point in the history
* Default connection and request timeouts of 5 seconds

* Clippy

* Allow disabling timeouts
  • Loading branch information
tustvold authored Oct 16, 2023
1 parent 57cd094 commit 31bc84c
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 6 deletions.
3 changes: 1 addition & 2 deletions object_store/src/aws/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1130,8 +1130,7 @@ impl AmazonS3Builder {

Arc::new(TokenCredentialProvider::new(
token,
// The instance metadata endpoint is access over HTTP
self.client_options.clone().with_allow_http(true).client()?,
self.client_options.metadata_client()?,
self.retry_config.clone(),
)) as _
};
Expand Down
2 changes: 1 addition & 1 deletion object_store/src/azure/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,7 @@ impl MicrosoftAzureBuilder {
);
Arc::new(TokenCredentialProvider::new(
msi_credential,
self.client_options.clone().with_allow_http(true).client()?,
self.client_options.metadata_client()?,
self.retry_config.clone(),
)) as _
};
Expand Down
66 changes: 64 additions & 2 deletions object_store/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ impl FromStr for ClientConfigKey {
}

/// HTTP client configuration for remote object stores
#[derive(Debug, Clone, Default)]
#[derive(Debug, Clone)]
pub struct ClientOptions {
user_agent: Option<ConfigValue<HeaderValue>>,
content_type_map: HashMap<String, String>,
Expand All @@ -188,6 +188,35 @@ pub struct ClientOptions {
http2_only: ConfigValue<bool>,
}

impl Default for ClientOptions {
fn default() -> Self {
// Defaults based on
// <https://docs.aws.amazon.com/sdkref/latest/guide/feature-smart-config-defaults.html>
// <https://docs.aws.amazon.com/whitepapers/latest/s3-optimizing-performance-best-practices/timeouts-and-retries-for-latency-sensitive-applications.html>
// Which recommend a connection timeout of 3.1s and a request timeout of 2s
Self {
user_agent: None,
content_type_map: Default::default(),
default_content_type: None,
default_headers: None,
proxy_url: None,
proxy_ca_certificate: None,
proxy_excludes: None,
allow_http: Default::default(),
allow_insecure: Default::default(),
timeout: Some(Duration::from_secs(5).into()),
connect_timeout: Some(Duration::from_secs(5).into()),
pool_idle_timeout: None,
pool_max_idle_per_host: None,
http2_keep_alive_interval: None,
http2_keep_alive_timeout: None,
http2_keep_alive_while_idle: Default::default(),
http1_only: Default::default(),
http2_only: Default::default(),
}
}
}

impl ClientOptions {
/// Create a new [`ClientOptions`] with default values
pub fn new() -> Self {
Expand Down Expand Up @@ -367,17 +396,37 @@ impl ClientOptions {
///
/// The timeout is applied from when the request starts connecting until the
/// response body has finished
///
/// Default is 5 seconds
pub fn with_timeout(mut self, timeout: Duration) -> Self {
self.timeout = Some(ConfigValue::Parsed(timeout));
self
}

/// Disables the request timeout
///
/// See [`Self::with_timeout`]
pub fn with_timeout_disabled(mut self) -> Self {
self.timeout = None;
self
}

/// Set a timeout for only the connect phase of a Client
///
/// Default is 5 seconds
pub fn with_connect_timeout(mut self, timeout: Duration) -> Self {
self.connect_timeout = Some(ConfigValue::Parsed(timeout));
self
}

/// Disables the connection timeout
///
/// See [`Self::with_connect_timeout`]
pub fn with_connect_timeout_disabled(mut self) -> Self {
self.timeout = None;
self
}

/// Set the pool max idle timeout
///
/// This is the length of time an idle connection will be kept alive
Expand Down Expand Up @@ -444,7 +493,20 @@ impl ClientOptions {
}
}

pub(crate) fn client(&self) -> super::Result<Client> {
/// Create a [`Client`] with overrides optimised for metadata endpoint access
///
/// In particular:
/// * Allows HTTP as metadata endpoints do not use TLS
/// * Configures a low connection timeout to provide quick feedback if not present
#[cfg(any(feature = "aws", feature = "gcp", feature = "azure"))]
pub(crate) fn metadata_client(&self) -> Result<Client> {
self.clone()
.with_allow_http(true)
.with_connect_timeout(Duration::from_secs(1))
.client()
}

pub(crate) fn client(&self) -> Result<Client> {
let mut builder = ClientBuilder::new();

match &self.user_agent {
Expand Down
2 changes: 1 addition & 1 deletion object_store/src/gcp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,7 @@ impl GoogleCloudStorageBuilder {
} else {
Arc::new(TokenCredentialProvider::new(
InstanceCredentialProvider::new(audience),
self.client_options.clone().with_allow_http(true).client()?,
self.client_options.metadata_client()?,
self.retry_config.clone(),
)) as _
};
Expand Down

0 comments on commit 31bc84c

Please sign in to comment.