Unverified Commit 4073ebab authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Fix several bugs associated with retry/sleep (#940) 

* Fix several bugs associated with retry/sleep

The sleep implementation was late-bound but early loaded in retry which created a large surface area for bugs & meant that we emitted a lot of spurious warnings. This commit:
- Removes more kruft from aws-hyper and deals with the consequences
- Cleans up the way that HTTP clients are generated
- Passes in sleep to each incarnation of the retry controller to work around issues caused by late-binding vs. early binding sleep.

* first round of cleanups

* delete unused import

* Update integration tests

* Fix examples

* Fix aws-smithy-client docs

* Clippy fix

* fix flaky test

* Fix  which relies on tokio/io
parent eea27bb3

CHANGELOG.next.toml

+19 −0
Original line number Diff line number Diff line
@@ -70,3 +70,22 @@ message = "Add changelog automation to sdk-lints" 
references = ["smithy-rs#922", "smithy-rs#914"]

meta = { "breaking" = false, "tada" = true, "bug" = false }

author = "rcoh"



[[aws-sdk-rust]]

message = """

`aws_hyper::Client` which was just a re-export of `aws_smithy_types::Client` with generics set has been removed. If you used

`aws_hyper::Client` or `aws_hyper::Client::https()` you can update your code to use `aws_smithy_client::Builder::https()`. Other

usage examples can be found on the [aws_hyper docs](https://docs.rs/aws-hyper).



**Note**: `aws-hyper` will be removed in a coming release and `AwsMiddleware` will be migrated elsewhere.

"""

meta = { "breaking" = true, "tada" = false, "bug" = false }

author = "rcoh"

references = ["smithy-rs#940"]



[[smithy-rs]]

message = """`aws_smithy_client::Client::https()` has been renamed to `dyn_https()`.

This is to clearly distinguish it from `rustls` and `native_tls` which do not use a boxed connector."""

meta = { "breaking" = true, "tada" = false, "bug" = false }

author = "rcoh"

references = ["smithy-rs#940"]

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

+1 −1
Original line number Diff line number Diff line
@@ -444,11 +444,11 @@ mod test { 
    use crate::provider_config::ProviderConfig;

    use crate::test_case::GenericTestResult;



    use aws_hyper::DynConnector;

    use aws_types::credentials::ProvideCredentials;

    use aws_types::os_shim_internal::Env;

    use aws_types::Credentials;



    use aws_smithy_client::erase::DynConnector;

    use aws_smithy_client::test_connection::TestConnection;

    use aws_smithy_http::body::SdkBody;

    use http::header::AUTHORIZATION;

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

+3 −4
Original line number Diff line number Diff line
@@ -8,11 +8,10 @@ 
//!

//! Future work will stabilize this interface and enable it to be used directly.



use aws_hyper::{DynConnector, SdkSuccess};

use aws_smithy_http::body::SdkBody;

use aws_smithy_http::operation::{Operation, Request};

use aws_smithy_http::response::ParseStrictResponse;

use aws_smithy_http::result::SdkError;

use aws_smithy_http::result::{SdkError, SdkSuccess};

use aws_smithy_http::retry::ClassifyResponse;

use aws_smithy_types::retry::{ErrorKind, RetryKind};

use aws_types::credentials::CredentialsError;
@@ -22,6 +21,7 @@ use crate::connector::expect_connector; 
use crate::json_credentials::{parse_json_credentials, JsonCredentials};

use crate::provider_config::{HttpSettings, ProviderConfig};



use aws_smithy_client::erase::DynConnector;

use aws_smithy_client::timeout;

use bytes::Bytes;

use http::header::{ACCEPT, AUTHORIZATION};
@@ -195,11 +195,10 @@ impl ClassifyResponse<SdkSuccess<Credentials>, SdkError<CredentialsError>> 
#[cfg(test)]

mod test {

    use crate::http_provider::{CredentialsResponseParser, HttpCredentialRetryPolicy};

    use aws_hyper::SdkSuccess;

    use aws_smithy_http::body::SdkBody;

    use aws_smithy_http::operation;

    use aws_smithy_http::response::ParseStrictResponse;

    use aws_smithy_http::result::SdkError;

    use aws_smithy_http::result::{SdkError, SdkSuccess};

    use aws_smithy_http::retry::ClassifyResponse;

    use aws_smithy_types::retry::{ErrorKind, RetryKind};

    use aws_types::credentials::CredentialsError;

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

+5 −7
Original line number Diff line number Diff line
@@ -553,17 +553,16 @@ impl Builder { 
            self.token_ttl.unwrap_or(DEFAULT_TOKEN_TTL),

            retry_config.clone(),

            timeout_config.clone(),

            config.sleep(),

        );

        let middleware = ImdsMiddleware { token_loader };

        let mut inner_client = aws_smithy_client::Builder::new()

        let inner_client = aws_smithy_client::Builder::new()

            .connector(connector.clone())

            .middleware(middleware)

            .sleep_impl(config.sleep())

            .build()

            .with_retry_config(retry_config)

            .with_timeout_config(timeout_config);

        if let Some(sleep) = config.sleep() {

            inner_client = inner_client.with_sleep_impl(sleep);

        }



        let client = Client {

            endpoint,
@@ -724,8 +723,8 @@ pub(crate) mod test { 
    use std::error::Error;

    use std::time::{Duration, SystemTime, UNIX_EPOCH};



    use aws_hyper::DynConnector;

    use aws_smithy_async::rt::sleep::TokioSleep;

    use aws_smithy_client::erase::DynConnector;

    use aws_smithy_client::test_connection::{capture_request, TestConnection};

    use aws_smithy_http::body::SdkBody;

    use aws_types::os_shim_internal::{Env, Fs, ManualTimeSource, TimeSource};
@@ -1011,7 +1010,6 @@ pub(crate) mod test { 
    }



    /// Verify that the end-to-end real client has a 1-second connect timeout

    #[ignore]

    #[tokio::test]

    async fn one_second_connect_timeout() {

        let client = Client::builder()
@@ -1028,7 +1026,7 @@ pub(crate) mod test { 
        assert!(now.elapsed().unwrap() > Duration::from_secs(1));

        assert!(now.elapsed().unwrap() < Duration::from_secs(2));

        match resp {

            ImdsError::FailedToLoadToken(err) if format!("{}", err).contains("timed out") => {} // ok,

            ImdsError::FailedToLoadToken(err) if format!("{}", err).contains("timeout") => {} // ok,

            other => panic!(

                "wrong error, expected construction failure with TimedOutError inside: {}",

                other

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

+6 −1
Original line number Diff line number Diff line
@@ -34,6 +34,7 @@ use http::{HeaderValue, Uri}; 


use crate::cache::ExpiringCache;

use crate::imds::client::{ImdsError, ImdsErrorPolicy, TokenError};

use aws_smithy_async::rt::sleep::AsyncSleep;

use aws_smithy_client::retry;

use aws_smithy_types::timeout::TimeoutConfig;

use std::fmt::{Debug, Formatter};
@@ -84,8 +85,12 @@ impl TokenMiddleware { 
        token_ttl: Duration,

        retry_config: retry::Config,

        timeout_config: TimeoutConfig,

        sleep_impl: Option<Arc<dyn AsyncSleep>>,

    ) -> Self {

        let inner_client = aws_smithy_client::Client::new(connector)

        let inner_client = aws_smithy_client::Builder::new()

            .connector(connector)

            .sleep_impl(sleep_impl)

            .build()

            .with_retry_config(retry_config)

            .with_timeout_config(timeout_config);

        let client = Arc::new(inner_client);