Unverified Commit 1ea9d055 authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Change timeout settings to merge them when configuration is merged (#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
https://github.com/smithy-lang/smithy-rs/discussions/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: default avatarJohn DiSanti <jdisanti@amazon.com>
parent fa61448b

CHANGELOG.next.toml

+21 −3
Original line number Diff line number Diff line
@@ -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"

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

+14 −8
Original line number Diff line number Diff line
@@ -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.

        ///
@@ -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()

            let base_config = timeout_config::default_provider()

                .configure(&conf)

                .timeout_config()

                    .await

            };

                .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),

aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/TestUtil.kt

+1 −1
Original line number Diff line number Diff line
@@ -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(

aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/TimeoutConfigMergingTest.kt

0 → 100644
+172 −0
Original line number Diff line number Diff line 
/*

 * 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) },

                )

            }

        }

    }

}

aws/sdk/integration-tests/s3/tests/service_timeout_overrides.rs

+62 −0
Original line number Diff line number Diff line
@@ -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;
@@ -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())
@@ -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
@@ -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));

}