From 692bf94ba30f570365f559c67d6a9981c037c371 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Tue, 23 Nov 2021 10:26:31 -0600 Subject: [PATCH] Feature: add TimeoutError variant to SdkError (#886) * feature: add TimeoutError variant to SdkError add: TimeoutError variant to ImdsError update: tests broken by new variant * update: changelogs * add: new inner timeout error RequestTimeoutError update: tests broken by new inner error rename: aws_smithy_client::hyper_ext::TimeoutError to HttpTimeoutError add: missing dep to integration test * revert: IMDS error change update: content for HttpTimeoutError fix: clippy lint * add: back the source method to HttpTimeoutError update: connector timeout tests remove: TODO --- CHANGELOG.md | 1 + aws/SDK_CHANGELOG.md | 1 + .../aws-config/src/imds/client.rs | 3 +- aws/sdk/integration-tests/s3/Cargo.toml | 2 +- .../integration-tests/s3/tests/timeouts.rs | 2 +- .../aws-smithy-client/src/hyper_ext.rs | 62 ++++++++++++------- rust-runtime/aws-smithy-client/src/timeout.rs | 38 +++++++++--- .../src/parse_response.rs | 3 + rust-runtime/aws-smithy-http/src/result.rs | 14 +++-- 9 files changed, 85 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b07029ba..ff8fc6958 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ vNext (Month Day, Year) **New this release** - Improve docs on `aws-smithy-client` (smithy-rs#855) - Fix http-body dependency version (smithy-rs#883, aws-sdk-rust#305) +- `SdkError` now includes a variant `TimeoutError` for when a request times out (smithy-rs#885) **Breaking Changes** - (aws-smithy-client): Extraneous `pub use SdkSuccess` removed from `aws_smithy_client::hyper_ext`. (smithy-rs#855) diff --git a/aws/SDK_CHANGELOG.md b/aws/SDK_CHANGELOG.md index 40c1cfb2d..a1bbd473f 100644 --- a/aws/SDK_CHANGELOG.md +++ b/aws/SDK_CHANGELOG.md @@ -7,6 +7,7 @@ vNext (Month Day, Year) **New this release** - :tada: Timeouts for requests are now configurable. You can set a timeout for each individual request attempt or for all attempts made for a request. (smithy-rs#831) + - `SdkError` now includes a variant `TimeoutError` for when a request times out (smithy-rs#885) - Improve docs on `aws-smithy-client` (smithy-rs#855) - Fix http-body dependency version (smithy-rs#883, aws-sdk-rust#305) diff --git a/aws/rust-runtime/aws-config/src/imds/client.rs b/aws/rust-runtime/aws-config/src/imds/client.rs index 253dc31cc..adcc352e0 100644 --- a/aws/rust-runtime/aws-config/src/imds/client.rs +++ b/aws/rust-runtime/aws-config/src/imds/client.rs @@ -187,6 +187,7 @@ impl Client { Ok(token_failure) => *token_failure, Err(other) => ImdsError::Unexpected(other), }, + SdkError::TimeoutError(err) => ImdsError::IoError(err), SdkError::DispatchFailure(err) => ImdsError::IoError(err.into()), SdkError::ResponseError { err, .. } => ImdsError::IoError(err), SdkError::ServiceError { @@ -259,7 +260,7 @@ impl Display for ImdsError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { ImdsError::FailedToLoadToken(inner) => { - write!(f, "failed to load session token: {}", inner) + write!(f, "Failed to load session token: {}", inner) } ImdsError::InvalidPath => write!( f, diff --git a/aws/sdk/integration-tests/s3/Cargo.toml b/aws/sdk/integration-tests/s3/Cargo.toml index 714b13213..7e95ebbdc 100644 --- a/aws/sdk/integration-tests/s3/Cargo.toml +++ b/aws/sdk/integration-tests/s3/Cargo.toml @@ -14,7 +14,7 @@ aws-smithy-http = { path = "../../build/aws-sdk/sdk/aws-smithy-http" } aws-smithy-async = { path = "../../build/aws-sdk/sdk/aws-smithy-async" } aws-smithy-types = { path = "../../build/aws-sdk/sdk/aws-smithy-types" } tracing-subscriber = "0.2.18" -tokio = { version = "1", features = ["full"]} +tokio = { version = "1", features = ["full", "test-util"]} [dev-dependencies] aws-http = { path = "../../build/aws-sdk/sdk/aws-http"} diff --git a/aws/sdk/integration-tests/s3/tests/timeouts.rs b/aws/sdk/integration-tests/s3/tests/timeouts.rs index 2cf9ff4c5..4aa5772a8 100644 --- a/aws/sdk/integration-tests/s3/tests/timeouts.rs +++ b/aws/sdk/integration-tests/s3/tests/timeouts.rs @@ -62,6 +62,6 @@ async fn test_timeout_service_ends_request_that_never_completes() { .await .unwrap_err(); - assert_eq!(format!("{:?}", err), "ConstructionFailure(TimedOutError)"); + assert_eq!(format!("{:?}", err), "TimeoutError(RequestTimeoutError { kind: \"API call (all attempts including retries)\", duration: 500ms })"); assert_elapsed!(now, std::time::Duration::from_secs_f32(0.5)); } diff --git a/rust-runtime/aws-smithy-client/src/hyper_ext.rs b/rust-runtime/aws-smithy-client/src/hyper_ext.rs index 12547bd46..b2c7398a2 100644 --- a/rust-runtime/aws-smithy-client/src/hyper_ext.rs +++ b/rust-runtime/aws-smithy-client/src/hyper_ext.rs @@ -55,7 +55,7 @@ use aws_smithy_types::retry::ErrorKind; use crate::{timeout, Builder as ClientBuilder}; -use self::timeout_middleware::{ConnectTimeout, HttpReadTimeout, TimeoutError}; +use self::timeout_middleware::{ConnectTimeout, HttpReadTimeout, HttpTimeoutError}; /// Adapter from a [`hyper::Client`](hyper::Client) to a connector usable by a Smithy [`Client`](crate::Client). /// @@ -128,7 +128,7 @@ fn downcast_error(err: BoxError) -> ConnectorError { /// Convert a [`hyper::Error`] into a [`ConnectorError`] fn to_connector_error(err: hyper::Error) -> ConnectorError { - if err.is_timeout() || find_source::(&err).is_some() { + if err.is_timeout() || find_source::(&err).is_some() { ConnectorError::timeout(err.into()) } else if err.is_user() { ConnectorError::user(err.into()) @@ -322,21 +322,27 @@ mod timeout_middleware { use aws_smithy_async::rt::sleep::Sleep; #[derive(Debug)] - pub(crate) struct TimeoutError { - operation: &'static str, + pub(crate) struct HttpTimeoutError { + kind: &'static str, duration: Duration, - cause: TimedOutError, } - impl std::fmt::Display for TimeoutError { + impl std::fmt::Display for HttpTimeoutError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "timed out after {:?}", self.duration) + write!( + f, + "{} timeout occurred after {:?}", + self.kind, self.duration + ) } } - impl Error for TimeoutError { + impl Error for HttpTimeoutError { + // We implement the `source` function as returning a `TimedOutError` because when `downcast_error` + // or `find_source` is called with an `HttpTimeoutError` (or another error wrapping an `HttpTimeoutError`) + // this method will be checked to determine if it's a timeout-related error. fn source(&self) -> Option<&(dyn Error + 'static)> { - Some(&self.cause) + Some(&TimedOutError) } } @@ -422,7 +428,7 @@ mod timeout_middleware { type Output = Result; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - let (timeout_future, timeout_type, dur) = match self.project() { + let (timeout_future, kind, &mut duration) = match self.project() { MaybeTimeoutFutureProj::NoTimeout { future } => { return future.poll(cx).map_err(|err| err.into()) } @@ -434,12 +440,9 @@ mod timeout_middleware { }; match timeout_future.poll(cx) { Poll::Ready(Ok(response)) => Poll::Ready(response.map_err(|err| err.into())), - Poll::Ready(Err(_timeout)) => Poll::Ready(Err(TimeoutError { - operation: timeout_type, - duration: *dur, - cause: TimedOutError, + Poll::Ready(Err(_timeout)) => { + Poll::Ready(Err(HttpTimeoutError { kind, duration }.into())) } - .into())), Poll::Pending => Poll::Pending, } } @@ -464,7 +467,7 @@ mod timeout_middleware { let sleep = sleep.sleep(*duration); MaybeTimeoutFuture::Timeout { timeout: future::timeout::Timeout::new(self.inner.call(req), sleep), - error_type: "connect", + error_type: "HTTP connect", duration: *duration, } } @@ -493,7 +496,6 @@ mod timeout_middleware { let sleep = sleep.sleep(*duration); MaybeTimeoutFuture::Timeout { timeout: future::timeout::Timeout::new(self.inner.call(req), sleep), - error_type: "HTTP read", duration: *duration, } @@ -528,7 +530,7 @@ mod timeout_middleware { fn is_send_sync() {} #[tokio::test] - async fn connect_timeout_works() { + async fn http_connect_timeout_works() { let inner = NeverConnected::new(); let timeout = timeout::Settings::new().with_connect_timeout(Duration::from_secs(1)); let mut hyper = Adapter::builder() @@ -545,17 +547,21 @@ mod timeout_middleware { .unwrap(), ) .await - .expect_err("timeout"); - assert!(resp.is_timeout(), "{:?}", resp); + .unwrap_err(); + assert!( + resp.is_timeout(), + "expected resp.is_timeout() to be true but it was false, resp == {:?}", + resp + ); assert_eq!( format!("{}", resp), - "timeout: error trying to connect: timed out after 1s" + "timeout: error trying to connect: HTTP connect timeout occurred after 1s" ); assert_elapsed!(now, Duration::from_secs(1)); } #[tokio::test] - async fn http_timeout_works() { + async fn http_read_timeout_works() { let inner = NeverReplies::new(); let timeout = timeout::Settings::new() .with_connect_timeout(Duration::from_secs(1)) @@ -574,8 +580,16 @@ mod timeout_middleware { .unwrap(), ) .await - .expect_err("timeout"); - assert!(resp.is_timeout(), "{:?}", resp); + .unwrap_err(); + assert!( + resp.is_timeout(), + "expected resp.is_timeout() to be true but it was false, resp == {:?}", + resp + ); + assert_eq!( + format!("{}", resp), + "timeout: HTTP read timeout occurred after 2s" + ); assert_elapsed!(now, Duration::from_secs(2)); } } diff --git a/rust-runtime/aws-smithy-client/src/timeout.rs b/rust-runtime/aws-smithy-client/src/timeout.rs index 22811c6d8..f167e2762 100644 --- a/rust-runtime/aws-smithy-client/src/timeout.rs +++ b/rust-runtime/aws-smithy-client/src/timeout.rs @@ -16,7 +16,7 @@ use std::task::{Context, Poll}; use std::time::Duration; use crate::SdkError; -use aws_smithy_async::future::timeout::{TimedOutError, Timeout}; +use aws_smithy_async::future::timeout::Timeout; use aws_smithy_async::rt::sleep::{AsyncSleep, Sleep}; use aws_smithy_http::operation::Operation; use aws_smithy_types::timeout::TimeoutConfig; @@ -65,6 +65,30 @@ impl Settings { } } +#[derive(Debug)] +struct RequestTimeoutError { + kind: &'static str, + duration: Duration, +} + +impl RequestTimeoutError { + pub fn new_boxed(kind: &'static str, duration: Duration) -> Box { + Box::new(Self { kind, duration }) + } +} + +impl std::fmt::Display for RequestTimeoutError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{} timeout occurred after {:?}", + self.kind, self.duration + ) + } +} + +impl std::error::Error for RequestTimeoutError {} + #[derive(Clone, Debug)] /// A struct containing everything needed to create a new [`TimeoutService`] pub struct TimeoutServiceParams { @@ -217,8 +241,7 @@ where type Output = Result>; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - // TODO duration will be used in the error message once a Timeout variant is added to SdkError - let (future, _kind, _duration) = match self.project() { + let (future, kind, duration) = match self.project() { TimeoutServiceFutureProj::NoTimeout { future } => return future.poll(cx), TimeoutServiceFutureProj::Timeout { future, @@ -228,10 +251,9 @@ where }; match future.poll(cx) { Poll::Ready(Ok(response)) => Poll::Ready(response), - Poll::Ready(Err(_timeout)) => { - // TODO update SdkError to include a variant specifically for timeouts - Poll::Ready(Err(SdkError::ConstructionFailure(Box::new(TimedOutError)))) - } + Poll::Ready(Err(_timeout)) => Poll::Ready(Err(SdkError::TimeoutError( + RequestTimeoutError::new_boxed(kind, *duration), + ))), Poll::Pending => Poll::Pending, } } @@ -295,7 +317,7 @@ mod test { let err: SdkError> = svc.ready().await.unwrap().call(op).await.unwrap_err(); - assert_eq!(format!("{:?}", err), "ConstructionFailure(TimedOutError)"); + assert_eq!(format!("{:?}", err), "TimeoutError(RequestTimeoutError { kind: \"API call (all attempts including retries)\", duration: 250ms })"); assert_elapsed!(now, Duration::from_secs_f32(0.25)); } } diff --git a/rust-runtime/aws-smithy-http-tower/src/parse_response.rs b/rust-runtime/aws-smithy-http-tower/src/parse_response.rs index 4259b9910..56990bbdb 100644 --- a/rust-runtime/aws-smithy-http-tower/src/parse_response.rs +++ b/rust-runtime/aws-smithy-http-tower/src/parse_response.rs @@ -130,6 +130,9 @@ where Err(SdkError::ConstructionFailure(err)) => inner_span .record("status", &"construction_failure") .record("message", &display(err)), + Err(SdkError::TimeoutError(err)) => inner_span + .record("status", &"timeout_error") + .record("message", &display(err)), }; resp } diff --git a/rust-runtime/aws-smithy-http/src/result.rs b/rust-runtime/aws-smithy-http/src/result.rs index ffc0cff82..a17c756b7 100644 --- a/rust-runtime/aws-smithy-http/src/result.rs +++ b/rust-runtime/aws-smithy-http/src/result.rs @@ -33,12 +33,12 @@ pub struct SdkSuccess { /// Failed SDK Result #[derive(Debug)] pub enum SdkError { - // TODO Request failures due to a timeout currently report this error type even though - // they're not really a construction failure. Add a new variant for timeouts or update - // DispatchFailure to accept more than just ConnectorErrors /// The request failed during construction. It was not dispatched over the network. ConstructionFailure(BoxError), + /// The request failed due to a timeout. The request MAY have been sent and received. + TimeoutError(BoxError), + /// The request failed during dispatch. An HTTP response was not received. The request MAY /// have been sent. DispatchFailure(ConnectorError), @@ -180,6 +180,7 @@ where fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { match self { SdkError::ConstructionFailure(err) => write!(f, "failed to construct request: {}", err), + SdkError::TimeoutError(err) => write!(f, "request has timed out: {}", err), SdkError::DispatchFailure(err) => Display::fmt(&err, f), SdkError::ResponseError { err, .. } => Display::fmt(&err, f), SdkError::ServiceError { err, .. } => Display::fmt(&err, f), @@ -193,12 +194,13 @@ where R: Debug, { fn source(&self) -> Option<&(dyn Error + 'static)> { + use SdkError::*; match self { - SdkError::ConstructionFailure(err) | SdkError::ResponseError { err, .. } => { + ConstructionFailure(err) | TimeoutError(err) | ResponseError { err, .. } => { Some(err.as_ref()) } - SdkError::DispatchFailure(err) => Some(err), - SdkError::ServiceError { err, .. } => Some(err), + DispatchFailure(err) => Some(err), + ServiceError { err, .. } => Some(err), } } } -- GitLab