Unverified Commit 1446681c authored by Russell Cohen's avatar Russell Cohen Committed by GitHub
Browse files

Add support for retrying individual call timeouts (#1478) 

When timeout support was added initially, the resulting error `SdkError::TimeoutError` was never added to the retry policy which prevented these errors from being retried. This was a bug—This commit rectifies the and adds two integration-level tests that ensure that timeouts are properly retried.
parent a3c7902d

CHANGELOG.next.toml

+10 −0
Original line number Diff line number Diff line
@@ -47,3 +47,13 @@ the prefix will be stripped automatically. 
references = ["aws-sdk-rust#554"]

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

author = "Velfi"



[[aws-sdk-rust]]

message = """

Fix bug in retry policy where user configured timeouts were not retried. With this fix, setting

[`with_call_attempt_timeout`](https://docs.rs/aws-smithy-types/0.43.0/aws_smithy_types/timeout/struct.Api.html#method.with_call_attempt_timeout)

will lead to a retry when retries are enabled.

"""

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

references = ["aws-sdk-rust#558", "smithy-rs#1478"]

author = "rcoh"

aws/rust-runtime/aws-http/src/retry.rs

+16 −2
Original line number Diff line number Diff line
@@ -60,6 +60,10 @@ where 
        let (err, response) = match err {

            Ok(_) => return RetryKind::Unnecessary,

            Err(SdkError::ServiceError { err, raw }) => (err, raw),

            Err(SdkError::TimeoutError(_err)) => {

                return RetryKind::Error(ErrorKind::TransientError)

            }



            Err(SdkError::DispatchFailure(err)) => {

                return if err.is_timeout() || err.is_io() {

                    RetryKind::Error(ErrorKind::TransientError)
@@ -67,7 +71,7 @@ where 
                    RetryKind::Error(ek)

                } else {

                    RetryKind::UnretryableFailure

                }

                };

            }

            Err(_) => return RetryKind::UnretryableFailure,

        };
@@ -198,7 +202,7 @@ mod test { 
                    CodedError {

                        code: "RequestTimeout"

                    },

                    test_response

                    test_response,

                )

                .as_ref()

            ),
@@ -253,4 +257,14 @@ mod test { 
            RetryKind::Explicit(Duration::from_millis(5000))

        );

    }



    #[test]

    fn test_timeout_error() {

        let policy = AwsErrorRetryPolicy::new();

        let err: Result<(), SdkError<UnmodeledError>> = Err(SdkError::TimeoutError("blah".into()));

        assert_eq!(

            policy.classify(err.as_ref()),

            RetryKind::Error(ErrorKind::TransientError)

        );

    }

}

aws/sdk/integration-tests/dynamodb/Cargo.toml

+2 −1
Original line number Diff line number Diff line
@@ -14,6 +14,7 @@ aws-smithy-client = { path = "../../build/aws-sdk/sdk/aws-smithy-client", featur 
aws-smithy-http = { path = "../../build/aws-sdk/sdk/aws-smithy-http" }

aws-smithy-types = { path = "../../build/aws-sdk/sdk/aws-smithy-types" }

aws-smithy-protocol-test = { path = "../../build/aws-sdk/sdk/aws-smithy-protocol-test" }

aws-smithy-async = { path = "../../build/aws-sdk/sdk/aws-smithy-async" }

aws-types = { path = "../../build/aws-sdk/sdk/aws-types" }

bytes = "1"

# TODO(https://github.com/awslabs/smithy-rs/issues/1044) v3.5 has an unmaintained dependency, upgrade this when possible

aws/sdk/integration-tests/dynamodb/tests/timeouts.rs

0 → 100644
+93 −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

 */



use aws_sdk_dynamodb::types::SdkError;

use aws_smithy_async::rt::sleep::{AsyncSleep, Sleep};

use aws_smithy_client::never::NeverConnector;

use aws_smithy_types::timeout;

use aws_smithy_types::timeout::Api;

use aws_smithy_types::tristate::TriState;

use aws_types::credentials::SharedCredentialsProvider;

use aws_types::region::Region;

use aws_types::{Credentials, SdkConfig};

use std::sync::Arc;

use std::time::Duration;



#[derive(Debug, Clone)]

struct InstantSleep;

impl AsyncSleep for InstantSleep {

    fn sleep(&self, _duration: Duration) -> Sleep {

        Sleep::new(Box::pin(async move {}))

    }

}



#[tokio::test]

async fn api_call_timeout_retries() {

    let conn = NeverConnector::new();

    let conf = SdkConfig::builder()

        .region(Region::new("us-east-2"))

        .credentials_provider(SharedCredentialsProvider::new(Credentials::new(

            "stub", "stub", None, None, "test",

        )))

        .timeout_config(timeout::Config::new().with_api_timeouts(

            Api::new().with_call_attempt_timeout(TriState::Set(Duration::new(123, 0))),

        ))

        .sleep_impl(Arc::new(InstantSleep))

        .build();

    let client = aws_sdk_dynamodb::Client::from_conf_conn(

        aws_sdk_dynamodb::Config::new(&conf),

        conn.clone(),

    );

    let resp = client

        .list_tables()

        .send()

        .await

        .expect_err("call should fail");

    assert_eq!(

        conn.num_calls(),

        3,

        "client level timeouts should be retried"

    );

    assert!(

        matches!(resp, SdkError::TimeoutError { .. }),

        "expected a timeout error, got: {}",

        resp

    );

}



#[tokio::test]

async fn no_retries_on_operation_timeout() {

    let conn = NeverConnector::new();

    let conf =

        SdkConfig::builder()

            .region(Region::new("us-east-2"))

            .credentials_provider(SharedCredentialsProvider::new(Credentials::new(

                "stub", "stub", None, None, "test",

            )))

            .timeout_config(timeout::Config::new().with_api_timeouts(

                Api::new().with_call_timeout(TriState::Set(Duration::new(123, 0))),

            ))

            .sleep_impl(Arc::new(InstantSleep))

            .build();

    let client = aws_sdk_dynamodb::Client::from_conf_conn(

        aws_sdk_dynamodb::Config::new(&conf),

        conn.clone(),

    );

    let resp = client

        .list_tables()

        .send()

        .await

        .expect_err("call should fail");

    assert_eq!(

        conn.num_calls(),

        1,

        "operation level timeouts should not be retried"

    );

    assert!(

        matches!(resp, SdkError::TimeoutError { .. }),

        "expected a timeout error, got: {}",

        resp

    );

}

rust-runtime/aws-smithy-client/src/never.rs

+11 −0
Original line number Diff line number Diff line
@@ -10,6 +10,8 @@ use http::Uri; 
use aws_smithy_async::future::never::Never;



use std::marker::PhantomData;

use std::sync::atomic::{AtomicU64, Ordering};

use std::sync::Arc;



use std::task::{Context, Poll};
@@ -25,12 +27,14 @@ use tower::BoxError; 
#[derive(Debug)]

pub struct NeverService<Req, Resp, Err> {

    _resp: PhantomData<(Req, Resp, Err)>,

    invocations: Arc<AtomicU64>,

}



impl<Req, Resp, Err> Clone for NeverService<Req, Resp, Err> {

    fn clone(&self) -> Self {

        Self {

            _resp: Default::default(),

            invocations: self.invocations.clone(),

        }

    }

}
@@ -46,8 +50,14 @@ impl<Req, Resp, Err> NeverService<Req, Resp, Err> { 
    pub fn new() -> Self {

        NeverService {

            _resp: Default::default(),

            invocations: Default::default(),

        }

    }



    /// Returns the number of invocations made to this service

    pub fn num_calls(&self) -> u64 {

        self.invocations.load(Ordering::SeqCst)

    }

}



/// A Connector that can be use with [`Client`](crate::Client) that never returns a response.
@@ -139,6 +149,7 @@ impl<Req, Resp, Err> tower::Service<Req> for NeverService<Req, Resp, Err> { 
    }



    fn call(&mut self, _req: Req) -> Self::Future {

        self.invocations.fetch_add(1, Ordering::SeqCst);

        Box::pin(async move {

            Never::new().await;

            unreachable!()