Skip to content

Commit

Permalink
Change timeout settings to merge them when configuration is merged (#…
Browse files Browse the repository at this point in the history
…3405)

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
For context, see
#3408

## Description
<!--- Describe your changes in detail -->
- During `invoke`, load all timeout configs and merge them via a custom
loader.
- Fix config bag bugs that prevented using a Stored type that differed
from `T`.
- Add new e2e and codegen integration test validating that timeout
settings are properly merged.
- Add fallback for an empty timeout config being equivalent to
`TimeoutConfig::disabled`.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the AWS
SDK, generated SDK code, or SDK runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

---------

Co-authored-by: John DiSanti <jdisanti@amazon.com>
  • Loading branch information
rcoh and jdisanti authored Feb 13, 2024
1 parent fa61448 commit 1ea9d05
Show file tree
Hide file tree
Showing 9 changed files with 537 additions and 54 deletions.
24 changes: 21 additions & 3 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@ author = "rcoh"

[[smithy-rs]]
message = "Added impl `Display` to Enums."
references = ["smithy-rs#3336","smithy-rs#3391"]
meta = { "breaking" = false, "tada" = false, "bug" = false , "target" = "client" }
references = ["smithy-rs#3336", "smithy-rs#3391"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" }
author = "iampkmone"

[[aws-sdk-rust]]
message = "Added impl `Display` to Enums."
references = ["smithy-rs#3336", "smithy-rs#3391"]
meta = { "breaking" = false, "tada" = false, "bug" = false}
meta = { "breaking" = false, "tada" = false, "bug" = false }
author = "iampkmone"

[[aws-sdk-rust]]
Expand Down Expand Up @@ -104,3 +104,21 @@ message = "Retain the SSO token cache between calls to `provide_credentials` whe
references = ["smithy-rs#3387"]
meta = { "breaking" = false, "bug" = true, "tada" = false }
author = "jdisanti"

[[smithy-rs]]
message = """Fix bug where timeout settings where not merged properly. This will add a default connect timeout of 3.1s seconds for most clients.
[**For more details see the long-form changelog discussion**](https://github.com/smithy-lang/smithy-rs/discussions/3408)."""

references = ["smithy-rs#3405", "smithy-rs#3400", "smithy-rs#3258"]
meta = { "bug" = true, "breaking" = true, tada = false, target = "client" }
author = "rcoh"

[[aws-sdk-rust]]
message = """Fix bug where timeout settings where not merged properly. This will add a default connect timeout of 3.1s seconds for most clients.
[**For more details see the long-form changelog discussion**](https://github.com/smithy-lang/smithy-rs/discussions/3408)."""

references = ["smithy-rs#3405", "smithy-rs#3400", "smithy-rs#3258"]
meta = { "bug" = true, "breaking" = true, tada = false }
author = "rcoh"
22 changes: 14 additions & 8 deletions aws/rust-runtime/aws-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,12 @@ mod loader {

/// Override the timeout config used to build [`SdkConfig`].
///
/// This will be merged with timeouts coming from the timeout information provider, which
/// currently includes a default `CONNECT` timeout of `3.1s`.
///
/// If you want to disable timeouts, use [`TimeoutConfig::disabled`]. If you want to disable
/// a specific timeout, use `TimeoutConfig::set_<type>(None)`.
///
/// **Note: This only sets timeouts for calls to AWS services.** Timeouts for the credentials
/// provider chain are configured separately.
///
Expand Down Expand Up @@ -728,14 +734,14 @@ mod loader {
.await
};

let timeout_config = if let Some(timeout_config) = self.timeout_config {
timeout_config
} else {
timeout_config::default_provider()
.configure(&conf)
.timeout_config()
.await
};
let base_config = timeout_config::default_provider()
.configure(&conf)
.timeout_config()
.await;
let mut timeout_config = self
.timeout_config
.unwrap_or_else(|| TimeoutConfig::builder().build());
timeout_config.take_defaults_from(&base_config);

let credentials_provider = match self.credentials_provider {
CredentialsProviderOption::Set(provider) => Some(provider),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ fun awsSdkIntegrationTest(

fun awsIntegrationTestParams() =
IntegrationTestParams(
cargoCommand = "cargo test --features test-util behavior-version-latest",
cargoCommand = "cargo test --features test-util,behavior-version-latest --tests --lib",
runtimeConfig = AwsTestRuntimeConfig,
additionalSettings =
ObjectNode.builder().withMember(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.rustsdk

import SdkCodegenIntegrationTest
import org.junit.jupiter.api.Test
import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.testutil.integrationTest

class TimeoutConfigMergingTest {
@Test
fun testTimeoutSettingsProperlyMerged() {
awsSdkIntegrationTest(SdkCodegenIntegrationTest.model) { ctx, crate ->
val name = ctx.moduleUseName()
crate.integrationTest("timeout_settings_properly_merged") {
rustTemplate(
"""
use $name::Client;
use aws_smithy_runtime::test_util::capture_test_logs::capture_test_logs;
use aws_smithy_runtime_api::box_error::BoxError;
use aws_smithy_runtime_api::client::interceptors::context::BeforeTransmitInterceptorContextRef;
use aws_smithy_runtime_api::client::interceptors::Intercept;
use aws_smithy_runtime_api::client::runtime_components::RuntimeComponents;
use aws_smithy_runtime::client::http::test_util::infallible_client_fn;
use aws_smithy_types::config_bag::ConfigBag;
use aws_smithy_types::timeout::TimeoutConfig;
use aws_smithy_types::body::SdkBody;
use aws_types::SdkConfig;
use std::sync::Arc;
use std::sync::Mutex;
use std::time::Duration;
##[derive(Debug, Clone)]
struct CaptureConfigInterceptor {
timeout_config: Arc<Mutex<Option<TimeoutConfig>>>,
}
impl Intercept for CaptureConfigInterceptor {
fn name(&self) -> &'static str {
"capture config interceptor"
}
fn read_before_attempt(
&self,
_context: &BeforeTransmitInterceptorContextRef<'_>,
_runtime_components: &RuntimeComponents,
cfg: &mut ConfigBag,
) -> Result<(), BoxError> {
*self.timeout_config.lock().unwrap() = cfg.load::<TimeoutConfig>().cloned();
Ok(())
}
}
#{tokio_test}
async fn test_all_timeouts() {
let (_logs, _guard) = capture_test_logs();
let connect_timeout = Duration::from_secs(1);
let read_timeout = Duration::from_secs(2);
let operation_attempt = Duration::from_secs(3);
let operation = Duration::from_secs(4);
let http_client = infallible_client_fn(|_req| http::Response::builder().body(SdkBody::empty()).unwrap());
let sdk_config = SdkConfig::builder()
.timeout_config(
TimeoutConfig::builder()
.connect_timeout(connect_timeout)
.build(),
)
.http_client(http_client)
.build();
let client_config = $name::config::Builder::from(&sdk_config)
.timeout_config(TimeoutConfig::builder().read_timeout(read_timeout).build())
.build();
let client = Client::from_conf(client_config);
let interceptor = CaptureConfigInterceptor {
timeout_config: Default::default(),
};
let _err = client
.some_operation()
.customize()
.config_override(
$name::Config::builder().timeout_config(
TimeoutConfig::builder()
.operation_attempt_timeout(operation_attempt)
.operation_timeout(operation)
.build(),
),
)
.interceptor(interceptor.clone())
.send()
.await;
let _ = dbg!(_err);
assert_eq!(
interceptor
.timeout_config
.lock()
.unwrap()
.as_ref()
.expect("timeout config not set"),
&TimeoutConfig::builder()
.operation_timeout(operation)
.operation_attempt_timeout(operation_attempt)
.read_timeout(read_timeout)
.connect_timeout(connect_timeout)
.build(),
"full set of timeouts set from all three sources."
);
// disable timeouts
let _err = client
.some_operation()
.customize()
.config_override(
$name::Config::builder().timeout_config(
TimeoutConfig::disabled(),
),
)
.interceptor(interceptor.clone())
.send()
.await;
let _ = dbg!(_err);
assert_eq!(
interceptor
.timeout_config
.lock()
.unwrap()
.as_ref()
.expect("timeout config not set"),
&TimeoutConfig::disabled(),
"timeouts disabled by config override"
);
// override one field
let _err = client
.some_operation()
.customize()
.config_override(
$name::Config::builder().timeout_config(
TimeoutConfig::builder().read_timeout(Duration::from_secs(10)).build(),
),
)
.interceptor(interceptor.clone())
.send()
.await;
let _ = dbg!(_err);
assert_eq!(
interceptor
.timeout_config
.lock()
.unwrap()
.as_ref()
.expect("timeout config not set"),
&TimeoutConfig::builder()
.read_timeout(Duration::from_secs(10))
.connect_timeout(connect_timeout)
.disable_operation_attempt_timeout()
.disable_operation_timeout()
.build(),
"read timeout overridden"
);
}
""",
"tokio_test" to writable { Attribute.TokioTest.render(this) },
)
}
}
}
}
62 changes: 62 additions & 0 deletions aws/sdk/integration-tests/s3/tests/service_timeout_overrides.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@

use aws_credential_types::provider::SharedCredentialsProvider;
use aws_credential_types::Credentials;
use aws_smithy_async::assert_elapsed;
use aws_smithy_async::rt::sleep::{SharedAsyncSleep, TokioSleep};
use aws_smithy_runtime::client::http::test_util::NeverClient;
use aws_smithy_runtime::test_util::capture_test_logs::capture_test_logs;
use aws_smithy_runtime_api::client::behavior_version::BehaviorVersion;
use aws_smithy_runtime_api::client::result::SdkError;
use aws_smithy_types::retry::RetryConfig;
use aws_smithy_types::timeout::TimeoutConfig;
use aws_types::region::Region;
use aws_types::SdkConfig;
Expand All @@ -23,9 +26,11 @@ async fn timeouts_can_be_set_by_service() {
.credentials_provider(SharedCredentialsProvider::new(Credentials::for_tests()))
.region(Region::from_static("us-east-1"))
.sleep_impl(SharedAsyncSleep::new(TokioSleep::new()))
.retry_config(RetryConfig::disabled())
.timeout_config(
TimeoutConfig::builder()
.operation_timeout(Duration::from_secs(5))
.read_timeout(Duration::from_secs(1))
.build(),
)
.http_client(NeverClient::new())
Expand All @@ -42,6 +47,7 @@ async fn timeouts_can_be_set_by_service() {
.build(),
)
.build();

let client = aws_sdk_s3::Client::from_conf(config);
let start = Instant::now();
let err = client
Expand All @@ -60,3 +66,59 @@ async fn timeouts_can_be_set_by_service() {
// it's shorter than the 5 second timeout if the test is broken
assert!(start.elapsed() < Duration::from_millis(500));
}

/// Ensures that a default timeout from aws-config is still persisted even if an operation_timeout
/// is set.
#[tokio::test]
async fn default_connect_timeout_set() {
let (_guard, _) = capture_test_logs();
let sdk_config = aws_config::defaults(BehaviorVersion::latest())
.test_credentials()
.region(Region::from_static("us-east-1"))
.timeout_config(
TimeoutConfig::builder()
.operation_timeout(Duration::from_secs(5))
.build(),
)
.retry_config(RetryConfig::disabled())
// ip that
.endpoint_url(
// Emulate a connect timeout error by hitting an unroutable IP
"http://172.255.255.0:18104",
)
.load()
.await;
assert_eq!(
sdk_config.timeout_config(),
Some(
&TimeoutConfig::builder()
.connect_timeout(Duration::from_millis(3100))
.operation_timeout(Duration::from_secs(5))
.build()
)
);
let config = aws_sdk_s3::config::Builder::from(&sdk_config)
.timeout_config(
TimeoutConfig::builder()
.operation_attempt_timeout(Duration::from_secs(4))
.build(),
)
.build();

let client = aws_sdk_s3::Client::from_conf(config);
let start = Instant::now();
let err = client
.get_object()
.key("foo")
.bucket("bar")
.send()
.await
.expect_err("unroutable IP should timeout");
assert!(
matches!(err, SdkError::DispatchFailure { .. }),
"expected DispatchFailure got {}",
err
);
// ensure that of the three timeouts, the one we hit is connect timeout.
assert_elapsed!(start, Duration::from_millis(3100));
}
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,11 @@ class ResiliencyConfigCustomization(codegenContext: ClientCodegenContext) : Conf
self
}
/// Set the timeout_config for the builder
/// Set the timeout_config for the builder.
///
/// Setting this to `None` has no effect if another source of configuration has set timeouts. If you
/// are attempting to disable timeouts, use [`TimeoutConfig::disabled`](#{TimeoutConfig}::disabled)
///
///
/// ## Examples
///
Expand All @@ -237,10 +241,22 @@ class ResiliencyConfigCustomization(codegenContext: ClientCodegenContext) : Conf
*codegenScope,
)

// A timeout config can be set from SdkConfig. We want to merge that with a timeout config set here.
// Ideally, we would actually preserve `SdkConfig` as a separate layer (probably by converting it into
// its own runtime plugin). In the short term, this functionality accomplishes that for
// timeout configs.
rustTemplate(
"""
pub fn set_timeout_config(&mut self, timeout_config: #{Option}<#{TimeoutConfig}>) -> &mut Self {
timeout_config.map(|t| self.config.store_put(t));
// passing None has no impact.
let Some(mut timeout_config) = timeout_config else {
return self
};
if let Some(base) = self.config.load::<#{TimeoutConfig}>() {
timeout_config.take_defaults_from(base);
}
self.config.store_put(timeout_config);
self
}
""",
Expand Down
Loading

0 comments on commit 1ea9d05

Please sign in to comment.