Skip to content
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

Default connection and request timeouts of 5 seconds #4928

Merged
merged 3 commits into from
Oct 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions object_store/src/aws/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1117,8 +1117,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 @@ -1074,7 +1074,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()),
Copy link
Contributor

@crepererum crepererum Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are with_{connection_}timeout methods that accepts Duration but not None, maybe add two new methods with_infinite_{connection_}timeout and with clear doc warnings? Otherwise there is no public way to set these options to None. If you NEVER want anyone to set this to None, then this shouldn't be an Option.

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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will help new users who may use the crate without configuring any credentials, which will then default to the metadata credentials, which will then hang indefinitely trying to get credentials from a non-existent metadata endpoint

.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 @@ -1080,7 +1080,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
Loading