Improve manual config experience for SDK retries and timeouts (#1603)

* Remove `Default` implementation from `RetryConfig`
* Add use case integration tests
* Panic when retries/timeouts are enabled without a `sleep_impl`
* Combine the sleep, retry, and timeout customizations
* Add `sleep_impl` validation to the Smithy client builder

CHANGELOG.next.toml

@@ -9,4 +9,66 @@
 # message = "Fix typos in module documentation for generated crates"
 # references = ["smithy-rs#920"]
 # meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
-# author = "rcoh"
+# author = "rcoh"
+
+[[aws-sdk-rust]]
+message = "`aws_config::RetryConfig` no longer implements `Default`, and its `new` function has been replaced with `standard`."
+references = ["smithy-rs#1603", "aws-sdk-rust#586"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = "`aws_smithy_types::RetryConfig` no longer implements `Default`, and its `new` function has been replaced with `standard`."
+references = ["smithy-rs#1603", "aws-sdk-rust#586"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"
+
+[[aws-sdk-rust]]
+message = """
+Direct configuration of `aws_config::SdkConfig` now defaults to retries being disabled.
+If you're using `aws_config::load_from_env()` or `aws_config::from_env()` to configure
+the SDK, then you are NOT affected by this change. If you use `SdkConfig::builder()` to
+configure the SDK, then you ARE affected by this change and should set the retry config
+on that builder.
+"""
+references = ["smithy-rs#1603", "aws-sdk-rust#586"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[aws-sdk-rust]]
+message = """
+Client creation now panics if retries or timeouts are enabled without an async sleep
+implementation set on the SDK config.
+If you're using the Tokio runtime and have the `rt-tokio` feature enabled (which is enabled by default),
+then you shouldn't notice this change at all.
+Otherwise, if using something other than Tokio as the async runtime, the `AsyncSleep` trait must be implemented,
+and that implementation given to the config builder via the `sleep_impl` method. Alternatively, retry can be
+explicitly turned off by setting the retry config to `RetryConfig::disabled()`, which will result in successful
+client creation without an async sleep implementation.
+"""
+references = ["smithy-rs#1603", "aws-sdk-rust#586"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = """
+Client creation now panics if retries or timeouts are enabled without an async sleep implementation.
+If you're using the Tokio runtime and have the `rt-tokio` feature enabled (which is enabled by default),
+then you shouldn't notice this change at all.
+Otherwise, if using something other than Tokio as the async runtime, the `AsyncSleep` trait must be implemented,
+and that implementation given to the config builder via the `sleep_impl` method. Alternatively, retry can be
+explicitly turned off by setting `max_attempts` to 1, which will result in successful client creation without an
+async sleep implementation.
+"""
+references = ["smithy-rs#1603", "aws-sdk-rust#586"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"
+
+[[smithy-rs]]
+message = """
+The `default_async_sleep` method on the `Client` builder has been removed. The default async sleep is
+wired up by default if none is provided.
+"""
+references = ["smithy-rs#1603", "aws-sdk-rust#586"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" }
+author = "jdisanti"

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

@@ -399,7 +399,7 @@ mod test {
             .retry_config()
             .await;
 
-        let expected_retry_config = RetryConfig::new();
+        let expected_retry_config = RetryConfig::standard();
 
         assert_eq!(actual_retry_config, expected_retry_config);
         // This is redundant but it's really important to make sure that
@@ -420,7 +420,7 @@ mod test {
             .retry_config()
             .await;
 
-        let expected_retry_config = RetryConfig::new();
+        let expected_retry_config = RetryConfig::standard();
 
         assert_eq!(actual_retry_config, expected_retry_config)
     }
@@ -447,9 +447,7 @@ retry_mode = standard
             .retry_config()
             .await;
 
-        let expected_retry_config = RetryConfig::new()
-            .with_max_attempts(1)
-            .with_retry_mode(RetryMode::Standard);
+        let expected_retry_config = RetryConfig::standard().with_max_attempts(1);
 
         assert_eq!(actual_retry_config, expected_retry_config)
     }
@@ -480,9 +478,7 @@ retry_mode = standard
             .retry_config()
             .await;
 
-        let expected_retry_config = RetryConfig::new()
-            .with_max_attempts(42)
-            .with_retry_mode(RetryMode::Standard);
+        let expected_retry_config = RetryConfig::standard().with_max_attempts(42);
 
         assert_eq!(actual_retry_config, expected_retry_config)
     }

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

@@ -76,7 +76,7 @@ impl Builder {
     /// Attempt to create a [RetryConfig](aws_smithy_types::retry::RetryConfig) from following sources in order:
     /// 1. [Environment variables](crate::environment::retry_config::EnvironmentVariableRetryConfigProvider)
     /// 2. [Profile file](crate::profile::retry_config::ProfileFileRetryConfigProvider)
-    /// 3. [RetryConfig::default()](aws_smithy_types::retry::RetryConfig::default)
+    /// 3. [RetryConfig::standard()](aws_smithy_types::retry::RetryConfig::standard)
     ///
     /// Precedence is considered on a per-field basis
     ///

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

@@ -102,7 +102,7 @@ mod test {
                 .retry_config_builder()
                 .unwrap()
                 .build(),
-            RetryConfig::new().with_max_attempts(88)
+            RetryConfig::standard().with_max_attempts(88)
         );
     }
 
@@ -123,7 +123,7 @@ mod test {
                 .retry_config_builder()
                 .unwrap()
                 .build(),
-            RetryConfig::new().with_retry_mode(RetryMode::Standard)
+            RetryConfig::standard()
         );
     }
 
@@ -137,9 +137,7 @@ mod test {
             .retry_config_builder()
             .unwrap()
             .build(),
-            RetryConfig::new()
-                .with_max_attempts(13)
-                .with_retry_mode(RetryMode::Standard)
+            RetryConfig::standard().with_max_attempts(13)
         );
     }
 

aws/rust-runtime/aws-config/src/imds/client.rs

@@ -744,11 +744,8 @@ impl<T, E> ClassifyResponse<SdkSuccess<T>, SdkError<E>> for ImdsErrorPolicy {
 
 #[cfg(test)]
 pub(crate) mod test {
-    use std::collections::HashMap;
-    use std::error::Error;
-    use std::io;
-    use std::time::{Duration, UNIX_EPOCH};
-
+    use crate::imds::client::{Client, EndpointMode, ImdsErrorPolicy};
+    use crate::provider_config::ProviderConfig;
     use aws_smithy_async::rt::sleep::TokioSleep;
     use aws_smithy_client::erase::DynConnector;
     use aws_smithy_client::test_connection::{capture_request, TestConnection};
@@ -760,11 +757,12 @@ pub(crate) mod test {
     use http::header::USER_AGENT;
     use http::Uri;
     use serde::Deserialize;
+    use std::collections::HashMap;
+    use std::error::Error;
+    use std::io;
+    use std::time::{Duration, UNIX_EPOCH};
     use tracing_test::traced_test;
 
-    use crate::imds::client::{Client, EndpointMode, ImdsErrorPolicy};
-    use crate::provider_config::ProviderConfig;
-
     const TOKEN_A: &str = "AQAEAFTNrA4eEGx0AQgJ1arIq_Cc-t4tWt3fB0Hd8RKhXlKc5ccvhg==";
     const TOKEN_B: &str = "alternatetoken==";
 
@@ -918,6 +916,7 @@ pub(crate) mod test {
         let client = super::Client::builder()
             .configure(
                 &ProviderConfig::no_configuration()
+                    .with_sleep(TokioSleep::new())
                     .with_http_connector(DynConnector::new(connection.clone()))
                     .with_time_source(TimeSource::manual(&time_source)),
             )
@@ -1134,6 +1133,7 @@ pub(crate) mod test {
     async fn check(test_case: ImdsConfigTest) {
         let (server, watcher) = capture_request(None);
         let provider_config = ProviderConfig::no_configuration()
+            .with_sleep(TokioSleep::new())
             .with_env(Env::from(test_case.env))
             .with_fs(Fs::from_map(test_case.fs))
             .with_http_connector(DynConnector::new(server));

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

@@ -227,7 +227,7 @@ mod loader {
         /// # use aws_smithy_types::retry::RetryConfig;
         /// # async fn create_config() {
         ///     let config = aws_config::from_env()
-        ///         .retry_config(RetryConfig::new().with_max_attempts(2))
+        ///         .retry_config(RetryConfig::standard().with_max_attempts(2))
         ///         .load().await;
         /// # }
         /// ```
@@ -449,6 +449,7 @@ mod loader {
     mod test {
         use crate::from_env;
         use crate::provider_config::ProviderConfig;
+        use aws_smithy_async::rt::sleep::TokioSleep;
         use aws_smithy_client::erase::DynConnector;
         use aws_smithy_client::never::NeverConnector;
         use aws_types::credentials::ProvideCredentials;
@@ -465,6 +466,7 @@ mod loader {
             let loader = from_env()
                 .configure(
                     ProviderConfig::empty()
+                        .with_sleep(TokioSleep::new())
                         .with_env(env)
                         .with_http_connector(DynConnector::new(NeverConnector::new())),
                 )

aws/rust-runtime/aws-config/src/sts/assume_role.rs

@@ -273,6 +273,7 @@ impl ProvideCredentials for AssumeRoleProvider {
 mod test {
     use crate::provider_config::ProviderConfig;
     use crate::sts::AssumeRoleProvider;
+    use aws_smithy_async::rt::sleep::TokioSleep;
     use aws_smithy_client::erase::DynConnector;
     use aws_smithy_client::test_connection::capture_request;
     use aws_smithy_http::body::SdkBody;
@@ -286,6 +287,7 @@ mod test {
     async fn configures_session_length() {
         let (server, request) = capture_request(None);
         let provider_conf = ProviderConfig::empty()
+            .with_sleep(TokioSleep::new())
             .with_time_source(TimeSource::manual(&ManualTimeSource::new(
                 UNIX_EPOCH + Duration::from_secs(1234567890 - 120),
             )))
@@ -314,6 +316,7 @@ mod test {
         ));
         let (server, _request) = capture_request(Some(resp));
         let provider_conf = ProviderConfig::empty()
+            .with_sleep(TokioSleep::new())
             .with_time_source(TimeSource::manual(&ManualTimeSource::new(
                 UNIX_EPOCH + Duration::from_secs(1234567890 - 120),
             )))

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

@@ -259,22 +259,22 @@ async fn load_credentials(
 
 #[cfg(test)]
 mod test {
+    use crate::provider_config::ProviderConfig;
+    use crate::test_case::no_traffic_connector;
     use crate::web_identity_token::{
         Builder, ENV_VAR_ROLE_ARN, ENV_VAR_SESSION_NAME, ENV_VAR_TOKEN_FILE,
     };
-
     use aws_sdk_sts::Region;
-    use aws_types::os_shim_internal::{Env, Fs};
-
-    use crate::provider_config::ProviderConfig;
-    use crate::test_case::no_traffic_connector;
+    use aws_smithy_async::rt::sleep::TokioSleep;
     use aws_types::credentials::CredentialsError;
+    use aws_types::os_shim_internal::{Env, Fs};
     use std::collections::HashMap;
 
     #[tokio::test]
     async fn unloaded_provider() {
         // empty environment
         let conf = ProviderConfig::empty()
+            .with_sleep(TokioSleep::new())
             .with_env(Env::from_slice(&[]))
             .with_http_connector(no_traffic_connector())
             .with_region(Some(Region::from_static("us-east-1")));
@@ -297,6 +297,7 @@ mod test {
         let provider = Builder::default()
             .configure(
                 &ProviderConfig::empty()
+                    .with_sleep(TokioSleep::new())
                     .with_region(region)
                     .with_env(env)
                     .with_http_connector(no_traffic_connector()),
@@ -328,6 +329,7 @@ mod test {
         let provider = Builder::default()
             .configure(
                 &ProviderConfig::empty()
+                    .with_sleep(TokioSleep::new())
                     .with_http_connector(no_traffic_connector())
                     .with_region(Some(Region::new("us-east-1")))
                     .with_env(env)

aws/rust-runtime/aws-types/src/sdk_config.rs

@@ -35,6 +35,10 @@ pub struct SdkConfig {
 }
 
 /// Builder for AWS Shared Configuration
+///
+/// _Important:_ Using the `aws-config` crate to configure the SDK is preferred to invoking this
+/// builder directly. Using this builder directly won't pull in any AWS recommended default
+/// configuration values.
 #[derive(Debug, Default)]
 pub struct Builder {
     app_name: Option<AppName>,
@@ -127,12 +131,15 @@ impl Builder {
 
     /// Set the retry_config for the builder
     ///
+    /// _Note:_ Retries require a sleep implementation in order to work. When enabling retry, make
+    /// sure to set one with [Self::sleep_impl] or [Self::set_sleep_impl].
+    ///
     /// # Examples
     /// ```rust
     /// use aws_types::SdkConfig;
     /// use aws_smithy_types::retry::RetryConfig;
     ///
-    /// let retry_config = RetryConfig::new().with_max_attempts(5);
+    /// let retry_config = RetryConfig::standard().with_max_attempts(5);
     /// let config = SdkConfig::builder().retry_config(retry_config).build();
     /// ```
     pub fn retry_config(mut self, retry_config: RetryConfig) -> Self {
@@ -142,13 +149,16 @@ impl Builder {
 
     /// Set the retry_config for the builder
     ///
+    /// _Note:_ Retries require a sleep implementation in order to work. When enabling retry, make
+    /// sure to set one with [Self::sleep_impl] or [Self::set_sleep_impl].
+    ///
     /// # Examples
     /// ```rust
     /// use aws_types::sdk_config::{SdkConfig, Builder};
     /// use aws_smithy_types::retry::RetryConfig;
     ///
     /// fn disable_retries(builder: &mut Builder) {
-    ///     let retry_config = RetryConfig::new().with_max_attempts(1);
+    ///     let retry_config = RetryConfig::standard().with_max_attempts(1);
     ///     builder.set_retry_config(Some(retry_config));
     /// }
     ///
@@ -163,6 +173,10 @@ impl Builder {
 
     /// Set the [`timeout::Config`](aws_smithy_types::timeout::Config) for the builder
     ///
+    /// _Note:_ Timeouts require a sleep implementation in order to work.
+    /// When enabling timeouts, be sure to set one with [Self::sleep_impl] or
+    /// [Self::set_sleep_impl].
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -184,6 +198,10 @@ impl Builder {
 
     /// Set the [`timeout::Config`](aws_smithy_types::timeout::Config) for the builder
     ///
+    /// _Note:_ Timeouts require a sleep implementation in order to work.
+    /// When enabling timeouts, be sure to set one with [Self::sleep_impl] or
+    /// [Self::set_sleep_impl].
+    ///
     /// # Examples
     /// ```rust
     /// # use std::time::Duration;
@@ -211,6 +229,9 @@ impl Builder {
     /// Set the sleep implementation for the builder. The sleep implementation is used to create
     /// timeout futures.
     ///
+    /// _Note:_ If you're using the Tokio runtime, a `TokioSleep` implementation is available in
+    /// the `aws-smithy-async` crate.
+    ///
     /// # Examples
     ///
     /// ```rust
@@ -238,6 +259,9 @@ impl Builder {
     /// Set the sleep implementation for the builder. The sleep implementation is used to create
     /// timeout futures.
     ///
+    /// _Note:_ If you're using the Tokio runtime, a `TokioSleep` implementation is available in
+    /// the `aws-smithy-async` crate.
+    ///
     /// # Examples
     /// ```rust
     /// # use aws_smithy_async::rt::sleep::{AsyncSleep, Sleep};
@@ -405,6 +429,10 @@ impl SdkConfig {
     }
 
     /// Config builder
+    ///
+    /// _Important:_ Using the `aws-config` crate to configure the SDK is preferred to invoking this
+    /// builder directly. Using this builder directly won't pull in any AWS recommended default
+    /// configuration values.
     pub fn builder() -> Builder {
         Builder::default()
     }