Improve timeout config ergonomics and add SDK default timeouts (#1740)

* Overhaul timeout configuration
* Improve resiliency re-exports
* Refactor Smithy client retry config handling in builder
* Remove non-standard timeout env vars and profile keys
* Break operation customization codegen into its own file
* Move operation customization into a submodule
* Improve operation customization re-exports
* Add integration tests for read and connect timeouts
* Fix connector timeout config in the Smithy `Client` builder
* Rename `HttpSettings` to `ConnectorSettings`

CHANGELOG.next.toml

@@ -21,3 +21,35 @@ message = "Implement support for pure Python request middleware. Improve idiomat
 references = ["smithy-rs#1734"]
 meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server" }
 author = "crisidev"
+
+[[aws-sdk-rust]]
+message = """
+The SDK, by default, now times out if socket connect or time to first byte read takes longer than
+3.1 seconds. There are a large number of breaking changes that come with this change that may
+affect you if you customize the client configuration at all.
+See [the upgrade guide](https://github.com/awslabs/aws-sdk-rust/issues/622) for information
+on how to configure timeouts, and how to resolve compilation issues after upgrading.
+"""
+references = ["smithy-rs#1740", "smithy-rs#256"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[aws-sdk-rust]]
+message = """
+Setting connect/read timeouts with `SdkConfig` now works. Previously, these timeout config values
+were lost during connector creation, so the only reliable way to set them was to manually override
+the HTTP connector.
+"""
+references = ["smithy-rs#1740", "smithy-rs#256"]
+meta = { "breaking" = false, "tada" = false, "bug" = true }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = """
+A large list of breaking changes were made to accomodate default timeouts in the AWS SDK.
+See [the smithy-rs upgrade guide](https://github.com/awslabs/smithy-rs/issues/1760) for a full list
+of breaking changes and how to resolve them.
+"""
+references = ["smithy-rs#1740", "smithy-rs#256"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"

aws/rust-runtime/aws-config/external-types.toml

@@ -8,12 +8,13 @@ allowed_external_types = [
    "aws_smithy_client::erase::DynConnector",
    "aws_smithy_client::erase::boxclone::BoxCloneService",
    "aws_smithy_client::http_connector::HttpConnector",
-   "aws_smithy_client::http_connector::HttpSettings",
+   "aws_smithy_client::http_connector::ConnectorSettings",
    "aws_smithy_http::body::SdkBody",
    "aws_smithy_http::result::SdkError",
-   "aws_smithy_types::retry::RetryConfig*",
+   "aws_smithy_types::retry",
+   "aws_smithy_types::retry::*",
    "aws_smithy_types::timeout",
-   "aws_smithy_types::timeout::config::Config",
+   "aws_smithy_types::timeout::config::TimeoutConfig",
    "aws_smithy_types::timeout::error::ConfigError",
    "aws_types::*",
    "http::response::Response",

aws/rust-runtime/aws-config/src/connector.rs

@@ -5,11 +5,10 @@
 
 //! Functionality related to creating new HTTP Connectors
 
-use std::sync::Arc;
-
 use aws_smithy_async::rt::sleep::AsyncSleep;
 use aws_smithy_client::erase::DynConnector;
-use aws_smithy_client::http_connector::HttpSettings;
+use aws_smithy_client::http_connector::ConnectorSettings;
+use std::sync::Arc;
 
 // unused when all crate features are disabled
 /// Unwrap an [`Option<DynConnector>`](aws_smithy_client::erase::DynConnector), and panic with a helpful error message if it's `None`
@@ -19,41 +18,41 @@ pub(crate) fn expect_connector(connector: Option<DynConnector>) -> DynConnector
 
 #[cfg(any(feature = "rustls", feature = "native-tls"))]
 fn base(
-    settings: &HttpSettings,
+    settings: &ConnectorSettings,
     sleep: Option<Arc<dyn AsyncSleep>>,
 ) -> aws_smithy_client::hyper_ext::Builder {
     let mut hyper =
-        aws_smithy_client::hyper_ext::Adapter::builder().timeout(&settings.http_timeout_config);
+        aws_smithy_client::hyper_ext::Adapter::builder().connector_settings(settings.clone());
     if let Some(sleep) = sleep {
         hyper = hyper.sleep_impl(sleep);
     }
     hyper
 }
 
-/// Given `HttpSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.
+/// Given `ConnectorSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.
 #[cfg(feature = "rustls")]
 pub fn default_connector(
-    settings: &HttpSettings,
+    settings: &ConnectorSettings,
     sleep: Option<Arc<dyn AsyncSleep>>,
 ) -> Option<DynConnector> {
     let hyper = base(settings, sleep).build(aws_smithy_client::conns::https());
     Some(DynConnector::new(hyper))
 }
 
-/// Given `HttpSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.
+/// Given `ConnectorSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.
 #[cfg(all(not(feature = "rustls"), feature = "native-tls"))]
 pub fn default_connector(
-    settings: &HttpSettings,
+    settings: &ConnectorSettings,
     sleep: Option<Arc<dyn AsyncSleep>>,
 ) -> Option<DynConnector> {
     let hyper = base(settings, sleep).build(aws_smithy_client::conns::native_tls());
     Some(DynConnector::new(hyper))
 }
 
-/// Given `HttpSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.
+/// Given `ConnectorSettings` and an `AsyncSleep`, create a `DynConnector` from defaults depending on what cargo features are activated.
 #[cfg(not(any(feature = "rustls", feature = "native-tls")))]
 pub fn default_connector(
-    _settings: &HttpSettings,
+    _settings: &ConnectorSettings,
     _sleep: Option<Arc<dyn AsyncSleep>>,
 ) -> Option<DynConnector> {
     None

aws/rust-runtime/aws-config/src/default_provider/timeout_config.rs

@@ -3,95 +3,39 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-use aws_smithy_types::timeout;
-
-use crate::environment::timeout_config::EnvironmentVariableTimeoutConfigProvider;
-use crate::profile;
 use crate::provider_config::ProviderConfig;
+use aws_sdk_sso::config::timeout::TimeoutConfig;
+use std::time::Duration;
+
+const SDK_DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_millis(3100);
 
-/// Default [`timeout::Config`] Provider chain
+/// Default [`TimeoutConfig`] provider chain
 ///
-/// Unlike other credentials and region, [`timeout::Config`] has no related `TimeoutConfigProvider` trait. Instead,
+/// Unlike other credentials and region, [`TimeoutConfig`] has no related `TimeoutConfigProvider` trait. Instead,
 /// a builder struct is returned which has a similar API.
 ///
-/// This provider will check the following sources in order:
-/// 1. [Environment variables](EnvironmentVariableTimeoutConfigProvider)
-/// 2. [Profile file](crate::profile::timeout_config::ProfileFileTimeoutConfigProvider) (`~/.aws/config`)
-///
-/// # Example
-///
-/// ```no_run
-/// # use std::error::Error;
-/// # #[tokio::main]
-/// # async fn main() {
-/// use aws_config::default_provider::timeout_config;
-///
-/// // Load a timeout config from a specific profile
-/// let timeout_config = timeout_config::default_provider()
-///     .profile_name("other_profile")
-///     .timeout_config()
-///     .await;
-/// let config = aws_config::from_env()
-///     // Override the timeout config set by the default profile
-///     .timeout_config(timeout_config)
-///     .load()
-///     .await;
-/// // instantiate a service client:
-/// // <my_aws_service>::Client::new(&config);
-/// # }
-/// ```
 pub fn default_provider() -> Builder {
     Builder::default()
 }
 
-/// Builder for [`timeout::Config`](aws_smithy_types::timeout::Config) that checks the environment variables and AWS profile files for configuration
+/// Builder for [`TimeoutConfig`] that resolves the default timeout configuration
+#[non_exhaustive]
 #[derive(Debug, Default)]
-pub struct Builder {
-    env_provider: EnvironmentVariableTimeoutConfigProvider,
-    profile_file: profile::timeout_config::Builder,
-}
+pub struct Builder;
 
 impl Builder {
     /// Configure the default chain
     ///
     /// Exposed for overriding the environment when unit-testing providers
-    pub fn configure(mut self, configuration: &ProviderConfig) -> Self {
-        self.env_provider =
-            EnvironmentVariableTimeoutConfigProvider::new_with_env(configuration.env());
-        self.profile_file = self.profile_file.configure(configuration);
+    pub fn configure(self, _configuration: &ProviderConfig) -> Self {
         self
     }
 
-    /// Override the profile name used by this provider
-    pub fn profile_name(mut self, name: &str) -> Self {
-        self.profile_file = self.profile_file.profile_name(name);
-        self
-    }
-
-    /// Attempt to create a [`timeout::Config`](aws_smithy_types::timeout::Config) from following sources in order:
-    /// 1. [Environment variables](crate::environment::timeout_config::EnvironmentVariableTimeoutConfigProvider)
-    /// 2. [Profile file](crate::profile::timeout_config::ProfileFileTimeoutConfigProvider)
-    ///
-    /// Precedence is considered on a per-field basis. If no timeout is specified, requests will never time out.
-    ///
-    /// # Panics
-    ///
-    /// This will panic if:
-    /// - a timeout is set to `NaN`, a negative number, or infinity
-    /// - a timeout can't be parsed as a floating point number
-    pub async fn timeout_config(self) -> timeout::Config {
-        // Both of these can return errors due to invalid config settings and we want to surface those as early as possible
-        // hence, we'll panic if any config values are invalid (missing values are OK though)
-        // We match this instead of unwrapping so we can print the error with the `Display` impl instead of the `Debug` impl that unwrap uses
-        let builder_from_env = match self.env_provider.timeout_config() {
-            Ok(timeout_config_builder) => timeout_config_builder,
-            Err(err) => panic!("{}", err),
-        };
-        let builder_from_profile = match self.profile_file.build().timeout_config().await {
-            Ok(timeout_config_builder) => timeout_config_builder,
-            Err(err) => panic!("{}", err),
-        };
-
-        builder_from_env.take_unset_from(builder_from_profile)
+    /// Resolve default timeout configuration
+    pub async fn timeout_config(self) -> TimeoutConfig {
+        // TODO(https://github.com/awslabs/smithy-rs/issues/1732): Implement complete timeout defaults specification
+        TimeoutConfig::builder()
+            .connect_timeout(SDK_DEFAULT_CONNECT_TIMEOUT)
+            .build()
     }
 }

aws/rust-runtime/aws-config/src/ecs.rs

@@ -61,11 +61,15 @@ use tower::{Service, ServiceExt};
 
 use crate::http_credential_provider::HttpCredentialProvider;
 use crate::provider_config::ProviderConfig;
+use aws_smithy_client::http_connector::ConnectorSettings;
 use aws_types::os_shim_internal::Env;
 use http::header::InvalidHeaderValue;
 use std::time::Duration;
 use tokio::sync::OnceCell;
 
+const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(5);
+const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_secs(2);
+
 // URL from https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-metadata-endpoint-v2.html
 const BASE_HOST: &str = "http://169.254.170.2";
 const ENV_RELATIVE_URI: &str = "AWS_CONTAINER_CREDENTIALS_RELATIVE_URI";
@@ -164,8 +168,12 @@ impl Provider {
         };
         let http_provider = HttpCredentialProvider::builder()
             .configure(&provider_config)
-            .connect_timeout(builder.connect_timeout)
-            .read_timeout(builder.read_timeout)
+            .connector_settings(
+                ConnectorSettings::builder()
+                    .connect_timeout(DEFAULT_CONNECT_TIMEOUT)
+                    .read_timeout(DEFAULT_READ_TIMEOUT)
+                    .build(),
+            )
             .build("EcsContainer", uri);
         Provider::Configured(http_provider)
     }

aws/rust-runtime/aws-config/src/environment/mod.rs

@@ -18,7 +18,3 @@ pub use region::EnvironmentVariableRegionProvider;
 /// Load retry behavior configuration from the environment
 pub mod retry_config;
 pub use retry_config::EnvironmentVariableRetryConfigProvider;
-
-/// Load timeout configuration from the environment
-pub mod timeout_config;
-pub use timeout_config::EnvironmentVariableTimeoutConfigProvider;