From e6177b3dc2625fb88c3433677339735b62154a86 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 14 Sep 2022 13:36:17 -0700 Subject: [PATCH] Fix native Smithy client retry and retry response errors (#1717) * Fix retry for native Smithy clients * Treat `SdkError::ResponseError` as a retryable transient failure * Rename `ClassifyResponse` to `ClassifyRetry` * Rename 'policy' to 'classifier' in AWS SDK public API * Rename `AwsResponseClassifier` to `AwsResponseRetryClassifier` Co-authored-by: Zelda Hessler --- CHANGELOG.next.toml | 44 ++++ .../src/http_credential_provider.rs | 26 ++- .../aws-config/src/imds/client.rs | 28 +-- .../aws-config/src/imds/client/token.rs | 4 +- aws/rust-runtime/aws-http/src/retry.rs | 125 ++++++----- .../tests/middleware_e2e_test.rs | 7 +- .../smithy/rustsdk/AwsCodegenDecorator.kt | 2 +- .../rustsdk/AwsFluentClientDecorator.kt | 8 +- ...corator.kt => RetryClassifierDecorator.kt} | 10 +- .../dynamodb/tests/movies.rs | 165 ++++---------- .../kms/tests/sensitive-it.rs | 10 +- .../smithy/generators/PaginatorGenerator.kt | 8 +- .../client/FluentClientGenerator.kt | 12 +- .../generators/client/FluentClientGenerics.kt | 8 +- .../protocol/MakeOperationGenerator.kt | 5 +- design/src/rfcs/rfc0010_waiters.md | 5 +- rust-runtime/aws-smithy-client/src/bounds.rs | 4 +- rust-runtime/aws-smithy-client/src/lib.rs | 2 +- rust-runtime/aws-smithy-client/src/retry.rs | 8 +- .../aws-smithy-client/src/static_tests.rs | 3 +- .../aws-smithy-client/tests/e2e_test.rs | 15 +- rust-runtime/aws-smithy-http-tower/src/lib.rs | 6 +- rust-runtime/aws-smithy-http/src/operation.rs | 21 +- rust-runtime/aws-smithy-http/src/retry.rs | 208 +++++++++++++++++- 24 files changed, 462 insertions(+), 272 deletions(-) rename aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/{RetryPolicyDecorator.kt => RetryClassifierDecorator.kt} (83%) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 4e13f28cd6..7d36001ddf 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -192,3 +192,47 @@ message = "The AWS STS SDK now automatically retries `IDPCommunicationError` whe references = ["smithy-rs#966", "smithy-rs#1718"] meta = { "breaking" = false, "tada" = false, "bug" = true } author = "jdisanti" + +[[smithy-rs]] +message = "Generated clients now retry transient errors without replacing the retry policy." +references = ["smithy-rs#1715", "smithy-rs#1717"] +meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" } +author = "jdisanti" + +[[smithy-rs]] +message = "`ClassifyResponse` was renamed to `ClassifyRetry` and is no longer implemented for the unit type." +references = ["smithy-rs#1715", "smithy-rs#1717"] +meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } +author = "jdisanti" + +[[smithy-rs]] +message = """ +The `with_retry_policy` and `retry_policy` functions on `aws_smithy_http::operation::Operation` have been +renamed to `with_retry_classifier` and `retry_classifier` respectively. Public member `retry_policy` on +`aws_smithy_http::operation::Parts` has been renamed to `retry_classifier`. +""" +references = ["smithy-rs#1715", "smithy-rs#1717"] +meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } +author = "jdisanti" + +[[aws-sdk-rust]] +message = "The `SdkError::ResponseError`, typically caused by a connection terminating before the full response is received, is now treated as a transient failure and retried." +references = ["aws-sdk-rust#303", "smithy-rs#1717"] +meta = { "breaking" = false, "tada" = false, "bug" = true } +author = "jdisanti" + +[[aws-sdk-rust]] +message = "`ClassifyResponse` was renamed to `ClassifyRetry` and is no longer implemented for the unit type." +references = ["smithy-rs#1715", "smithy-rs#1717"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "jdisanti" + +[[aws-sdk-rust]] +message = """ +The `with_retry_policy` and `retry_policy` functions on `aws_smithy_http::operation::Operation` have been +renamed to `with_retry_classifier` and `retry_classifier` respectively. Public member `retry_policy` on +`aws_smithy_http::operation::Parts` has been renamed to `retry_classifier`. +""" +references = ["smithy-rs#1715", "smithy-rs#1717"] +meta = { "breaking" = true, "tada" = false, "bug" = false } +author = "jdisanti" diff --git a/aws/rust-runtime/aws-config/src/http_credential_provider.rs b/aws/rust-runtime/aws-config/src/http_credential_provider.rs index d77aa81ca6..2e6d6e137f 100644 --- a/aws/rust-runtime/aws-config/src/http_credential_provider.rs +++ b/aws/rust-runtime/aws-config/src/http_credential_provider.rs @@ -14,7 +14,7 @@ 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, SdkSuccess}; -use aws_smithy_http::retry::ClassifyResponse; +use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, RetryKind}; use aws_smithy_types::timeout; use aws_smithy_types::tristate::TriState; @@ -58,7 +58,7 @@ impl HttpCredentialProvider { fn operation( &self, auth: Option, - ) -> Operation { + ) -> Operation { let mut http_req = http::Request::builder() .uri(&self.uri) .header(ACCEPT, "application/json"); @@ -73,7 +73,7 @@ impl HttpCredentialProvider { provider_name: self.provider_name, }, ) - .with_retry_policy(HttpCredentialRetryPolicy) + .with_retry_classifier(HttpCredentialRetryClassifier) } } @@ -165,12 +165,12 @@ impl ParseStrictResponse for CredentialsResponseParser { } #[derive(Clone, Debug)] -struct HttpCredentialRetryPolicy; +struct HttpCredentialRetryClassifier; -impl ClassifyResponse, SdkError> - for HttpCredentialRetryPolicy +impl ClassifyRetry, SdkError> + for HttpCredentialRetryClassifier { - fn classify( + fn classify_retry( &self, response: Result<&SdkSuccess, &SdkError>, ) -> RetryKind { @@ -206,12 +206,14 @@ impl ClassifyResponse, SdkError> #[cfg(test)] mod test { - use crate::http_credential_provider::{CredentialsResponseParser, HttpCredentialRetryPolicy}; + use crate::http_credential_provider::{ + CredentialsResponseParser, HttpCredentialRetryClassifier, + }; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation; use aws_smithy_http::response::ParseStrictResponse; use aws_smithy_http::result::{SdkError, SdkSuccess}; - use aws_smithy_http::retry::ClassifyResponse; + use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, RetryKind}; use aws_types::credentials::CredentialsError; use aws_types::Credentials; @@ -245,7 +247,7 @@ mod test { .unwrap(); assert_eq!( - HttpCredentialRetryPolicy.classify(sdk_resp(bad_response).as_ref()), + HttpCredentialRetryClassifier.classify_retry(sdk_resp(bad_response).as_ref()), RetryKind::Error(ErrorKind::ServerError) ); } @@ -266,7 +268,7 @@ mod test { let sdk_result = sdk_resp(ok_response); assert_eq!( - HttpCredentialRetryPolicy.classify(sdk_result.as_ref()), + HttpCredentialRetryClassifier.classify_retry(sdk_result.as_ref()), RetryKind::Unnecessary ); @@ -281,7 +283,7 @@ mod test { .unwrap(); let sdk_result = sdk_resp(error_response); assert_eq!( - HttpCredentialRetryPolicy.classify(sdk_result.as_ref()), + HttpCredentialRetryClassifier.classify_retry(sdk_result.as_ref()), RetryKind::UnretryableFailure ); let sdk_error = sdk_result.expect_err("should be error"); diff --git a/aws/rust-runtime/aws-config/src/imds/client.rs b/aws/rust-runtime/aws-config/src/imds/client.rs index 8f30468bcd..9e1fd43c7d 100644 --- a/aws/rust-runtime/aws-config/src/imds/client.rs +++ b/aws/rust-runtime/aws-config/src/imds/client.rs @@ -23,7 +23,7 @@ use aws_smithy_http::endpoint::Endpoint; use aws_smithy_http::operation; use aws_smithy_http::operation::{Metadata, Operation}; use aws_smithy_http::response::ParseStrictResponse; -use aws_smithy_http::retry::ClassifyResponse; +use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_http_tower::map_request::{ AsyncMapRequestLayer, AsyncMapRequestService, MapRequestLayer, MapRequestService, }; @@ -232,7 +232,7 @@ impl Client { fn make_operation( &self, path: &str, - ) -> Result, ImdsError> { + ) -> Result, ImdsError> { let mut base_uri: Uri = path.parse().map_err(|_| ImdsError::InvalidPath)?; self.inner.endpoint.set_endpoint(&mut base_uri, None); let request = http::Request::builder() @@ -243,7 +243,7 @@ impl Client { request.properties_mut().insert(user_agent()); Ok(Operation::new(request, ImdsGetResponseHandler) .with_metadata(Metadata::new("get", "imds")) - .with_retry_policy(ImdsErrorPolicy)) + .with_retry_classifier(ImdsResponseRetryClassifier)) } } @@ -706,9 +706,9 @@ impl Display for TokenError { impl Error for TokenError {} #[derive(Clone)] -struct ImdsErrorPolicy; +struct ImdsResponseRetryClassifier; -impl ImdsErrorPolicy { +impl ImdsResponseRetryClassifier { fn classify(response: &operation::Response) -> RetryKind { let status = response.http().status(); match status { @@ -721,7 +721,7 @@ impl ImdsErrorPolicy { } } -/// IMDS Retry Policy +/// IMDS Response Retry Classifier /// /// Possible status codes: /// - 200 (OK) @@ -730,12 +730,12 @@ impl ImdsErrorPolicy { /// - 403 (IMDS disabled): **Not Retryable** /// - 404 (Not found): **Not Retryable** /// - >=500 (server error): **Retryable** -impl ClassifyResponse, SdkError> for ImdsErrorPolicy { - fn classify(&self, response: Result<&SdkSuccess, &SdkError>) -> RetryKind { +impl ClassifyRetry, SdkError> for ImdsResponseRetryClassifier { + fn classify_retry(&self, response: Result<&SdkSuccess, &SdkError>) -> RetryKind { match response { Ok(_) => RetryKind::Unnecessary, Err(SdkError::ResponseError { raw, .. }) | Err(SdkError::ServiceError { raw, .. }) => { - ImdsErrorPolicy::classify(raw) + Self::classify(raw) } _ => RetryKind::UnretryableFailure, } @@ -744,7 +744,7 @@ impl ClassifyResponse, SdkError> for ImdsErrorPolicy { #[cfg(test)] pub(crate) mod test { - use crate::imds::client::{Client, EndpointMode, ImdsErrorPolicy}; + use crate::imds::client::{Client, EndpointMode, ImdsResponseRetryClassifier}; use crate::provider_config::ProviderConfig; use aws_smithy_async::rt::sleep::TokioSleep; use aws_smithy_client::erase::DynConnector; @@ -1033,9 +1033,9 @@ pub(crate) mod test { /// Successful responses should classify as `RetryKind::Unnecessary` #[test] fn successful_response_properly_classified() { - use aws_smithy_http::retry::ClassifyResponse; + use aws_smithy_http::retry::ClassifyRetry; - let policy = ImdsErrorPolicy; + let classifier = ImdsResponseRetryClassifier; fn response_200() -> operation::Response { operation::Response::new(imds_response("").map(|_| SdkBody::empty())) } @@ -1045,7 +1045,7 @@ pub(crate) mod test { }; assert_eq!( RetryKind::Unnecessary, - policy.classify(Ok::<_, &SdkError<()>>(&success)) + classifier.classify_retry(Ok::<_, &SdkError<()>>(&success)) ); // Emulate a failure to parse the response body (using an io error since it's easy to construct in a test) @@ -1055,7 +1055,7 @@ pub(crate) mod test { }; assert_eq!( RetryKind::UnretryableFailure, - policy.classify(Err::<&SdkSuccess<()>, _>(&failure)) + classifier.classify_retry(Err::<&SdkSuccess<()>, _>(&failure)) ); } diff --git a/aws/rust-runtime/aws-config/src/imds/client/token.rs b/aws/rust-runtime/aws-config/src/imds/client/token.rs index 52db23d259..ba14c092b8 100644 --- a/aws/rust-runtime/aws-config/src/imds/client/token.rs +++ b/aws/rust-runtime/aws-config/src/imds/client/token.rs @@ -38,7 +38,7 @@ use aws_types::os_shim_internal::TimeSource; use http::{HeaderValue, Uri}; use crate::cache::ExpiringCache; -use crate::imds::client::{ImdsError, ImdsErrorPolicy, TokenError}; +use crate::imds::client::{ImdsError, ImdsResponseRetryClassifier, TokenError}; /// Token Refresh Buffer /// @@ -143,7 +143,7 @@ impl TokenMiddleware { request.properties_mut().insert(super::user_agent()); let operation = Operation::new(request, self.token_parser.clone()) - .with_retry_policy(ImdsErrorPolicy) + .with_retry_classifier(ImdsResponseRetryClassifier) .with_metadata(Metadata::new("get-token", "imds")); let response = self .client diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 748d958415..4ee9a3c792 100644 --- a/aws/rust-runtime/aws-http/src/retry.rs +++ b/aws/rust-runtime/aws-http/src/retry.rs @@ -5,21 +5,10 @@ //! AWS-specific retry logic use aws_smithy_http::result::SdkError; -use aws_smithy_http::retry::ClassifyResponse; +use aws_smithy_http::retry::{ClassifyRetry, DefaultResponseRetryClassifier}; use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; use std::time::Duration; -/// A retry policy that models AWS error codes as outlined in the SEP -/// -/// In order of priority: -/// 1. The `x-amz-retry-after` header is checked -/// 2. The modeled error retry mode is checked -/// 3. The code is checked against a predetermined list of throttling errors & transient error codes -/// 4. The status code is checked against a predetermined list of status codes -#[non_exhaustive] -#[derive(Clone, Debug)] -pub struct AwsErrorRetryPolicy; - const TRANSIENT_ERROR_STATUS_CODES: &[u16] = &[500, 502, 503, 504]; const THROTTLING_ERRORS: &[&str] = &[ "Throttling", @@ -39,41 +28,42 @@ const THROTTLING_ERRORS: &[&str] = &[ ]; const TRANSIENT_ERRORS: &[&str] = &["RequestTimeout", "RequestTimeoutException"]; -impl AwsErrorRetryPolicy { - /// Create an `AwsErrorRetryPolicy` with the default set of known error & status codes +/// Implementation of [`ClassifyRetry`] that classifies AWS error codes. +/// +/// In order of priority: +/// 1. The `x-amz-retry-after` header is checked +/// 2. The modeled error retry mode is checked +/// 3. The code is checked against a predetermined list of throttling errors & transient error codes +/// 4. The status code is checked against a predetermined list of status codes +#[non_exhaustive] +#[derive(Clone, Debug)] +pub struct AwsResponseRetryClassifier; + +impl AwsResponseRetryClassifier { + /// Create an `AwsResponseRetryClassifier` with the default set of known error & status codes pub fn new() -> Self { - AwsErrorRetryPolicy + Self } } -impl Default for AwsErrorRetryPolicy { +impl Default for AwsResponseRetryClassifier { fn default() -> Self { Self::new() } } -impl ClassifyResponse> for AwsErrorRetryPolicy +impl ClassifyRetry> for AwsResponseRetryClassifier where E: ProvideErrorKind, { - fn classify(&self, err: Result<&T, &SdkError>) -> RetryKind { - 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) - } else if let Some(ek) = err.is_other() { - RetryKind::Error(ek) - } else { - RetryKind::UnretryableFailure - }; - } - Err(_) => return RetryKind::UnretryableFailure, + fn classify_retry(&self, result: Result<&T, &SdkError>) -> RetryKind { + // Run common retry classification logic from aws-smithy-http, and if it yields + // a `RetryKind`, then return that immediately. Otherwise, continue on to run some + // AWS SDK specific classification logic. + let (err, response) = match DefaultResponseRetryClassifier::try_extract_err_response(result) + { + Ok(extracted) => extracted, + Err(retry_kind) => return retry_kind, }; if let Some(retry_after_delay) = response .http() @@ -105,15 +95,23 @@ where #[cfg(test)] mod test { - use crate::retry::AwsErrorRetryPolicy; + use crate::retry::AwsResponseRetryClassifier; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation; use aws_smithy_http::result::{SdkError, SdkSuccess}; - use aws_smithy_http::retry::ClassifyResponse; + use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; + use std::fmt; use std::time::Duration; + #[derive(Debug)] struct UnmodeledError; + impl fmt::Display for UnmodeledError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "UnmodeledError") + } + } + impl std::error::Error for UnmodeledError {} struct CodedError { code: &'static str, @@ -151,36 +149,36 @@ mod test { #[test] fn not_an_error() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); let test_response = http::Response::new("OK"); assert_eq!( - policy.classify(make_err(UnmodeledError, test_response).as_ref()), + policy.classify_retry(make_err(UnmodeledError, test_response).as_ref()), RetryKind::UnretryableFailure ); } #[test] fn classify_by_response_status() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); let test_resp = http::Response::builder() .status(500) .body("error!") .unwrap(); assert_eq!( - policy.classify(make_err(UnmodeledError, test_resp).as_ref()), + policy.classify_retry(make_err(UnmodeledError, test_resp).as_ref()), RetryKind::Error(ErrorKind::TransientError) ); } #[test] fn classify_by_response_status_not_retryable() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); let test_resp = http::Response::builder() .status(408) .body("error!") .unwrap(); assert_eq!( - policy.classify(make_err(UnmodeledError, test_resp).as_ref()), + policy.classify_retry(make_err(UnmodeledError, test_resp).as_ref()), RetryKind::UnretryableFailure ); } @@ -188,16 +186,18 @@ mod test { #[test] fn classify_by_error_code() { let test_response = http::Response::new("OK"); - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( - policy.classify(make_err(CodedError { code: "Throttling" }, test_response).as_ref()), + policy.classify_retry( + make_err(CodedError { code: "Throttling" }, test_response).as_ref() + ), RetryKind::Error(ErrorKind::ThrottlingError) ); let test_response = http::Response::new("OK"); assert_eq!( - policy.classify( + policy.classify_retry( make_err( CodedError { code: "RequestTimeout" @@ -214,9 +214,9 @@ mod test { fn classify_generic() { let err = aws_smithy_types::Error::builder().code("SlowDown").build(); let test_response = http::Response::new("OK"); - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( - policy.classify(make_err(err, test_response).as_ref()), + policy.classify_retry(make_err(err, test_response).as_ref()), RetryKind::Error(ErrorKind::ThrottlingError) ); } @@ -236,34 +236,51 @@ mod test { } } - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( - policy.classify(make_err(ModeledRetries, test_response).as_ref()), + policy.classify_retry(make_err(ModeledRetries, test_response).as_ref()), RetryKind::Error(ErrorKind::ClientError) ); } #[test] fn test_retry_after_header() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); let test_response = http::Response::builder() .header("x-amz-retry-after", "5000") .body("retry later") .unwrap(); assert_eq!( - policy.classify(make_err(UnmodeledError, test_response).as_ref()), + policy.classify_retry(make_err(UnmodeledError, test_response).as_ref()), RetryKind::Explicit(Duration::from_millis(5000)) ); } + #[test] + fn classify_response_error() { + let policy = AwsResponseRetryClassifier::new(); + assert_eq!( + policy.classify_retry( + Result::, SdkError>::Err(SdkError::ResponseError { + err: Box::new(UnmodeledError), + raw: operation::Response::new( + http::Response::new("OK").map(|b| SdkBody::from(b)) + ), + }) + .as_ref() + ), + RetryKind::Error(ErrorKind::TransientError) + ); + } + #[test] fn test_timeout_error() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseRetryClassifier::new(); let err: Result<(), SdkError> = Err(SdkError::TimeoutError("blah".into())); assert_eq!( - policy.classify(err.as_ref()), + policy.classify_retry(err.as_ref()), RetryKind::Error(ErrorKind::TransientError) ); } diff --git a/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs b/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs index bfec2313d4..23932395db 100644 --- a/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs +++ b/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs @@ -15,7 +15,7 @@ use http::{self, Uri}; use aws_endpoint::partition::endpoint::{Protocol, SignatureVersion}; use aws_endpoint::{EndpointShim, Params}; -use aws_http::retry::AwsErrorRetryPolicy; +use aws_http::retry::AwsResponseRetryClassifier; use aws_http::user_agent::AwsUserAgent; use aws_inlineable::middleware::DefaultMiddleware; use aws_sig_auth::signer::OperationSigningConfig; @@ -75,7 +75,7 @@ impl ParseHttpResponse for TestOperationParser { } } -fn test_operation() -> Operation { +fn test_operation() -> Operation { let req = operation::Request::new( http::Request::builder() .uri("https://test-service.test-region.amazonaws.com/") @@ -110,7 +110,8 @@ fn test_operation() -> Operation { Result::<_, Infallible>::Ok(req) }) .unwrap(); - Operation::new(req, TestOperationParser).with_retry_policy(AwsErrorRetryPolicy::new()) + Operation::new(req, TestOperationParser) + .with_retry_classifier(AwsResponseRetryClassifier::new()) } #[cfg(any(feature = "native-tls", feature = "rustls"))] diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsCodegenDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsCodegenDecorator.kt index d228a60949..f63e7078d8 100644 --- a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsCodegenDecorator.kt +++ b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsCodegenDecorator.kt @@ -26,7 +26,7 @@ val DECORATORS = listOf( SigV4SigningDecorator(), HttpRequestChecksumDecorator(), HttpResponseChecksumDecorator(), - RetryPolicyDecorator(), + RetryClassifierDecorator(), IntegrationTestDecorator(), AwsFluentClientDecorator(), CrateLicenseDecorator(), diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt index aa49746e15..746443223b 100644 --- a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt +++ b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt @@ -74,7 +74,7 @@ private class AwsClientGenerics(private val types: Types) : FluentClientGenerics override val bounds = writable { } /** Bounds for generated `send()` functions */ - override fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: RuntimeType, retryPolicy: Writable): Writable = writable { } + override fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: RuntimeType, retryClassifier: RuntimeType): Writable = writable { } override fun toGenericsGenerator(): GenericsGenerator { return GenericsGenerator() @@ -98,7 +98,7 @@ class AwsFluentClientDecorator : RustCodegenDecorator { AwsPresignedFluentBuilderMethod(runtimeConfig), AwsFluentClientDocs(codegenContext), ), - retryPolicy = runtimeConfig.awsHttp().asType().member("retry::AwsErrorRetryPolicy").writable, + retryClassifier = runtimeConfig.awsHttp().asType().member("retry::AwsResponseRetryClassifier"), ).render(rustCrate) rustCrate.withModule(FluentClientGenerator.customizableOperationModule) { writer -> renderCustomizableOperationSendMethod(runtimeConfig, generics, writer) @@ -282,7 +282,7 @@ private fun renderCustomizableOperationSendMethod( "combined_generics_decl" to combinedGenerics.declaration(), "handle_generics_bounds" to handleGenerics.bounds(), "SdkSuccess" to smithyHttp.member("result::SdkSuccess"), - "ClassifyResponse" to smithyHttp.member("retry::ClassifyResponse"), + "ClassifyRetry" to smithyHttp.member("retry::ClassifyRetry"), "ParseHttpResponse" to smithyHttp.member("response::ParseHttpResponse"), ) @@ -297,7 +297,7 @@ private fun renderCustomizableOperationSendMethod( where E: std::error::Error, O: #{ParseHttpResponse}> + Send + Sync + Clone + 'static, - Retry: #{ClassifyResponse}<#{SdkSuccess}, SdkError> + Send + Sync + Clone, + Retry: #{ClassifyRetry}<#{SdkSuccess}, SdkError> + Send + Sync + Clone, { self.handle.client.call(self.operation).await } diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryPolicyDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt similarity index 83% rename from aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryPolicyDecorator.kt rename to aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt index dfb0bbdf59..30d3126f40 100644 --- a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryPolicyDecorator.kt +++ b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt @@ -17,7 +17,7 @@ import software.amazon.smithy.rust.codegen.client.smithy.customize.OperationCust import software.amazon.smithy.rust.codegen.client.smithy.customize.OperationSection import software.amazon.smithy.rust.codegen.client.smithy.customize.RustCodegenDecorator -class RetryPolicyDecorator : RustCodegenDecorator { +class RetryClassifierDecorator : RustCodegenDecorator { override val name: String = "RetryPolicy" override val order: Byte = 0 @@ -26,19 +26,19 @@ class RetryPolicyDecorator : RustCodegenDecorator { operation: OperationShape, baseCustomizations: List, ): List { - return baseCustomizations + RetryPolicyFeature(codegenContext.runtimeConfig) + return baseCustomizations + RetryClassifierFeature(codegenContext.runtimeConfig) } override fun supportsCodegenContext(clazz: Class): Boolean = clazz.isAssignableFrom(ClientCodegenContext::class.java) } -class RetryPolicyFeature(private val runtimeConfig: RuntimeConfig) : OperationCustomization() { - override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsErrorRetryPolicy") +class RetryClassifierFeature(private val runtimeConfig: RuntimeConfig) : OperationCustomization() { + override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsResponseRetryClassifier") override fun section(section: OperationSection) = when (section) { is OperationSection.FinalizeOperation -> writable { rust( - "let ${section.operation} = ${section.operation}.with_retry_policy(#T::new());", + "let ${section.operation} = ${section.operation}.with_retry_classifier(#T::new());", retryType(), ) } diff --git a/aws/sdk/integration-tests/dynamodb/tests/movies.rs b/aws/sdk/integration-tests/dynamodb/tests/movies.rs index c3a0f079ad..c6cded6c0f 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/movies.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/movies.rs @@ -4,24 +4,14 @@ */ use aws_sdk_dynamodb as dynamodb; - -use aws_http::retry::AwsErrorRetryPolicy; -use aws_sdk_dynamodb::input::CreateTableInput; use aws_smithy_client::test_connection::TestConnection; use aws_smithy_http::body::SdkBody; -use aws_smithy_http::operation::Operation; -use aws_smithy_http::result::{SdkError, SdkSuccess}; -use aws_smithy_http::retry::ClassifyResponse; -use aws_smithy_types::retry::RetryKind; -use dynamodb::error::DescribeTableError; -use dynamodb::input::{DescribeTableInput, PutItemInput, QueryInput}; use dynamodb::model::{ AttributeDefinition, AttributeValue, KeySchemaElement, KeyType, ProvisionedThroughput, ScalarAttributeType, TableStatus, }; -use dynamodb::operation::{CreateTable, DescribeTable}; -use dynamodb::output::DescribeTableOutput; -use dynamodb::{Config, Credentials, Region}; +use dynamodb::output::QueryOutput; +use dynamodb::{Client, Credentials, Region}; use http::header::{HeaderName, AUTHORIZATION}; use http::Uri; use serde_json::Value; @@ -29,12 +19,9 @@ use std::collections::HashMap; use std::time::Duration; use tokio::time::Instant; -use aws_sdk_dynamodb::middleware::DefaultMiddleware; -use aws_smithy_client::Client as CoreClient; -pub type Client = CoreClient; - -fn create_table(table_name: &str) -> CreateTableInput { - CreateTable::builder() +async fn create_table(client: &Client, table_name: &str) { + client + .create_table() .table_name(table_name) .key_schema( KeySchemaElement::builder() @@ -66,8 +53,9 @@ fn create_table(table_name: &str) -> CreateTableInput { .write_capacity_units(10) .build(), ) - .build() - .expect("valid operation") + .send() + .await + .expect("failed to create table"); } fn value_to_item(value: Value) -> AttributeValue { @@ -83,92 +71,57 @@ fn value_to_item(value: Value) -> AttributeValue { } } -fn add_item(table_name: impl Into, item: Value) -> PutItemInput { +async fn add_item(client: &Client, table_name: impl Into, item: Value) { let attribute_value = match value_to_item(item) { AttributeValue::M(map) => map, other => panic!("can only insert top level values, got {:?}", other), }; - PutItemInput::builder() + client + .put_item() .table_name(table_name) .set_item(Some(attribute_value)) - .build() - .expect("valid operation") + .send() + .await + .expect("valid operation"); } -fn movies_in_year(table_name: &str, year: u16) -> QueryInput { +async fn movies_in_year(client: &Client, table_name: &str, year: u16) -> QueryOutput { let mut expr_attrib_names = HashMap::new(); expr_attrib_names.insert("#yr".to_string(), "year".to_string()); let mut expr_attrib_values = HashMap::new(); expr_attrib_values.insert(":yyyy".to_string(), AttributeValue::N(year.to_string())); - QueryInput::builder() + + client + .query() .table_name(table_name) .key_condition_expression("#yr = :yyyy") .set_expression_attribute_names(Some(expr_attrib_names)) .set_expression_attribute_values(Some(expr_attrib_values)) - .build() + .send() + .await .expect("valid operation") } -/// Hand-written waiter to retry every second until the table is out of `Creating` state -#[derive(Clone)] -struct WaitForReadyTable { - inner: R, -} - -impl ClassifyResponse, SdkError> - for WaitForReadyTable -where - R: ClassifyResponse, SdkError>, -{ - fn classify( - &self, - response: Result<&SdkSuccess, &SdkError>, - ) -> RetryKind { - match self.inner.classify(response) { - RetryKind::UnretryableFailure | RetryKind::Unnecessary => (), - other => return other, - }; - match response { - Ok(SdkSuccess { parsed, .. }) => { - if parsed - .table - .as_ref() - .unwrap() - .table_status - .as_ref() - .unwrap() - == &TableStatus::Creating - { - RetryKind::Explicit(Duration::from_secs(1)) - } else { - RetryKind::Unnecessary - } +/// Poll the DescribeTable operation once per second until the table exists. +async fn wait_for_ready_table(client: &Client, table_name: &str) { + loop { + if let Some(table) = client + .describe_table() + .table_name(table_name) + .send() + .await + .expect("success") + .table() + { + if !matches!(table.table_status, Some(TableStatus::Creating)) { + break; } - _ => RetryKind::UnretryableFailure, } + tokio::time::sleep(Duration::from_secs(1)).await; } } -/// Construct a `DescribeTable` request with a policy to retry every second until the table -/// is ready -async fn wait_for_ready_table( - table_name: &str, - conf: &Config, -) -> Operation> { - let operation = DescribeTableInput::builder() - .table_name(table_name) - .build() - .unwrap() - .make_operation(&conf) - .await - .expect("valid operation"); - let waiting_policy = WaitForReadyTable { - inner: operation.retry_policy().clone(), - }; - operation.with_retry_policy(waiting_policy) -} - /// Validate that time has passed with a 5ms tolerance /// /// This is to account for some non-determinism in the Tokio timer @@ -193,7 +146,6 @@ async fn movies_it() { // The waiter will retry 5 times tokio::time::pause(); let conn = movies_it_test_connection(); // RecordingConnection::https(); - let client = Client::new(conn.clone()); let conf = dynamodb::Config::builder() .region(Region::new("us-east-1")) .credentials_provider(Credentials::new( @@ -204,21 +156,12 @@ async fn movies_it() { "test", )) .build(); - client - .call( - create_table(table_name) - .make_operation(&conf) - .await - .expect("valid request"), - ) - .await - .expect("failed to create table"); + let client = Client::from_conf_conn(conf, conn.clone()); + + create_table(&client, table_name).await; let waiter_start = tokio::time::Instant::now(); - client - .call(wait_for_ready_table(table_name, &conf).await) - .await - .expect("table should become ready"); + wait_for_ready_table(&client, table_name).await; assert_time_passed(waiter_start, Duration::from_secs(4)); // data.json contains 2 movies from 2013 @@ -228,37 +171,13 @@ async fn movies_it() { data => panic!("data must be an array, got: {:?}", data), }; for item in data { - client - .call( - add_item(table_name, item.clone()) - .make_operation(&conf) - .await - .expect("valid request"), - ) - .await - .expect("failed to insert item"); + add_item(&client, table_name, item.clone()).await; } - let films_2222 = client - .call( - movies_in_year(table_name, 2222) - .make_operation(&conf) - .await - .expect("valid request"), - ) - .await - .expect("query should succeed"); - // this isn't back to the future, there are no movies from 2022 + let films_2222 = movies_in_year(&client, table_name, 2222).await; + // this isn't "Back To The Future", there are no movies from 2222 assert_eq!(films_2222.count, 0); - let films_2013 = client - .call( - movies_in_year(table_name, 2013) - .make_operation(&conf) - .await - .expect("valid request"), - ) - .await - .expect("query should succeed"); + let films_2013 = movies_in_year(&client, table_name, 2013).await; assert_eq!(films_2013.count, 2); let titles: Vec = films_2013 .items diff --git a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs index ec49cdd08b..610d40dfe2 100644 --- a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs +++ b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs @@ -3,13 +3,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -use aws_http::retry::AwsErrorRetryPolicy; +use aws_http::retry::AwsResponseRetryClassifier; use aws_sdk_kms as kms; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation::{self, Parts}; use aws_smithy_http::response::ParseStrictResponse; use aws_smithy_http::result::SdkError; -use aws_smithy_http::retry::ClassifyResponse; +use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, RetryKind}; use bytes::Bytes; use kms::error::CreateAliasError; @@ -65,7 +65,7 @@ fn types_are_debug() { assert_debug::(); } -async fn create_alias_op() -> Parts { +async fn create_alias_op() -> Parts { let conf = kms::Config::builder().build(); let (_, parts) = CreateAlias::builder() .build() @@ -94,7 +94,7 @@ async fn errors_are_retryable() { err: e, raw: operation::Response::new(http_response.map(SdkBody::from)), }); - let retry_kind = op.retry_policy.classify(err.as_ref()); + let retry_kind = op.retry_classifier.classify_retry(err.as_ref()); assert_eq!(retry_kind, RetryKind::Error(ErrorKind::ThrottlingError)); } @@ -112,6 +112,6 @@ async fn unmodeled_errors_are_retryable() { err: e, raw: operation::Response::new(http_response.map(SdkBody::from)), }); - let retry_kind = op.retry_policy.classify(err.as_ref()); + let retry_kind = op.retry_classifier.classify_retry(err.as_ref()); assert_eq!(retry_kind, RetryKind::Error(ErrorKind::ThrottlingError)); } diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/PaginatorGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/PaginatorGenerator.kt index 052e196f0b..9c1873627a 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/PaginatorGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/PaginatorGenerator.kt @@ -48,14 +48,14 @@ class PaginatorGenerator private constructor( service: ServiceShape, operation: OperationShape, private val generics: FluentClientGenerics, - retryPolicy: Writable = RustType.Unit.writable, + retryClassifier: RuntimeType, ) { companion object { fun paginatorType( coreCodegenContext: CoreCodegenContext, generics: FluentClientGenerics, operationShape: OperationShape, - retryPolicy: Writable = RustType.Unit.writable, + retryClassifier: RuntimeType, ): RuntimeType? { return if (operationShape.isPaginated(coreCodegenContext.model)) { PaginatorGenerator( @@ -64,7 +64,7 @@ class PaginatorGenerator private constructor( coreCodegenContext.serviceShape, operationShape, generics, - retryPolicy, + retryClassifier, ).paginatorType() } else { null @@ -98,7 +98,7 @@ class PaginatorGenerator private constructor( "generics" to generics.decl, "bounds" to generics.bounds, "page_size_setter" to pageSizeSetter(), - "send_bounds" to generics.sendBounds(symbolProvider.toSymbol(operation), outputType, errorType, retryPolicy), + "send_bounds" to generics.sendBounds(symbolProvider.toSymbol(operation), outputType, errorType, retryClassifier), // Operation Types "operation" to symbolProvider.toSymbol(operation), diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt index 8699c19c2c..c0e53cf052 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt @@ -17,7 +17,6 @@ import software.amazon.smithy.rust.codegen.client.rustlang.RustModule import software.amazon.smithy.rust.codegen.client.rustlang.RustReservedWords import software.amazon.smithy.rust.codegen.client.rustlang.RustType import software.amazon.smithy.rust.codegen.client.rustlang.RustWriter -import software.amazon.smithy.rust.codegen.client.rustlang.Writable import software.amazon.smithy.rust.codegen.client.rustlang.asArgumentType import software.amazon.smithy.rust.codegen.client.rustlang.asOptional import software.amazon.smithy.rust.codegen.client.rustlang.asType @@ -67,7 +66,8 @@ class FluentClientGenerator( client = CargoDependency.SmithyClient(codegenContext.runtimeConfig).asType(), ), private val customizations: List = emptyList(), - private val retryPolicy: Writable = RustType.Unit.writable, + private val retryClassifier: RuntimeType = CargoDependency.SmithyHttp(codegenContext.runtimeConfig).asType() + .member("retry::DefaultResponseRetryClassifier"), ) { companion object { fun clientOperationFnName(operationShape: OperationShape, symbolProvider: RustSymbolProvider): String = @@ -317,19 +317,19 @@ class FluentClientGenerator( self.handle.client.call(op).await } """, - "ClassifyResponse" to runtimeConfig.smithyHttp().member("retry::ClassifyResponse"), + "ClassifyRetry" to runtimeConfig.smithyHttp().member("retry::ClassifyRetry"), "OperationError" to errorType, "OperationOutput" to outputType, "SdkError" to runtimeConfig.smithyHttp().member("result::SdkError"), "SdkSuccess" to runtimeConfig.smithyHttp().member("result::SdkSuccess"), - "send_bounds" to generics.sendBounds(operationSymbol, outputType, errorType, retryPolicy), + "send_bounds" to generics.sendBounds(operationSymbol, outputType, errorType, retryClassifier), "customizable_op_type_params" to rustTypeParameters( symbolProvider.toSymbol(operation), - retryPolicy, + retryClassifier, generics.toGenericsGenerator(), ), ) - PaginatorGenerator.paginatorType(codegenContext, generics, operation, retryPolicy)?.also { paginatorType -> + PaginatorGenerator.paginatorType(codegenContext, generics, operation, retryClassifier)?.also { paginatorType -> rustTemplate( """ /// Create a paginator for this request diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerics.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerics.kt index 7ae3c0033f..e5300d1a05 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerics.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerics.kt @@ -28,7 +28,7 @@ interface FluentClientGenerics { val bounds: Writable /** Bounds for generated `send()` functions */ - fun sendBounds(input: Symbol, output: Symbol, error: RuntimeType, retryPolicy: Writable): Writable + fun sendBounds(operation: Symbol, output: Symbol, error: RuntimeType, retryClassifier: RuntimeType): Writable /** Convert this `FluentClientGenerics` into the more general `GenericsGenerator` */ fun toGenericsGenerator(): GenericsGenerator @@ -70,7 +70,7 @@ data class FlexibleClientGenerics( } /** Bounds for generated `send()` functions */ - override fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: RuntimeType, retryPolicy: Writable): Writable = writable { + override fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: RuntimeType, retryClassifier: RuntimeType): Writable = writable { rustTemplate( """ where @@ -78,14 +78,14 @@ data class FlexibleClientGenerics( #{Operation}, #{OperationOutput}, #{OperationError}, - #{RetryPolicy:W} + #{RetryClassifier} > """, "client" to client, "Operation" to operation, "OperationOutput" to operationOutput, "OperationError" to operationError, - "RetryPolicy" to retryPolicy, + "RetryClassifier" to retryClassifier, ) } diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/MakeOperationGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/MakeOperationGenerator.kt index 2d781e24f2..2771e7b345 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/MakeOperationGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/protocol/MakeOperationGenerator.kt @@ -10,7 +10,6 @@ import software.amazon.smithy.model.shapes.BlobShape import software.amazon.smithy.model.shapes.OperationShape import software.amazon.smithy.rust.codegen.client.rustlang.Attribute import software.amazon.smithy.rust.codegen.client.rustlang.CargoDependency -import software.amazon.smithy.rust.codegen.client.rustlang.RustType import software.amazon.smithy.rust.codegen.client.rustlang.RustWriter import software.amazon.smithy.rust.codegen.client.rustlang.asType import software.amazon.smithy.rust.codegen.client.rustlang.docs @@ -48,6 +47,8 @@ open class MakeOperationGenerator( protected val runtimeConfig = coreCodegenContext.runtimeConfig protected val symbolProvider = coreCodegenContext.symbolProvider protected val httpBindingResolver = protocol.httpBindingResolver + private val defaultClassifier = CargoDependency.SmithyHttp(runtimeConfig) + .asType().member("retry::DefaultResponseRetryClassifier") private val sdkId = coreCodegenContext.serviceShape.getTrait()?.sdkId?.lowercase()?.replace(" ", "") @@ -152,7 +153,7 @@ open class MakeOperationGenerator( writer.format(symbolProvider.toSymbol(shape)) private fun buildOperationTypeRetry(writer: RustWriter, customizations: List): String = - (customizations.firstNotNullOfOrNull { it.retryType() } ?: RustType.Unit).let { writer.format(it) } + (customizations.firstNotNullOfOrNull { it.retryType() } ?: defaultClassifier).let { writer.format(it) } private fun needsContentLength(operationShape: OperationShape): Boolean { return protocol.httpBindingResolver.requestBindings(operationShape) diff --git a/design/src/rfcs/rfc0010_waiters.md b/design/src/rfcs/rfc0010_waiters.md index ba6293b98f..f738f0f357 100644 --- a/design/src/rfcs/rfc0010_waiters.md +++ b/design/src/rfcs/rfc0010_waiters.md @@ -146,9 +146,8 @@ pub async fn wait( .map_err(|err| { aws_smithy_http::result::SdkError::ConstructionFailure(err.into()) })?; - // The `ClassifyResponse` trait is implemented for `()` as never retry, - // so this disables the default retry for the operation - let operation = operation.with_retry_policy(()); + // Assume `ClassifyRetry` trait is implemented for `NeverRetry` to always return `RetryKind::Unnecessary` + let operation = operation.with_retry_classifier(NeverRetry::new()); let result = handle.client.call(operation).await; match classify_result(&input, result) { diff --git a/rust-runtime/aws-smithy-client/src/bounds.rs b/rust-runtime/aws-smithy-client/src/bounds.rs index 13c6443497..32478e1d12 100644 --- a/rust-runtime/aws-smithy-client/src/bounds.rs +++ b/rust-runtime/aws-smithy-client/src/bounds.rs @@ -135,7 +135,7 @@ pub trait SmithyRetryPolicy: /// Forwarding type to `Retry` for bound inference. /// /// See module-level docs for details. - type Retry: ClassifyResponse, SdkError>; + type Retry: ClassifyRetry, SdkError>; } impl SmithyRetryPolicy for R @@ -143,7 +143,7 @@ where R: tower::retry::Policy, SdkSuccess, SdkError> + Clone, O: ParseHttpResponse> + Send + Sync + Clone + 'static, E: Error, - Retry: ClassifyResponse, SdkError>, + Retry: ClassifyRetry, SdkError>, { type O = O; type E = E; diff --git a/rust-runtime/aws-smithy-client/src/lib.rs b/rust-runtime/aws-smithy-client/src/lib.rs index b5d44612e7..1ab3fb4f86 100644 --- a/rust-runtime/aws-smithy-client/src/lib.rs +++ b/rust-runtime/aws-smithy-client/src/lib.rs @@ -94,7 +94,7 @@ use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation::Operation; use aws_smithy_http::response::ParseHttpResponse; pub use aws_smithy_http::result::{SdkError, SdkSuccess}; -use aws_smithy_http::retry::ClassifyResponse; +use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_http_tower::dispatch::DispatchLayer; use aws_smithy_http_tower::parse_response::ParseResponseLayer; use aws_smithy_types::retry::ProvideErrorKind; diff --git a/rust-runtime/aws-smithy-client/src/retry.rs b/rust-runtime/aws-smithy-client/src/retry.rs index 3c9bdc27c5..d58194e70e 100644 --- a/rust-runtime/aws-smithy-client/src/retry.rs +++ b/rust-runtime/aws-smithy-client/src/retry.rs @@ -21,7 +21,7 @@ use crate::{SdkError, SdkSuccess}; use aws_smithy_async::rt::sleep::AsyncSleep; use aws_smithy_http::operation::Operation; -use aws_smithy_http::retry::ClassifyResponse; +use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, RetryKind}; use tracing::Instrument; @@ -364,7 +364,7 @@ impl tower::retry::Policy, SdkSuccess for RetryHandler where Handler: Clone, - R: ClassifyResponse, SdkError>, + R: ClassifyRetry, SdkError>, { type Future = BoxFuture; @@ -373,8 +373,8 @@ where req: &Operation, result: Result<&SdkSuccess, &SdkError>, ) -> Option { - let policy = req.retry_policy(); - let retry_kind = policy.classify(result); + let classifier = req.retry_classifier(); + let retry_kind = classifier.classify_retry(result); self.retry_for(retry_kind) } diff --git a/rust-runtime/aws-smithy-client/src/static_tests.rs b/rust-runtime/aws-smithy-client/src/static_tests.rs index f9835135a3..4c9ef91bad 100644 --- a/rust-runtime/aws-smithy-client/src/static_tests.rs +++ b/rust-runtime/aws-smithy-client/src/static_tests.rs @@ -7,6 +7,7 @@ use crate::{Builder, Error, Operation, ParseHttpResponse, ProvideErrorKind}; use aws_smithy_http::operation; +use aws_smithy_http::retry::DefaultResponseRetryClassifier; #[derive(Debug)] #[non_exhaustive] @@ -40,7 +41,7 @@ impl ParseHttpResponse for TestOperation { unreachable!("only used for static tests") } } -pub type ValidTestOperation = Operation; +pub type ValidTestOperation = Operation; // Statically check that a standard retry can actually be used to build a Client. #[allow(dead_code)] diff --git a/rust-runtime/aws-smithy-client/tests/e2e_test.rs b/rust-runtime/aws-smithy-client/tests/e2e_test.rs index c5c4d17179..d2e393f8a8 100644 --- a/rust-runtime/aws-smithy-client/tests/e2e_test.rs +++ b/rust-runtime/aws-smithy-client/tests/e2e_test.rs @@ -3,9 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -use crate::test_operation::{TestOperationParser, TestPolicy}; +use crate::test_operation::{TestOperationParser, TestRetryClassifier}; use aws_smithy_async::rt::sleep::TokioSleep; - use aws_smithy_client::test_connection::TestConnection; use aws_smithy_client::Client; use aws_smithy_http::body::SdkBody; @@ -20,7 +19,7 @@ mod test_operation { use aws_smithy_http::operation; use aws_smithy_http::response::ParseHttpResponse; use aws_smithy_http::result::SdkError; - use aws_smithy_http::retry::ClassifyResponse; + use aws_smithy_http::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; use bytes::Bytes; use std::error::Error; @@ -67,14 +66,14 @@ mod test_operation { } #[derive(Clone)] - pub(super) struct TestPolicy; + pub(super) struct TestRetryClassifier; - impl ClassifyResponse> for TestPolicy + impl ClassifyRetry> for TestRetryClassifier where E: ProvideErrorKind + Debug, T: Debug, { - fn classify(&self, err: Result<&T, &SdkError>) -> RetryKind { + fn classify_retry(&self, err: Result<&T, &SdkError>) -> RetryKind { let kind = match err { Err(SdkError::ServiceError { err, .. }) => err.retryable_error_kind(), Ok(_) => return RetryKind::Unnecessary, @@ -88,14 +87,14 @@ mod test_operation { } } -fn test_operation() -> Operation { +fn test_operation() -> Operation { let req = operation::Request::new( http::Request::builder() .uri("https://test-service.test-region.amazonaws.com/") .body(SdkBody::from("request body")) .unwrap(), ); - Operation::new(req, TestOperationParser).with_retry_policy(TestPolicy) + Operation::new(req, TestOperationParser).with_retry_classifier(TestRetryClassifier) } #[tokio::test] diff --git a/rust-runtime/aws-smithy-http-tower/src/lib.rs b/rust-runtime/aws-smithy-http-tower/src/lib.rs index eefe1bb613..e79e48d728 100644 --- a/rust-runtime/aws-smithy-http-tower/src/lib.rs +++ b/rust-runtime/aws-smithy-http-tower/src/lib.rs @@ -62,6 +62,7 @@ mod tests { use aws_smithy_http::operation::{Operation, Request}; use aws_smithy_http::response::ParseStrictResponse; use aws_smithy_http::result::ConnectorError; + use aws_smithy_http::retry::DefaultResponseRetryClassifier; use bytes::Bytes; use http::Response; use std::convert::{Infallible, TryInto}; @@ -102,7 +103,10 @@ mod tests { }); let mut svc = ServiceBuilder::new() - .layer(ParseResponseLayer::::new()) + .layer(ParseResponseLayer::< + TestParseResponse, + DefaultResponseRetryClassifier, + >::new()) .layer(MapRequestLayer::for_mapper(AddHeader)) .layer(DispatchLayer) .service(http_layer); diff --git a/rust-runtime/aws-smithy-http/src/operation.rs b/rust-runtime/aws-smithy-http/src/operation.rs index 4d2bb1a3eb..7fc7ce5174 100644 --- a/rust-runtime/aws-smithy-http/src/operation.rs +++ b/rust-runtime/aws-smithy-http/src/operation.rs @@ -5,6 +5,7 @@ use crate::body::SdkBody; use crate::property_bag::{PropertyBag, SharedPropertyBag}; +use crate::retry::DefaultResponseRetryClassifier; use aws_smithy_types::date_time::DateTimeFormatError; use http::uri::InvalidUri; use std::borrow::Cow; @@ -42,7 +43,7 @@ impl Metadata { #[derive(Clone, Debug)] pub struct Parts { pub response_handler: H, - pub retry_policy: R, + pub retry_classifier: R, pub metadata: Option, } @@ -166,6 +167,9 @@ impl From for SerializationError { } } +// Generics: +// - H: Response handler +// - R: Implementation of `ClassifyRetry` #[derive(Debug)] pub struct Operation { request: Request, @@ -203,19 +207,19 @@ impl Operation { self } - pub fn with_retry_policy(self, retry_policy: R2) -> Operation { + pub fn with_retry_classifier(self, retry_classifier: R2) -> Operation { Operation { request: self.request, parts: Parts { response_handler: self.parts.response_handler, - retry_policy, + retry_classifier, metadata: self.parts.metadata, }, } } - pub fn retry_policy(&self) -> &R { - &self.parts.retry_policy + pub fn retry_classifier(&self) -> &R { + &self.parts.retry_classifier } pub fn try_clone(&self) -> Option @@ -232,12 +236,15 @@ impl Operation { } impl Operation { - pub fn new(request: Request, response_handler: H) -> Self { + pub fn new( + request: Request, + response_handler: H, + ) -> Operation { Operation { request, parts: Parts { response_handler, - retry_policy: (), + retry_classifier: DefaultResponseRetryClassifier::new(), metadata: None, }, } diff --git a/rust-runtime/aws-smithy-http/src/retry.rs b/rust-runtime/aws-smithy-http/src/retry.rs index 2b133dc13f..b562b66290 100644 --- a/rust-runtime/aws-smithy-http/src/retry.rs +++ b/rust-runtime/aws-smithy-http/src/retry.rs @@ -7,14 +7,210 @@ //! //! For protocol agnostic retries, see `aws_smithy_types::Retry`. -use aws_smithy_types::retry::RetryKind; +use crate::operation::Response; +use crate::result::SdkError; +use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; -pub trait ClassifyResponse: Clone { - fn classify(&self, response: Result<&T, &E>) -> RetryKind; +/// Classifies what kind of retry is needed for a given `response`. +pub trait ClassifyRetry: Clone { + fn classify_retry(&self, response: Result<&T, &E>) -> RetryKind; } -impl ClassifyResponse for () { - fn classify(&self, _: Result<&T, &E>) -> RetryKind { - RetryKind::Unnecessary +const TRANSIENT_ERROR_STATUS_CODES: &[u16] = &[500, 502, 503, 504]; + +/// The default implementation of [`ClassifyRetry`] for generated clients. +#[derive(Clone, Debug, Default)] +pub struct DefaultResponseRetryClassifier; + +impl DefaultResponseRetryClassifier { + /// Creates a new `DefaultResponseRetryClassifier` + pub fn new() -> Self { + Default::default() + } + + /// Matches on the given `result` and, if possible, returns the underlying cause and the operation response + /// that can be used for further classification logic. Otherwise, it returns a `RetryKind` that should be used + /// for the result. + // + // IMPORTANT: This function is used by the AWS SDK in `aws-http` for the SDK's own response classification logic + #[doc(hidden)] + pub fn try_extract_err_response<'a, T, E>( + result: Result<&T, &'a SdkError>, + ) -> Result<(&'a E, &'a Response), RetryKind> { + match result { + Ok(_) => Err(RetryKind::Unnecessary), + Err(SdkError::ServiceError { err, raw }) => Ok((err, raw)), + Err(SdkError::TimeoutError(_err)) => Err(RetryKind::Error(ErrorKind::TransientError)), + Err(SdkError::DispatchFailure(err)) => { + if err.is_timeout() || err.is_io() { + Err(RetryKind::Error(ErrorKind::TransientError)) + } else if let Some(ek) = err.is_other() { + Err(RetryKind::Error(ek)) + } else { + Err(RetryKind::UnretryableFailure) + } + } + Err(SdkError::ResponseError { .. }) => Err(RetryKind::Error(ErrorKind::TransientError)), + Err(SdkError::ConstructionFailure(_)) => Err(RetryKind::UnretryableFailure), + } + } +} + +impl ClassifyRetry> for DefaultResponseRetryClassifier +where + E: ProvideErrorKind, +{ + fn classify_retry(&self, result: Result<&T, &SdkError>) -> RetryKind { + let (err, response) = match Self::try_extract_err_response(result) { + Ok(extracted) => extracted, + Err(retry_kind) => return retry_kind, + }; + if let Some(kind) = err.retryable_error_kind() { + return RetryKind::Error(kind); + }; + if TRANSIENT_ERROR_STATUS_CODES.contains(&response.http().status().as_u16()) { + return RetryKind::Error(ErrorKind::TransientError); + }; + RetryKind::UnretryableFailure + } +} + +#[cfg(test)] +mod test { + use super::*; + use crate::body::SdkBody; + use crate::operation; + use crate::result::{SdkError, SdkSuccess}; + use crate::retry::ClassifyRetry; + use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; + use std::fmt; + + #[derive(Debug)] + struct UnmodeledError; + impl fmt::Display for UnmodeledError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "UnmodeledError") + } + } + impl std::error::Error for UnmodeledError {} + + struct CodedError { + code: &'static str, + } + + impl ProvideErrorKind for UnmodeledError { + fn retryable_error_kind(&self) -> Option { + None + } + + fn code(&self) -> Option<&str> { + None + } + } + + impl ProvideErrorKind for CodedError { + fn retryable_error_kind(&self) -> Option { + None + } + + fn code(&self) -> Option<&str> { + Some(self.code) + } + } + + fn make_err( + err: E, + raw: http::Response<&'static str>, + ) -> Result, SdkError> { + Err(SdkError::ServiceError { + err, + raw: operation::Response::new(raw.map(|b| SdkBody::from(b))), + }) + } + + #[test] + fn not_an_error() { + let policy = DefaultResponseRetryClassifier::new(); + let test_response = http::Response::new("OK"); + assert_eq!( + policy.classify_retry(make_err(UnmodeledError, test_response).as_ref()), + RetryKind::UnretryableFailure + ); + } + + #[test] + fn classify_by_response_status() { + let policy = DefaultResponseRetryClassifier::new(); + let test_resp = http::Response::builder() + .status(500) + .body("error!") + .unwrap(); + assert_eq!( + policy.classify_retry(make_err(UnmodeledError, test_resp).as_ref()), + RetryKind::Error(ErrorKind::TransientError) + ); + } + + #[test] + fn classify_by_response_status_not_retryable() { + let policy = DefaultResponseRetryClassifier::new(); + let test_resp = http::Response::builder() + .status(408) + .body("error!") + .unwrap(); + assert_eq!( + policy.classify_retry(make_err(UnmodeledError, test_resp).as_ref()), + RetryKind::UnretryableFailure + ); + } + + #[test] + fn classify_by_error_kind() { + struct ModeledRetries; + let test_response = http::Response::new("OK"); + impl ProvideErrorKind for ModeledRetries { + fn retryable_error_kind(&self) -> Option { + Some(ErrorKind::ClientError) + } + + fn code(&self) -> Option<&str> { + // code should not be called when `error_kind` is provided + unimplemented!() + } + } + + let policy = DefaultResponseRetryClassifier::new(); + + assert_eq!( + policy.classify_retry(make_err(ModeledRetries, test_response).as_ref()), + RetryKind::Error(ErrorKind::ClientError) + ); + } + + #[test] + fn classify_response_error() { + let policy = DefaultResponseRetryClassifier::new(); + assert_eq!( + policy.classify_retry( + Result::, SdkError>::Err(SdkError::ResponseError { + err: Box::new(UnmodeledError), + raw: operation::Response::new( + http::Response::new("OK").map(|b| SdkBody::from(b)) + ), + }) + .as_ref() + ), + RetryKind::Error(ErrorKind::TransientError) + ); + } + + #[test] + fn test_timeout_error() { + let policy = DefaultResponseRetryClassifier::new(); + let err: Result<(), SdkError> = Err(SdkError::TimeoutError("blah".into())); + assert_eq!( + policy.classify_retry(err.as_ref()), + RetryKind::Error(ErrorKind::TransientError) + ); } }