From 27a563e531743d088f9520dff0a81caf534100d2 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 7 Sep 2022 14:55:24 -0700 Subject: [PATCH 01/11] Fix retry for native Smithy clients --- aws/rust-runtime/aws-http/src/retry.rs | 74 +++----- .../tests/middleware_e2e_test.rs | 6 +- .../rustsdk/AwsFluentClientDecorator.kt | 2 +- .../smithy/rustsdk/RetryPolicyDecorator.kt | 2 +- .../dynamodb/tests/movies.rs | 4 +- .../kms/tests/sensitive-it.rs | 4 +- .../client/FluentClientGenerator.kt | 3 +- .../protocol/MakeOperationGenerator.kt | 5 +- .../aws-smithy-client/src/static_tests.rs | 3 +- rust-runtime/aws-smithy-http-tower/src/lib.rs | 6 +- rust-runtime/aws-smithy-http/src/operation.rs | 5 +- rust-runtime/aws-smithy-http/src/retry.rs | 176 +++++++++++++++++- 12 files changed, 226 insertions(+), 64 deletions(-) diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 748d958415..71131a11d7 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::{ClassifyResponse, DefaultResponseClassifier}; 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,38 @@ 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 [`ClassifyResponse`] that models 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 AwsResponseClassifier; + +impl AwsResponseClassifier { + /// Create an `AwsResponseClassifier` with the default set of known error & status codes pub fn new() -> Self { - AwsErrorRetryPolicy + AwsResponseClassifier } } -impl Default for AwsErrorRetryPolicy { +impl Default for AwsResponseClassifier { fn default() -> Self { Self::new() } } -impl ClassifyResponse> for AwsErrorRetryPolicy +impl ClassifyResponse> for AwsResponseClassifier 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(&self, result: Result<&T, &SdkError>) -> RetryKind { + let (err, response) = match DefaultResponseClassifier::try_extract_err_response(result) { + Ok(extracted) => extracted, + Err(retry_kind) => return retry_kind, }; if let Some(retry_after_delay) = response .http() @@ -105,7 +91,7 @@ where #[cfg(test)] mod test { - use crate::retry::AwsErrorRetryPolicy; + use crate::retry::AwsResponseClassifier; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation; use aws_smithy_http::result::{SdkError, SdkSuccess}; @@ -151,7 +137,7 @@ mod test { #[test] fn not_an_error() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); let test_response = http::Response::new("OK"); assert_eq!( policy.classify(make_err(UnmodeledError, test_response).as_ref()), @@ -161,7 +147,7 @@ mod test { #[test] fn classify_by_response_status() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); let test_resp = http::Response::builder() .status(500) .body("error!") @@ -174,7 +160,7 @@ mod test { #[test] fn classify_by_response_status_not_retryable() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); let test_resp = http::Response::builder() .status(408) .body("error!") @@ -188,7 +174,7 @@ mod test { #[test] fn classify_by_error_code() { let test_response = http::Response::new("OK"); - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); assert_eq!( policy.classify(make_err(CodedError { code: "Throttling" }, test_response).as_ref()), @@ -214,7 +200,7 @@ 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 = AwsResponseClassifier::new(); assert_eq!( policy.classify(make_err(err, test_response).as_ref()), RetryKind::Error(ErrorKind::ThrottlingError) @@ -236,7 +222,7 @@ mod test { } } - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); assert_eq!( policy.classify(make_err(ModeledRetries, test_response).as_ref()), @@ -246,7 +232,7 @@ mod test { #[test] fn test_retry_after_header() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); let test_response = http::Response::builder() .header("x-amz-retry-after", "5000") .body("retry later") @@ -260,7 +246,7 @@ mod test { #[test] fn test_timeout_error() { - let policy = AwsErrorRetryPolicy::new(); + let policy = AwsResponseClassifier::new(); let err: Result<(), SdkError> = Err(SdkError::TimeoutError("blah".into())); assert_eq!( policy.classify(err.as_ref()), 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..a1680e5e79 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::AwsResponseClassifier; 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,7 @@ fn test_operation() -> Operation { Result::<_, Infallible>::Ok(req) }) .unwrap(); - Operation::new(req, TestOperationParser).with_retry_policy(AwsErrorRetryPolicy::new()) + Operation::new(req, TestOperationParser).with_retry_policy(AwsResponseClassifier::new()) } #[cfg(any(feature = "native-tls", feature = "rustls"))] 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 768387569b..a5f2a511fa 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 @@ -98,7 +98,7 @@ class AwsFluentClientDecorator : RustCodegenDecorator { AwsPresignedFluentBuilderMethod(runtimeConfig), AwsFluentClientDocs(codegenContext), ), - retryPolicy = runtimeConfig.awsHttp().asType().member("retry::AwsErrorRetryPolicy").writable, + retryPolicy = runtimeConfig.awsHttp().asType().member("retry::AwsResponseClassifier").writable, ).render(rustCrate) rustCrate.withModule(FluentClientGenerator.customizableOperationModule) { writer -> renderCustomizableOperationSendMethod(runtimeConfig, generics, writer) 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/RetryPolicyDecorator.kt index 5daee32e56..e4d89952a9 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/RetryPolicyDecorator.kt @@ -34,7 +34,7 @@ class RetryPolicyDecorator : RustCodegenDecorator { } class RetryPolicyFeature(private val runtimeConfig: RuntimeConfig) : OperationCustomization() { - override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsErrorRetryPolicy") + override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsResponseClassifier") override fun section(section: OperationSection) = when (section) { is OperationSection.FinalizeOperation -> writable { rust( diff --git a/aws/sdk/integration-tests/dynamodb/tests/movies.rs b/aws/sdk/integration-tests/dynamodb/tests/movies.rs index c3a0f079ad..cd653cc919 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/movies.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/movies.rs @@ -5,7 +5,7 @@ use aws_sdk_dynamodb as dynamodb; -use aws_http::retry::AwsErrorRetryPolicy; +use aws_http::retry::AwsResponseClassifier; use aws_sdk_dynamodb::input::CreateTableInput; use aws_smithy_client::test_connection::TestConnection; use aws_smithy_http::body::SdkBody; @@ -155,7 +155,7 @@ where async fn wait_for_ready_table( table_name: &str, conf: &Config, -) -> Operation> { +) -> Operation> { let operation = DescribeTableInput::builder() .table_name(table_name) .build() diff --git a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs index ec49cdd08b..92a0a9f3f7 100644 --- a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs +++ b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -use aws_http::retry::AwsErrorRetryPolicy; +use aws_http::retry::AwsResponseClassifier; use aws_sdk_kms as kms; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation::{self, Parts}; @@ -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() diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt index 34e72c8785..381abb077f 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt @@ -67,7 +67,8 @@ class FluentClientGenerator( client = CargoDependency.SmithyClient(codegenContext.runtimeConfig).asType(), ), private val customizations: List = emptyList(), - private val retryPolicy: Writable = RustType.Unit.writable, + private val retryPolicy: Writable = CargoDependency.SmithyHttp(codegenContext.runtimeConfig).asType() + .member("retry::DefaultResponseClassifier").writable, ) { companion object { fun clientOperationFnName(operationShape: OperationShape, symbolProvider: RustSymbolProvider): String = diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/protocol/MakeOperationGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/protocol/MakeOperationGenerator.kt index aa975c0307..ed200d159e 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/protocol/MakeOperationGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/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.rustlang.Attribute import software.amazon.smithy.rust.codegen.rustlang.CargoDependency -import software.amazon.smithy.rust.codegen.rustlang.RustType import software.amazon.smithy.rust.codegen.rustlang.RustWriter import software.amazon.smithy.rust.codegen.rustlang.asType import software.amazon.smithy.rust.codegen.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::DefaultResponseClassifier") 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/rust-runtime/aws-smithy-client/src/static_tests.rs b/rust-runtime/aws-smithy-client/src/static_tests.rs index f9835135a3..02ed86dcee 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::DefaultResponseClassifier; #[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-http-tower/src/lib.rs b/rust-runtime/aws-smithy-http-tower/src/lib.rs index eefe1bb613..9aded450c6 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::DefaultResponseClassifier; 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, + DefaultResponseClassifier, + >::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..925bc91535 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::DefaultResponseClassifier; use aws_smithy_types::date_time::DateTimeFormatError; use http::uri::InvalidUri; use std::borrow::Cow; @@ -232,12 +233,12 @@ 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_policy: DefaultResponseClassifier::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..ea363930e8 100644 --- a/rust-runtime/aws-smithy-http/src/retry.rs +++ b/rust-runtime/aws-smithy-http/src/retry.rs @@ -7,14 +7,182 @@ //! //! 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}; +/// Classifies what kind of retry is needed for a given `response`. pub trait ClassifyResponse: Clone { fn classify(&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 [`ClassifyResponse`] for generated clients. +#[derive(Clone, Debug, Default)] +pub struct DefaultResponseClassifier; + +impl DefaultResponseClassifier { + /// Creates a new `DefaultResponseClassifier` + 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. + #[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(_) => Err(RetryKind::UnretryableFailure), + } + } +} + +impl ClassifyResponse> for DefaultResponseClassifier +where + E: ProvideErrorKind, +{ + fn classify(&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::ClassifyResponse; + use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; + + struct 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 = DefaultResponseClassifier::new(); + let test_response = http::Response::new("OK"); + assert_eq!( + policy.classify(make_err(UnmodeledError, test_response).as_ref()), + RetryKind::UnretryableFailure + ); + } + + #[test] + fn classify_by_response_status() { + let policy = DefaultResponseClassifier::new(); + let test_resp = http::Response::builder() + .status(500) + .body("error!") + .unwrap(); + assert_eq!( + policy.classify(make_err(UnmodeledError, test_resp).as_ref()), + RetryKind::Error(ErrorKind::TransientError) + ); + } + + #[test] + fn classify_by_response_status_not_retryable() { + let policy = DefaultResponseClassifier::new(); + let test_resp = http::Response::builder() + .status(408) + .body("error!") + .unwrap(); + assert_eq!( + policy.classify(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 = DefaultResponseClassifier::new(); + + assert_eq!( + policy.classify(make_err(ModeledRetries, test_response).as_ref()), + RetryKind::Error(ErrorKind::ClientError) + ); + } + + #[test] + fn test_timeout_error() { + let policy = DefaultResponseClassifier::new(); + let err: Result<(), SdkError> = Err(SdkError::TimeoutError("blah".into())); + assert_eq!( + policy.classify(err.as_ref()), + RetryKind::Error(ErrorKind::TransientError) + ); } } From 323758b3add120a1a0f87ada3b91698e78df9546 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 7 Sep 2022 15:59:25 -0700 Subject: [PATCH 02/11] Treat `SdkError::ResponseError` as a retryable transient failure --- aws/rust-runtime/aws-http/src/retry.rs | 25 +++++++++++++++++++ rust-runtime/aws-smithy-http/src/retry.rs | 30 ++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 71131a11d7..11333395e6 100644 --- a/aws/rust-runtime/aws-http/src/retry.rs +++ b/aws/rust-runtime/aws-http/src/retry.rs @@ -97,9 +97,17 @@ mod test { use aws_smithy_http::result::{SdkError, SdkSuccess}; use aws_smithy_http::retry::ClassifyResponse; 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, @@ -244,6 +252,23 @@ mod test { ); } + #[test] + fn classify_response_error() { + let policy = AwsResponseClassifier::new(); + assert_eq!( + policy.classify( + 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 = AwsResponseClassifier::new(); diff --git a/rust-runtime/aws-smithy-http/src/retry.rs b/rust-runtime/aws-smithy-http/src/retry.rs index ea363930e8..cc4f625bdb 100644 --- a/rust-runtime/aws-smithy-http/src/retry.rs +++ b/rust-runtime/aws-smithy-http/src/retry.rs @@ -31,6 +31,8 @@ impl DefaultResponseClassifier { /// 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>, @@ -48,7 +50,8 @@ impl DefaultResponseClassifier { Err(RetryKind::UnretryableFailure) } } - Err(_) => Err(RetryKind::UnretryableFailure), + Err(SdkError::ResponseError { .. }) => Err(RetryKind::Error(ErrorKind::TransientError)), + Err(SdkError::ConstructionFailure(_)) => Err(RetryKind::UnretryableFailure), } } } @@ -80,8 +83,16 @@ mod test { use crate::result::{SdkError, SdkSuccess}; use crate::retry::ClassifyResponse; 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, @@ -176,6 +187,23 @@ mod test { ); } + #[test] + fn classify_response_error() { + let policy = DefaultResponseClassifier::new(); + assert_eq!( + policy.classify( + 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 = DefaultResponseClassifier::new(); From b9b778a65f9c7419e6958c6db77e9d5e42f57cef Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Wed, 7 Sep 2022 16:16:00 -0700 Subject: [PATCH 03/11] Update changelog --- CHANGELOG.next.toml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index e5dcda5393..086dee4a5e 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -186,3 +186,21 @@ message = "Smithy IDL v2 mixins are now supported" references = ["smithy-rs#1680"] meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "all"} author = "ogudavid" + +[[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` 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" + +[[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" From a001b02710beb765b4b3e1850d076512dfc508a4 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 9 Sep 2022 14:14:46 -0700 Subject: [PATCH 04/11] Rename `ClassifyResponse` to `ClassifyRetry` --- CHANGELOG.next.toml | 2 +- .../src/http_credential_provider.rs | 14 +++---- .../aws-config/src/imds/client.rs | 12 +++--- aws/rust-runtime/aws-http/src/retry.rs | 35 +++++++++------- .../rustsdk/AwsFluentClientDecorator.kt | 4 +- .../dynamodb/tests/movies.rs | 8 ++-- .../kms/tests/sensitive-it.rs | 2 +- .../client/FluentClientGenerator.kt | 2 +- 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 | 6 +-- .../aws-smithy-client/src/static_tests.rs | 4 +- .../aws-smithy-client/tests/e2e_test.rs | 6 +-- rust-runtime/aws-smithy-http-tower/src/lib.rs | 4 +- rust-runtime/aws-smithy-http/src/operation.rs | 9 ++-- rust-runtime/aws-smithy-http/src/retry.rs | 42 +++++++++---------- 16 files changed, 81 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 086dee4a5e..74af848717 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -194,7 +194,7 @@ meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client" } author = "jdisanti" [[smithy-rs]] -message = "`ClassifyResponse` is no longer implemented for the unit type." +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" 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..8f6c7374ba 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; @@ -167,10 +167,10 @@ impl ParseStrictResponse for CredentialsResponseParser { #[derive(Clone, Debug)] struct HttpCredentialRetryPolicy; -impl ClassifyResponse, SdkError> +impl ClassifyRetry, SdkError> for HttpCredentialRetryPolicy { - fn classify( + fn classify_retry( &self, response: Result<&SdkSuccess, &SdkError>, ) -> RetryKind { @@ -211,7 +211,7 @@ mod test { 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 +245,7 @@ mod test { .unwrap(); assert_eq!( - HttpCredentialRetryPolicy.classify(sdk_resp(bad_response).as_ref()), + HttpCredentialRetryPolicy.classify_retry(sdk_resp(bad_response).as_ref()), RetryKind::Error(ErrorKind::ServerError) ); } @@ -266,7 +266,7 @@ mod test { let sdk_result = sdk_resp(ok_response); assert_eq!( - HttpCredentialRetryPolicy.classify(sdk_result.as_ref()), + HttpCredentialRetryPolicy.classify_retry(sdk_result.as_ref()), RetryKind::Unnecessary ); @@ -281,7 +281,7 @@ mod test { .unwrap(); let sdk_result = sdk_resp(error_response); assert_eq!( - HttpCredentialRetryPolicy.classify(sdk_result.as_ref()), + HttpCredentialRetryPolicy.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 b1b9c7ab44..6ba12667be 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, }; @@ -730,8 +730,8 @@ 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 ImdsErrorPolicy { + fn classify_retry(&self, response: Result<&SdkSuccess, &SdkError>) -> RetryKind { match response { Ok(_) => RetryKind::Unnecessary, Err(SdkError::ResponseError { raw, .. }) | Err(SdkError::ServiceError { raw, .. }) => { @@ -1006,7 +1006,7 @@ 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; fn response_200() -> operation::Response { @@ -1018,7 +1018,7 @@ pub(crate) mod test { }; assert_eq!( RetryKind::Unnecessary, - policy.classify(Ok::<_, &SdkError<()>>(&success)) + policy.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) @@ -1028,7 +1028,7 @@ pub(crate) mod test { }; assert_eq!( RetryKind::UnretryableFailure, - policy.classify(Err::<&SdkSuccess<()>, _>(&failure)) + policy.classify_retry(Err::<&SdkSuccess<()>, _>(&failure)) ); } diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 11333395e6..72f431d3b4 100644 --- a/aws/rust-runtime/aws-http/src/retry.rs +++ b/aws/rust-runtime/aws-http/src/retry.rs @@ -5,7 +5,7 @@ //! AWS-specific retry logic use aws_smithy_http::result::SdkError; -use aws_smithy_http::retry::{ClassifyResponse, DefaultResponseClassifier}; +use aws_smithy_http::retry::{ClassifyRetry, DefaultResponseRetryClassifier}; use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; use std::time::Duration; @@ -28,7 +28,7 @@ const THROTTLING_ERRORS: &[&str] = &[ ]; const TRANSIENT_ERRORS: &[&str] = &["RequestTimeout", "RequestTimeoutException"]; -/// Implementation of [`ClassifyResponse`] that models AWS error codes. +/// Implementation of [`ClassifyRetry`] that models AWS error codes. /// /// In order of priority: /// 1. The `x-amz-retry-after` header is checked @@ -52,12 +52,13 @@ impl Default for AwsResponseClassifier { } } -impl ClassifyResponse> for AwsResponseClassifier +impl ClassifyRetry> for AwsResponseClassifier where E: ProvideErrorKind, { - fn classify(&self, result: Result<&T, &SdkError>) -> RetryKind { - let (err, response) = match DefaultResponseClassifier::try_extract_err_response(result) { + fn classify_retry(&self, result: Result<&T, &SdkError>) -> RetryKind { + let (err, response) = match DefaultResponseRetryClassifier::try_extract_err_response(result) + { Ok(extracted) => extracted, Err(retry_kind) => return retry_kind, }; @@ -95,7 +96,7 @@ mod test { 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; @@ -148,7 +149,7 @@ mod test { let policy = AwsResponseClassifier::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 ); } @@ -161,7 +162,7 @@ mod test { .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) ); } @@ -174,7 +175,7 @@ mod test { .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 ); } @@ -185,13 +186,15 @@ mod test { let policy = AwsResponseClassifier::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" @@ -210,7 +213,7 @@ mod test { let test_response = http::Response::new("OK"); let policy = AwsResponseClassifier::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) ); } @@ -233,7 +236,7 @@ mod test { let policy = AwsResponseClassifier::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) ); } @@ -247,7 +250,7 @@ mod test { .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)) ); } @@ -256,7 +259,7 @@ mod test { fn classify_response_error() { let policy = AwsResponseClassifier::new(); assert_eq!( - policy.classify( + policy.classify_retry( Result::, SdkError>::Err(SdkError::ResponseError { err: Box::new(UnmodeledError), raw: operation::Response::new( @@ -274,7 +277,7 @@ mod test { let policy = AwsResponseClassifier::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/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 a5f2a511fa..5c9ebba22a 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 @@ -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/integration-tests/dynamodb/tests/movies.rs b/aws/sdk/integration-tests/dynamodb/tests/movies.rs index cd653cc919..7f25d88de9 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/movies.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/movies.rs @@ -11,7 +11,7 @@ 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_http::retry::ClassifyRetry; use aws_smithy_types::retry::RetryKind; use dynamodb::error::DescribeTableError; use dynamodb::input::{DescribeTableInput, PutItemInput, QueryInput}; @@ -116,12 +116,12 @@ struct WaitForReadyTable { inner: R, } -impl ClassifyResponse, SdkError> +impl ClassifyRetry, SdkError> for WaitForReadyTable where - R: ClassifyResponse, SdkError>, + R: ClassifyRetry, SdkError>, { - fn classify( + fn classify_retry( &self, response: Result<&SdkSuccess, &SdkError>, ) -> RetryKind { diff --git a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs index 92a0a9f3f7..3985db4ac3 100644 --- a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs +++ b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs @@ -9,7 +9,7 @@ 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; diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt index 381abb077f..4d82c6a6ad 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt @@ -318,7 +318,7 @@ 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"), 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..0b6797ff31 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; @@ -374,7 +374,7 @@ where result: Result<&SdkSuccess, &SdkError>, ) -> Option { let policy = req.retry_policy(); - let retry_kind = policy.classify(result); + let retry_kind = policy.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 02ed86dcee..4c9ef91bad 100644 --- a/rust-runtime/aws-smithy-client/src/static_tests.rs +++ b/rust-runtime/aws-smithy-client/src/static_tests.rs @@ -7,7 +7,7 @@ use crate::{Builder, Error, Operation, ParseHttpResponse, ProvideErrorKind}; use aws_smithy_http::operation; -use aws_smithy_http::retry::DefaultResponseClassifier; +use aws_smithy_http::retry::DefaultResponseRetryClassifier; #[derive(Debug)] #[non_exhaustive] @@ -41,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..095c423e0d 100644 --- a/rust-runtime/aws-smithy-client/tests/e2e_test.rs +++ b/rust-runtime/aws-smithy-client/tests/e2e_test.rs @@ -20,7 +20,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; @@ -69,12 +69,12 @@ mod test_operation { #[derive(Clone)] pub(super) struct TestPolicy; - impl ClassifyResponse> for TestPolicy + impl ClassifyRetry> for TestPolicy 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, diff --git a/rust-runtime/aws-smithy-http-tower/src/lib.rs b/rust-runtime/aws-smithy-http-tower/src/lib.rs index 9aded450c6..e79e48d728 100644 --- a/rust-runtime/aws-smithy-http-tower/src/lib.rs +++ b/rust-runtime/aws-smithy-http-tower/src/lib.rs @@ -62,7 +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::DefaultResponseClassifier; + use aws_smithy_http::retry::DefaultResponseRetryClassifier; use bytes::Bytes; use http::Response; use std::convert::{Infallible, TryInto}; @@ -105,7 +105,7 @@ mod tests { let mut svc = ServiceBuilder::new() .layer(ParseResponseLayer::< TestParseResponse, - DefaultResponseClassifier, + DefaultResponseRetryClassifier, >::new()) .layer(MapRequestLayer::for_mapper(AddHeader)) .layer(DispatchLayer) diff --git a/rust-runtime/aws-smithy-http/src/operation.rs b/rust-runtime/aws-smithy-http/src/operation.rs index 925bc91535..76dc5bd5a5 100644 --- a/rust-runtime/aws-smithy-http/src/operation.rs +++ b/rust-runtime/aws-smithy-http/src/operation.rs @@ -5,7 +5,7 @@ use crate::body::SdkBody; use crate::property_bag::{PropertyBag, SharedPropertyBag}; -use crate::retry::DefaultResponseClassifier; +use crate::retry::DefaultResponseRetryClassifier; use aws_smithy_types::date_time::DateTimeFormatError; use http::uri::InvalidUri; use std::borrow::Cow; @@ -233,12 +233,15 @@ impl Operation { } impl Operation { - pub fn new(request: Request, response_handler: H) -> Operation { + pub fn new( + request: Request, + response_handler: H, + ) -> Operation { Operation { request, parts: Parts { response_handler, - retry_policy: DefaultResponseClassifier::new(), + retry_policy: 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 cc4f625bdb..b562b66290 100644 --- a/rust-runtime/aws-smithy-http/src/retry.rs +++ b/rust-runtime/aws-smithy-http/src/retry.rs @@ -12,18 +12,18 @@ use crate::result::SdkError; use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; /// Classifies what kind of retry is needed for a given `response`. -pub trait ClassifyResponse: Clone { - fn classify(&self, response: Result<&T, &E>) -> RetryKind; +pub trait ClassifyRetry: Clone { + fn classify_retry(&self, response: Result<&T, &E>) -> RetryKind; } const TRANSIENT_ERROR_STATUS_CODES: &[u16] = &[500, 502, 503, 504]; -/// The default implementation of [`ClassifyResponse`] for generated clients. +/// The default implementation of [`ClassifyRetry`] for generated clients. #[derive(Clone, Debug, Default)] -pub struct DefaultResponseClassifier; +pub struct DefaultResponseRetryClassifier; -impl DefaultResponseClassifier { - /// Creates a new `DefaultResponseClassifier` +impl DefaultResponseRetryClassifier { + /// Creates a new `DefaultResponseRetryClassifier` pub fn new() -> Self { Default::default() } @@ -56,11 +56,11 @@ impl DefaultResponseClassifier { } } -impl ClassifyResponse> for DefaultResponseClassifier +impl ClassifyRetry> for DefaultResponseRetryClassifier where E: ProvideErrorKind, { - fn classify(&self, result: Result<&T, &SdkError>) -> RetryKind { + 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, @@ -81,7 +81,7 @@ mod test { use crate::body::SdkBody; use crate::operation; use crate::result::{SdkError, SdkSuccess}; - use crate::retry::ClassifyResponse; + use crate::retry::ClassifyRetry; use aws_smithy_types::retry::{ErrorKind, ProvideErrorKind, RetryKind}; use std::fmt; @@ -130,36 +130,36 @@ mod test { #[test] fn not_an_error() { - let policy = DefaultResponseClassifier::new(); + let policy = DefaultResponseRetryClassifier::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 = DefaultResponseClassifier::new(); + let policy = DefaultResponseRetryClassifier::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 = DefaultResponseClassifier::new(); + let policy = DefaultResponseRetryClassifier::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 ); } @@ -179,19 +179,19 @@ mod test { } } - let policy = DefaultResponseClassifier::new(); + let policy = DefaultResponseRetryClassifier::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 classify_response_error() { - let policy = DefaultResponseClassifier::new(); + let policy = DefaultResponseRetryClassifier::new(); assert_eq!( - policy.classify( + policy.classify_retry( Result::, SdkError>::Err(SdkError::ResponseError { err: Box::new(UnmodeledError), raw: operation::Response::new( @@ -206,10 +206,10 @@ mod test { #[test] fn test_timeout_error() { - let policy = DefaultResponseClassifier::new(); + let policy = DefaultResponseRetryClassifier::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) ); } From ff771d54346fc42c02587c274264c6f6c60dd638 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 9 Sep 2022 14:40:09 -0700 Subject: [PATCH 05/11] Rename 'policy' to 'classifier' in AWS SDK public API --- CHANGELOG.next.toml | 26 +++++++++++++++++++ .../src/http_credential_provider.rs | 18 +++++++------ .../aws-config/src/imds/client.rs | 22 ++++++++-------- .../aws-config/src/imds/client/token.rs | 4 +-- .../tests/middleware_e2e_test.rs | 2 +- .../smithy/rustsdk/AwsCodegenDecorator.kt | 2 +- ...corator.kt => RetryClassifierDecorator.kt} | 8 +++--- .../dynamodb/tests/movies.rs | 4 +-- design/src/rfcs/rfc0010_waiters.md | 5 ++-- rust-runtime/aws-smithy-client/src/retry.rs | 4 +-- .../aws-smithy-client/tests/e2e_test.rs | 11 ++++---- rust-runtime/aws-smithy-http/src/operation.rs | 15 ++++++----- 12 files changed, 75 insertions(+), 46 deletions(-) rename aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/{RetryPolicyDecorator.kt => RetryClassifierDecorator.kt} (85%) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 74af848717..3b96a2db58 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -199,8 +199,34 @@ 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 8f6c7374ba..2e6d6e137f 100644 --- a/aws/rust-runtime/aws-config/src/http_credential_provider.rs +++ b/aws/rust-runtime/aws-config/src/http_credential_provider.rs @@ -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,10 +165,10 @@ impl ParseStrictResponse for CredentialsResponseParser { } #[derive(Clone, Debug)] -struct HttpCredentialRetryPolicy; +struct HttpCredentialRetryClassifier; impl ClassifyRetry, SdkError> - for HttpCredentialRetryPolicy + for HttpCredentialRetryClassifier { fn classify_retry( &self, @@ -206,7 +206,9 @@ impl ClassifyRetry, 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; @@ -245,7 +247,7 @@ mod test { .unwrap(); assert_eq!( - HttpCredentialRetryPolicy.classify_retry(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_retry(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_retry(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 6ba12667be..dcfa03a774 100644 --- a/aws/rust-runtime/aws-config/src/imds/client.rs +++ b/aws/rust-runtime/aws-config/src/imds/client.rs @@ -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 ClassifyRetry, SdkError> for ImdsErrorPolicy { +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 ClassifyRetry, 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; @@ -1008,7 +1008,7 @@ pub(crate) mod test { fn successful_response_properly_classified() { 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())) } @@ -1018,7 +1018,7 @@ pub(crate) mod test { }; assert_eq!( RetryKind::Unnecessary, - policy.classify_retry(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) @@ -1028,7 +1028,7 @@ pub(crate) mod test { }; assert_eq!( RetryKind::UnretryableFailure, - policy.classify_retry(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-inlineable/tests/middleware_e2e_test.rs b/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs index a1680e5e79..30e46e8b11 100644 --- a/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs +++ b/aws/rust-runtime/aws-inlineable/tests/middleware_e2e_test.rs @@ -110,7 +110,7 @@ fn test_operation() -> Operation { Result::<_, Infallible>::Ok(req) }) .unwrap(); - Operation::new(req, TestOperationParser).with_retry_policy(AwsResponseClassifier::new()) + Operation::new(req, TestOperationParser).with_retry_classifier(AwsResponseClassifier::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 219aac7d4a..70742c8b53 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 @@ -25,7 +25,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/RetryPolicyDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt similarity index 85% 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 e4d89952a9..1cbe66ad9f 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.smithy.customize.OperationCustomizati import software.amazon.smithy.rust.codegen.smithy.customize.OperationSection import software.amazon.smithy.rust.codegen.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() { +class RetryClassifierFeature(private val runtimeConfig: RuntimeConfig) : OperationCustomization() { override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsResponseClassifier") 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 7f25d88de9..8fa6784f6e 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/movies.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/movies.rs @@ -163,10 +163,10 @@ async fn wait_for_ready_table( .make_operation(&conf) .await .expect("valid operation"); - let waiting_policy = WaitForReadyTable { + let waiting_classifier = WaitForReadyTable { inner: operation.retry_policy().clone(), }; - operation.with_retry_policy(waiting_policy) + operation.with_retry_classifier(waiting_classifier) } /// Validate that time has passed with a 5ms tolerance 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/retry.rs b/rust-runtime/aws-smithy-client/src/retry.rs index 0b6797ff31..d58194e70e 100644 --- a/rust-runtime/aws-smithy-client/src/retry.rs +++ b/rust-runtime/aws-smithy-client/src/retry.rs @@ -373,8 +373,8 @@ where req: &Operation, result: Result<&SdkSuccess, &SdkError>, ) -> Option { - let policy = req.retry_policy(); - let retry_kind = policy.classify_retry(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/tests/e2e_test.rs b/rust-runtime/aws-smithy-client/tests/e2e_test.rs index 095c423e0d..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; @@ -67,9 +66,9 @@ mod test_operation { } #[derive(Clone)] - pub(super) struct TestPolicy; + pub(super) struct TestRetryClassifier; - impl ClassifyRetry> for TestPolicy + impl ClassifyRetry> for TestRetryClassifier where E: ProvideErrorKind + Debug, T: Debug, @@ -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/src/operation.rs b/rust-runtime/aws-smithy-http/src/operation.rs index 76dc5bd5a5..7fc7ce5174 100644 --- a/rust-runtime/aws-smithy-http/src/operation.rs +++ b/rust-runtime/aws-smithy-http/src/operation.rs @@ -43,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, } @@ -167,6 +167,9 @@ impl From for SerializationError { } } +// Generics: +// - H: Response handler +// - R: Implementation of `ClassifyRetry` #[derive(Debug)] pub struct Operation { request: Request, @@ -204,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 @@ -241,7 +244,7 @@ impl Operation { request, parts: Parts { response_handler, - retry_policy: DefaultResponseRetryClassifier::new(), + retry_classifier: DefaultResponseRetryClassifier::new(), metadata: None, }, } From 01eae518818ad44187a2ab627dae4ed2e9d451ad Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 9 Sep 2022 14:47:09 -0700 Subject: [PATCH 06/11] Rename `AwsResponseClassifier` to `AwsResponseRetryClassifier` --- aws/rust-runtime/aws-http/src/retry.rs | 32 +++++++++---------- .../tests/middleware_e2e_test.rs | 7 ++-- .../rustsdk/AwsFluentClientDecorator.kt | 2 +- .../rustsdk/RetryClassifierDecorator.kt | 2 +- .../dynamodb/tests/movies.rs | 4 +-- .../kms/tests/sensitive-it.rs | 4 +-- 6 files changed, 26 insertions(+), 25 deletions(-) diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 72f431d3b4..6f8655bacc 100644 --- a/aws/rust-runtime/aws-http/src/retry.rs +++ b/aws/rust-runtime/aws-http/src/retry.rs @@ -37,22 +37,22 @@ const TRANSIENT_ERRORS: &[&str] = &["RequestTimeout", "RequestTimeoutException"] /// 4. The status code is checked against a predetermined list of status codes #[non_exhaustive] #[derive(Clone, Debug)] -pub struct AwsResponseClassifier; +pub struct AwsResponseRetryClassifier; -impl AwsResponseClassifier { - /// Create an `AwsResponseClassifier` with the default set of known error & status codes +impl AwsResponseRetryClassifier { + /// Create an `AwsResponseRetryClassifier` with the default set of known error & status codes pub fn new() -> Self { - AwsResponseClassifier + AwsResponseRetryClassifier } } -impl Default for AwsResponseClassifier { +impl Default for AwsResponseRetryClassifier { fn default() -> Self { Self::new() } } -impl ClassifyRetry> for AwsResponseClassifier +impl ClassifyRetry> for AwsResponseRetryClassifier where E: ProvideErrorKind, { @@ -92,7 +92,7 @@ where #[cfg(test)] mod test { - use crate::retry::AwsResponseClassifier; + use crate::retry::AwsResponseRetryClassifier; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation; use aws_smithy_http::result::{SdkError, SdkSuccess}; @@ -146,7 +146,7 @@ mod test { #[test] fn not_an_error() { - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); let test_response = http::Response::new("OK"); assert_eq!( policy.classify_retry(make_err(UnmodeledError, test_response).as_ref()), @@ -156,7 +156,7 @@ mod test { #[test] fn classify_by_response_status() { - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); let test_resp = http::Response::builder() .status(500) .body("error!") @@ -169,7 +169,7 @@ mod test { #[test] fn classify_by_response_status_not_retryable() { - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); let test_resp = http::Response::builder() .status(408) .body("error!") @@ -183,7 +183,7 @@ mod test { #[test] fn classify_by_error_code() { let test_response = http::Response::new("OK"); - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( policy.classify_retry( @@ -211,7 +211,7 @@ mod test { fn classify_generic() { let err = aws_smithy_types::Error::builder().code("SlowDown").build(); let test_response = http::Response::new("OK"); - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( policy.classify_retry(make_err(err, test_response).as_ref()), RetryKind::Error(ErrorKind::ThrottlingError) @@ -233,7 +233,7 @@ mod test { } } - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( policy.classify_retry(make_err(ModeledRetries, test_response).as_ref()), @@ -243,7 +243,7 @@ mod test { #[test] fn test_retry_after_header() { - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); let test_response = http::Response::builder() .header("x-amz-retry-after", "5000") .body("retry later") @@ -257,7 +257,7 @@ mod test { #[test] fn classify_response_error() { - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); assert_eq!( policy.classify_retry( Result::, SdkError>::Err(SdkError::ResponseError { @@ -274,7 +274,7 @@ mod test { #[test] fn test_timeout_error() { - let policy = AwsResponseClassifier::new(); + let policy = AwsResponseRetryClassifier::new(); let err: Result<(), SdkError> = Err(SdkError::TimeoutError("blah".into())); assert_eq!( policy.classify_retry(err.as_ref()), 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 30e46e8b11..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::AwsResponseClassifier; +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_classifier(AwsResponseClassifier::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/AwsFluentClientDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt index 5c9ebba22a..7be3b5134e 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 @@ -98,7 +98,7 @@ class AwsFluentClientDecorator : RustCodegenDecorator { AwsPresignedFluentBuilderMethod(runtimeConfig), AwsFluentClientDocs(codegenContext), ), - retryPolicy = runtimeConfig.awsHttp().asType().member("retry::AwsResponseClassifier").writable, + retryPolicy = runtimeConfig.awsHttp().asType().member("retry::AwsResponseRetryClassifier").writable, ).render(rustCrate) rustCrate.withModule(FluentClientGenerator.customizableOperationModule) { writer -> renderCustomizableOperationSendMethod(runtimeConfig, generics, writer) diff --git a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt index 1cbe66ad9f..b18b0fa415 100644 --- a/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt +++ b/aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/RetryClassifierDecorator.kt @@ -34,7 +34,7 @@ class RetryClassifierDecorator : RustCodegenDecorator { } class RetryClassifierFeature(private val runtimeConfig: RuntimeConfig) : OperationCustomization() { - override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsResponseClassifier") + override fun retryType(): RuntimeType = runtimeConfig.awsHttp().asType().member("retry::AwsResponseRetryClassifier") override fun section(section: OperationSection) = when (section) { is OperationSection.FinalizeOperation -> writable { rust( diff --git a/aws/sdk/integration-tests/dynamodb/tests/movies.rs b/aws/sdk/integration-tests/dynamodb/tests/movies.rs index 8fa6784f6e..047684bc18 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/movies.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/movies.rs @@ -5,7 +5,7 @@ use aws_sdk_dynamodb as dynamodb; -use aws_http::retry::AwsResponseClassifier; +use aws_http::retry::AwsResponseRetryClassifier; use aws_sdk_dynamodb::input::CreateTableInput; use aws_smithy_client::test_connection::TestConnection; use aws_smithy_http::body::SdkBody; @@ -155,7 +155,7 @@ where async fn wait_for_ready_table( table_name: &str, conf: &Config, -) -> Operation> { +) -> Operation> { let operation = DescribeTableInput::builder() .table_name(table_name) .build() diff --git a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs index 3985db4ac3..55e1440a07 100644 --- a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs +++ b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -use aws_http::retry::AwsResponseClassifier; +use aws_http::retry::AwsResponseRetryClassifier; use aws_sdk_kms as kms; use aws_smithy_http::body::SdkBody; use aws_smithy_http::operation::{self, Parts}; @@ -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() From c54cfc1948ad22eda64aa57ea9492640b44e0537 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 9 Sep 2022 15:05:17 -0700 Subject: [PATCH 07/11] Incorporate feedback --- aws/rust-runtime/aws-http/src/retry.rs | 5 ++++- .../amazon/smithy/rustsdk/AwsFluentClientDecorator.kt | 4 ++-- .../codegen/smithy/generators/PaginatorGenerator.kt | 8 ++++---- .../smithy/generators/client/FluentClientGenerator.kt | 11 +++++------ .../smithy/generators/client/FluentClientGenerics.kt | 8 ++++---- 5 files changed, 19 insertions(+), 17 deletions(-) diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 6f8655bacc..7d19b06800 100644 --- a/aws/rust-runtime/aws-http/src/retry.rs +++ b/aws/rust-runtime/aws-http/src/retry.rs @@ -42,7 +42,7 @@ pub struct AwsResponseRetryClassifier; impl AwsResponseRetryClassifier { /// Create an `AwsResponseRetryClassifier` with the default set of known error & status codes pub fn new() -> Self { - AwsResponseRetryClassifier + Self } } @@ -57,6 +57,9 @@ where E: ProvideErrorKind, { 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, 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 7be3b5134e..6f74895428 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::AwsResponseRetryClassifier").writable, + retryClassifier = runtimeConfig.awsHttp().asType().member("retry::AwsResponseRetryClassifier"), ).render(rustCrate) rustCrate.withModule(FluentClientGenerator.customizableOperationModule) { writer -> renderCustomizableOperationSendMethod(runtimeConfig, generics, writer) diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt index 727c803eb0..bdd0614f0f 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/PaginatorGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/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/smithy/generators/client/FluentClientGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt index 4d82c6a6ad..020aead71c 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerator.kt @@ -17,7 +17,6 @@ import software.amazon.smithy.rust.codegen.rustlang.RustModule import software.amazon.smithy.rust.codegen.rustlang.RustReservedWords import software.amazon.smithy.rust.codegen.rustlang.RustType import software.amazon.smithy.rust.codegen.rustlang.RustWriter -import software.amazon.smithy.rust.codegen.rustlang.Writable import software.amazon.smithy.rust.codegen.rustlang.asArgumentType import software.amazon.smithy.rust.codegen.rustlang.asOptional import software.amazon.smithy.rust.codegen.rustlang.asType @@ -67,8 +66,8 @@ class FluentClientGenerator( client = CargoDependency.SmithyClient(codegenContext.runtimeConfig).asType(), ), private val customizations: List = emptyList(), - private val retryPolicy: Writable = CargoDependency.SmithyHttp(codegenContext.runtimeConfig).asType() - .member("retry::DefaultResponseClassifier").writable, + private val retryClassifier: RuntimeType = CargoDependency.SmithyHttp(codegenContext.runtimeConfig).asType() + .member("retry::DefaultResponseClassifier"), ) { companion object { fun clientOperationFnName(operationShape: OperationShape, symbolProvider: RustSymbolProvider): String = @@ -323,14 +322,14 @@ class FluentClientGenerator( "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/smithy/generators/client/FluentClientGenerics.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerics.kt index a0c6ffdeea..1286262bf5 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/client/FluentClientGenerics.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/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:W} > """, "client" to client, "Operation" to operation, "OperationOutput" to operationOutput, "OperationError" to operationError, - "RetryPolicy" to retryPolicy, + "RetryClassifier" to retryClassifier, ) } From 02db8d7659fb48be2c3801db49caff0b225c7f41 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 9 Sep 2022 16:08:27 -0700 Subject: [PATCH 08/11] Fix `codegen-client` tests --- .../client/smithy/generators/client/FluentClientGenerator.kt | 2 +- .../client/smithy/generators/client/FluentClientGenerics.kt | 2 +- .../client/smithy/generators/protocol/MakeOperationGenerator.kt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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 376a3167e3..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 @@ -67,7 +67,7 @@ class FluentClientGenerator( ), private val customizations: List = emptyList(), private val retryClassifier: RuntimeType = CargoDependency.SmithyHttp(codegenContext.runtimeConfig).asType() - .member("retry::DefaultResponseClassifier"), + .member("retry::DefaultResponseRetryClassifier"), ) { companion object { fun clientOperationFnName(operationShape: OperationShape, symbolProvider: RustSymbolProvider): String = 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 903d5211e7..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 @@ -78,7 +78,7 @@ data class FlexibleClientGenerics( #{Operation}, #{OperationOutput}, #{OperationError}, - #{RetryClassifier:W} + #{RetryClassifier} > """, "client" to client, 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 515f2e9ba0..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 @@ -48,7 +48,7 @@ open class MakeOperationGenerator( protected val symbolProvider = coreCodegenContext.symbolProvider protected val httpBindingResolver = protocol.httpBindingResolver private val defaultClassifier = CargoDependency.SmithyHttp(runtimeConfig) - .asType().member("retry::DefaultResponseClassifier") + .asType().member("retry::DefaultResponseRetryClassifier") private val sdkId = coreCodegenContext.serviceShape.getTrait()?.sdkId?.lowercase()?.replace(" ", "") From ab6f6cfe39cc71f2fab46507f58122bf889a313e Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Fri, 9 Sep 2022 17:18:53 -0700 Subject: [PATCH 09/11] Fix the SDK integration tests --- .../dynamodb/tests/movies.rs | 163 +++++------------- .../kms/tests/sensitive-it.rs | 4 +- 2 files changed, 43 insertions(+), 124 deletions(-) diff --git a/aws/sdk/integration-tests/dynamodb/tests/movies.rs b/aws/sdk/integration-tests/dynamodb/tests/movies.rs index 047684bc18..ea1f23f436 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::AwsResponseRetryClassifier; -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::ClassifyRetry; -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 ClassifyRetry, SdkError> - for WaitForReadyTable -where - R: ClassifyRetry, SdkError>, -{ - fn classify_retry( - &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_classifier = WaitForReadyTable { - inner: operation.retry_policy().clone(), - }; - operation.with_retry_classifier(waiting_classifier) -} - /// 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"); + let films_2222 = movies_in_year(&client, table_name, 2222).await; // this isn't back to the future, there are no movies from 2022 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 55e1440a07..610d40dfe2 100644 --- a/aws/sdk/integration-tests/kms/tests/sensitive-it.rs +++ b/aws/sdk/integration-tests/kms/tests/sensitive-it.rs @@ -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)); } From ab64eb41ecb8945173555ae75e05a3ef08944775 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Mon, 12 Sep 2022 15:58:09 -0700 Subject: [PATCH 10/11] Fix comment Co-authored-by: Zelda Hessler --- aws/sdk/integration-tests/dynamodb/tests/movies.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/sdk/integration-tests/dynamodb/tests/movies.rs b/aws/sdk/integration-tests/dynamodb/tests/movies.rs index ea1f23f436..c6cded6c0f 100644 --- a/aws/sdk/integration-tests/dynamodb/tests/movies.rs +++ b/aws/sdk/integration-tests/dynamodb/tests/movies.rs @@ -174,7 +174,7 @@ async fn movies_it() { add_item(&client, table_name, item.clone()).await; } let films_2222 = movies_in_year(&client, table_name, 2222).await; - // this isn't back to the future, there are no movies from 2022 + // this isn't "Back To The Future", there are no movies from 2222 assert_eq!(films_2222.count, 0); let films_2013 = movies_in_year(&client, table_name, 2013).await; From 9bf1fe2f82baa403f8fe7ac90301fe00915f9275 Mon Sep 17 00:00:00 2001 From: John DiSanti Date: Mon, 12 Sep 2022 16:03:46 -0700 Subject: [PATCH 11/11] Improve doc comment --- aws/rust-runtime/aws-http/src/retry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/rust-runtime/aws-http/src/retry.rs b/aws/rust-runtime/aws-http/src/retry.rs index 7d19b06800..4ee9a3c792 100644 --- a/aws/rust-runtime/aws-http/src/retry.rs +++ b/aws/rust-runtime/aws-http/src/retry.rs @@ -28,7 +28,7 @@ const THROTTLING_ERRORS: &[&str] = &[ ]; const TRANSIENT_ERRORS: &[&str] = &["RequestTimeout", "RequestTimeoutException"]; -/// Implementation of [`ClassifyRetry`] that models AWS error codes. +/// Implementation of [`ClassifyRetry`] that classifies AWS error codes. /// /// In order of priority: /// 1. The `x-amz-retry-after` header is checked