Retry bug (#238)

* Fix bug in RetryPolicy * Add tracing to the retry policy
smithy-lang · Mar 5, 2021 · 00671a2 · 00671a2
1 parent 77b24b2
commit 00671a2
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 6 deletions.
diff --git a/aws/rust-runtime/aws-http/src/lib.rs b/aws/rust-runtime/aws-http/src/lib.rs
@@ -16,7 +16,7 @@ use std::time::Duration;
 #[derive(Clone)]
 pub struct AwsErrorRetryPolicy;
 
-const TRANSIENT_ERROR_STATUS_CODES: [u16; 2] = [400, 408];
+const TRANSIENT_ERROR_STATUS_CODES: &[u16] = &[500, 502, 503, 504];
 const THROTTLING_ERRORS: &[&str] = &[
  "Throttling",
  "ThrottlingException",
@@ -137,7 +137,7 @@ mod test {
  fn classify_by_response_status() {
  let policy = AwsErrorRetryPolicy::new();
  let test_resp = http::Response::builder()
- .status(408)
+ .status(500)
  .body("error!")
  .unwrap();
  assert_eq!(
@@ -146,6 +146,19 @@ mod test {
  );
  }
 
+ #[test]
+ fn classify_by_response_status_not_retryable() {
+ let policy = AwsErrorRetryPolicy::new();
+ let test_resp = http::Response::builder()
+ .status(408)
+ .body("error!")
+ .unwrap();
+ assert_eq!(
+ policy.classify(make_err(UnmodeledError, test_resp).as_ref()),
+ RetryKind::NotRetryable
+ );
+ }
+
  #[test]
  fn classify_by_error_code() {
  let test_response = http::Response::new("OK");

diff --git a/aws/rust-runtime/aws-hyper/Cargo.toml b/aws/rust-runtime/aws-hyper/Cargo.toml
@@ -22,6 +22,7 @@ smithy-types = { path = "../../../rust-runtime/smithy-types" }
 smithy-http-tower = { path = "../../../rust-runtime/smithy-http-tower" }
 fastrand = "1.4.0"
 tokio = { version = "1", features = ["time"]}
+tracing = "0.1.25"
 
 [dev-dependencies]
 tokio = { version = "1", features = ["full", "test-util"] }

diff --git a/aws/rust-runtime/aws-hyper/src/retry.rs b/aws/rust-runtime/aws-hyper/src/retry.rs
@@ -26,6 +26,7 @@ use std::future::Future;
 use std::pin::Pin;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
+use tracing::Instrument;
 
 /// Retry Policy Configuration
 ///
@@ -249,17 +250,17 @@ where
  ) -> Option<Self::Future> {
  let policy = req.retry_policy();
  let retry = policy.classify(result);
- let (next, fut) = match retry {
+ let (next, dur) = match retry {
  RetryKind::Explicit(dur) => (self.clone(), dur),
  RetryKind::NotRetryable => return None,
  RetryKind::Error(err) => self.attempt_retry(Err(err))?,
  _ => return None,
  };
  let fut = async move {
- tokio::time::sleep(fut).await;
+ tokio::time::sleep(dur).await;
  next
- };
-  Some(Box::pin(fut))
+ }.instrument(tracing::info_span!("retry", kind = &debug(retry)));
+ Some(Box::pin(fut))
  }
 
  fn clone_request(&self, req: &Operation<Handler, R>) -> Option<Operation<Handler, R>> {