-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix native Smithy client retry and retry response errors #1717
Changes from 3 commits
27a563e
323758b
b9b778a
a001b02
ff771d5
01eae51
c54cfc1
92cebfe
02db8d7
ab6f6cf
ab64eb4
9bf1fe2
c1e5350
5ae896e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be cool if we could link to code or docs showing what errors get retried There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is a way to link to the private constants with rustdoc, then I could add this pretty easily. I'm reluctant to duplicate the list in the documentation since it will get out of date. |
||
pub fn new() -> Self { | ||
AwsErrorRetryPolicy | ||
AwsResponseClassifier | ||
jdisanti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
impl Default for AwsErrorRetryPolicy { | ||
impl Default for AwsResponseClassifier { | ||
fn default() -> Self { | ||
Self::new() | ||
} | ||
} | ||
|
||
impl<T, E> ClassifyResponse<T, SdkError<E>> for AwsErrorRetryPolicy | ||
impl<T, E> ClassifyResponse<T, SdkError<E>> for AwsResponseClassifier | ||
where | ||
E: ProvideErrorKind, | ||
{ | ||
fn classify(&self, err: Result<&T, &SdkError<E>>) -> 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<E>>) -> RetryKind { | ||
let (err, response) = match DefaultResponseClassifier::try_extract_err_response(result) { | ||
jdisanti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Ok(extracted) => extracted, | ||
Err(retry_kind) => return retry_kind, | ||
}; | ||
if let Some(retry_after_delay) = response | ||
.http() | ||
|
@@ -105,15 +91,23 @@ 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}; | ||
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, | ||
|
@@ -151,7 +145,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 +155,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 +168,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 +182,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 +208,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 +230,7 @@ mod test { | |
} | ||
} | ||
|
||
let policy = AwsErrorRetryPolicy::new(); | ||
let policy = AwsResponseClassifier::new(); | ||
|
||
assert_eq!( | ||
policy.classify(make_err(ModeledRetries, test_response).as_ref()), | ||
|
@@ -246,7 +240,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") | ||
|
@@ -258,9 +252,26 @@ mod test { | |
); | ||
} | ||
|
||
#[test] | ||
fn classify_response_error() { | ||
let policy = AwsResponseClassifier::new(); | ||
assert_eq!( | ||
policy.classify( | ||
Result::<SdkSuccess<()>, SdkError<UnmodeledError>>::Err(SdkError::ResponseError { | ||
err: Box::new(UnmodeledError), | ||
raw: operation::Response::new( | ||
http::Response::new("OK").map(|b| SdkBody::from(b)) | ||
), | ||
}) | ||
.as_ref() | ||
), | ||
RetryKind::Error(ErrorKind::TransientError) | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_timeout_error() { | ||
let policy = AwsErrorRetryPolicy::new(); | ||
let policy = AwsResponseClassifier::new(); | ||
let err: Result<(), SdkError<UnmodeledError>> = Err(SdkError::TimeoutError("blah".into())); | ||
assert_eq!( | ||
policy.classify(err.as_ref()), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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::<TestParseResponse, ()>::new()) | ||
.layer(ParseResponseLayer::< | ||
TestParseResponse, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider calling this "TestResponseHandler" or, instead, changing the verbiage for "response handler" to "response parser" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the name is actually fine considering it implements |
||
DefaultResponseClassifier, | ||
>::new()) | ||
.layer(MapRequestLayer::for_mapper(AddHeader)) | ||
.layer(DispatchLayer) | ||
.service(http_layer); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
White label/adhoc AWS clients already had retry. This is specifically fixing for Smithy clients. I think this should be clear by it being a
smithy-rs
entry.