diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b07029ba9e1587cf8bc033426e375f871573b95..ff8fc695887f00f4ae39a34dd1bc543422bbd431 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 40c1cfb2d9f287d7c9d637c3976682254cb56f11..a1bbd473f407efe2c745becc2d761747cd154859 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 253dc31cc8d377cc036e595ddd53f344cc47246e..adcc352e0e6d697cabbab9c6b9d13109d52b4f53 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 714b132137ad8bbf445998c13bf8c276721ed6ac..7e95ebbdc53647b1456ce37a35ecee604dc4f33a 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 2cf9ff4c5907a54a81b1affc04c61a18419ee5a3..4aa5772a871aca68f01b4d8d7c88a3c6454d7d9f 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 12547bd4606408e1e2c2f251c45bac4fa0b5169d..b2c7398a2f76c427e43736e2d48e79fca71f91f1 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 22811c6d8a510154de0999eb801f6e281e278acf..f167e2762833b0c56a88b0e0910bf15bf3fdf760 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 4259b9910e9dd74b9efb852e6ab741a810c063ca..56990bbdb01f68d3671ce6427ac02f8cab9fa13c 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 ffc0cff8225b5ac041d3e28efcc02b9547542393..a17c756b749aa47dfcd9efb58a7a3248c52668f0 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), } } }