feat: retry if retryable trait is set (#1238)

Author: trivikr

Diff for: packages/middleware-retry/package.json

@@ -20,6 +20,7 @@
   },
   "devDependencies": {
     "@aws-sdk/protocol-http": "1.0.0-gamma.1",
+    "@aws-sdk/smithy-client": "1.0.0-gamma.1",
     "@types/jest": "^25.1.4",
     "jest": "^25.1.0",
     "typescript": "~3.8.3"

Diff for: packages/middleware-retry/src/defaultStrategy.ts

@@ -5,8 +5,8 @@ import {
 import { defaultDelayDecider } from "./delayDecider";
 import { defaultRetryDecider } from "./retryDecider";
 import { isThrottlingError } from "@aws-sdk/service-error-classification";
+import { SdkError } from "@aws-sdk/smithy-client";
 import {
-  SdkError,
   FinalizeHandler,
   MetadataBearer,
   FinalizeHandlerArguments,

Diff for: packages/middleware-retry/src/index.spec.ts

@@ -7,7 +7,7 @@ import { resolveRetryConfig } from "./configurations";
 import * as delayDeciderModule from "./delayDecider";
 import { ExponentialBackOffStrategy, RetryDecider } from "./defaultStrategy";
 import { HttpRequest } from "@aws-sdk/protocol-http";
-import { SdkError } from "@aws-sdk/types";
+import { SdkError } from "@aws-sdk/smithy-client";
 
 describe("retryMiddleware", () => {
   it("should not retry when the handler completes successfully", async () => {

Diff for: packages/middleware-retry/src/retryDecider.spec.ts

@@ -1,25 +1,39 @@
 import {
+  isRetryableByTrait,
   isClockSkewError,
   isThrottlingError,
   isTransientError
 } from "@aws-sdk/service-error-classification";
 import { defaultRetryDecider } from "./retryDecider";
+import { SdkError } from "@aws-sdk/smithy-client";
 
 jest.mock("@aws-sdk/service-error-classification", () => ({
+  isRetryableByTrait: jest.fn().mockReturnValue(false),
   isClockSkewError: jest.fn().mockReturnValue(false),
   isThrottlingError: jest.fn().mockReturnValue(false),
   isTransientError: jest.fn().mockReturnValue(false)
 }));
 
 describe("defaultRetryDecider", () => {
-  const createMockError = () => Object.assign(new Error(), { $metadata: {} });
+  const createMockError = () =>
+    Object.assign(new Error(), { $metadata: {} }) as SdkError;
 
   beforeEach(() => {
     jest.clearAllMocks();
   });
 
   it("should return false when the provided error is falsy", () => {
     expect(defaultRetryDecider(null as any)).toBe(false);
+    expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(0);
+    expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(0);
+    expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(0);
+    expect((isTransientError as jest.Mock).mock.calls.length).toBe(0);
+  });
+
+  it("should return true for RetryableByTrait error", () => {
+    (isRetryableByTrait as jest.Mock).mockReturnValueOnce(true);
+    expect(defaultRetryDecider(createMockError())).toBe(true);
+    expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1);
     expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(0);
     expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(0);
     expect((isTransientError as jest.Mock).mock.calls.length).toBe(0);
@@ -28,6 +42,7 @@ describe("defaultRetryDecider", () => {
   it("should return true for ClockSkewError", () => {
     (isClockSkewError as jest.Mock).mockReturnValueOnce(true);
     expect(defaultRetryDecider(createMockError())).toBe(true);
+    expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1);
     expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1);
     expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(0);
     expect((isTransientError as jest.Mock).mock.calls.length).toBe(0);
@@ -36,6 +51,7 @@ describe("defaultRetryDecider", () => {
   it("should return true for ThrottlingError", () => {
     (isThrottlingError as jest.Mock).mockReturnValueOnce(true);
     expect(defaultRetryDecider(createMockError())).toBe(true);
+    expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1);
     expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1);
     expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(1);
     expect((isTransientError as jest.Mock).mock.calls.length).toBe(0);
@@ -44,13 +60,15 @@ describe("defaultRetryDecider", () => {
   it("should return true for TransientError", () => {
     (isTransientError as jest.Mock).mockReturnValueOnce(true);
     expect(defaultRetryDecider(createMockError())).toBe(true);
+    expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1);
     expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1);
     expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(1);
     expect((isTransientError as jest.Mock).mock.calls.length).toBe(1);
   });
 
   it("should return false for other errors", () => {
     expect(defaultRetryDecider(createMockError())).toBe(false);
+    expect((isRetryableByTrait as jest.Mock).mock.calls.length).toBe(1);
     expect((isClockSkewError as jest.Mock).mock.calls.length).toBe(1);
     expect((isThrottlingError as jest.Mock).mock.calls.length).toBe(1);
     expect((isTransientError as jest.Mock).mock.calls.length).toBe(1);

Diff for: packages/middleware-retry/src/retryDecider.ts

@@ -1,16 +1,18 @@
 import {
   isClockSkewError,
+  isRetryableByTrait,
   isThrottlingError,
   isTransientError
 } from "@aws-sdk/service-error-classification";
-import { SdkError } from "@aws-sdk/types";
+import { SdkError } from "@aws-sdk/smithy-client";
 
 export const defaultRetryDecider = (error: SdkError) => {
   if (!error) {
     return false;
   }
 
   return (
+    isRetryableByTrait(error) ||
     isClockSkewError(error) ||
     isThrottlingError(error) ||
     isTransientError(error)

Diff for: packages/service-error-classification/package.json

@@ -13,10 +13,8 @@
     "url": "https://aws.amazon.com/javascript/"
   },
   "license": "Apache-2.0",
-  "dependencies": {
-    "@aws-sdk/types": "1.0.0-gamma.1"
-  },
   "devDependencies": {
+    "@aws-sdk/smithy-client": "1.0.0-gamma.1",
     "@types/jest": "^25.1.4",
     "jest": "^25.1.0",
     "typescript": "~3.8.3"

Diff for: packages/service-error-classification/src/index.spec.ts

@@ -4,22 +4,43 @@ import {
   TRANSIENT_ERROR_CODES,
   TRANSIENT_ERROR_STATUS_CODES
 } from "./constants";
-import { isClockSkewError, isThrottlingError, isTransientError } from "./index";
-import { SdkError } from "@aws-sdk/types";
+import {
+  isRetryableByTrait,
+  isClockSkewError,
+  isThrottlingError,
+  isTransientError
+} from "./index";
+import { SdkError, RetryableTrait } from "@aws-sdk/smithy-client";
 
 const checkForErrorType = (
   isErrorTypeFunc: (error: SdkError) => boolean,
-  options: { name?: string; httpStatusCode?: number },
+  options: {
+    name?: string;
+    httpStatusCode?: number;
+    $retryable?: RetryableTrait;
+  },
   errorTypeResult: boolean
 ) => {
-  const { name, httpStatusCode } = options;
+  const { name, httpStatusCode, $retryable } = options;
   const error = Object.assign(new Error(), {
     name,
-    $metadata: { httpStatusCode }
+    $metadata: { httpStatusCode },
+    $retryable
   });
-  expect(isErrorTypeFunc(error)).toBe(errorTypeResult);
+  expect(isErrorTypeFunc(error as SdkError)).toBe(errorTypeResult);
 };
 
+describe("isRetryableByTrait", () => {
+  it("should declare error with $retryable set to be a Retryable by trait", () => {
+    const $retryable = {};
+    checkForErrorType(isRetryableByTrait, { $retryable }, true);
+  });
+
+  it("should not declare error with $retryable not set to be a Retryable by trait", () => {
+    checkForErrorType(isRetryableByTrait, {}, false);
+  });
+});
+
 describe("isClockSkewError", () => {
   CLOCK_SKEW_ERROR_CODES.forEach(name => {
     it(`should declare error with the name "${name}" to be a ClockSkew error`, () => {
@@ -54,6 +75,21 @@ describe("isThrottlingError", () => {
       break;
     }
   }
+
+  it("should declare error with $retryable.throttling set to true to be a Throttling error", () => {
+    const $retryable = { throttling: true };
+    checkForErrorType(isThrottlingError, { $retryable }, true);
+  });
+
+  it("should not declare error with $retryable.throttling set to false to be a Throttling error", () => {
+    const $retryable = { throttling: false };
+    checkForErrorType(isThrottlingError, { $retryable }, false);
+  });
+
+  it("should not declare error with $retryable.throttling not set to be a Throttling error", () => {
+    const $retryable = {};
+    checkForErrorType(isThrottlingError, { $retryable }, false);
+  });
 });
 
 describe("isTransientError", () => {

Diff for: packages/service-error-classification/src/index.ts

@@ -4,13 +4,17 @@ import {
   TRANSIENT_ERROR_CODES,
   TRANSIENT_ERROR_STATUS_CODES
 } from "./constants";
-import { SdkError } from "@aws-sdk/types";
+import { SdkError } from "@aws-sdk/smithy-client";
+
+export const isRetryableByTrait = (error: SdkError) =>
+  error.$retryable !== undefined;
 
 export const isClockSkewError = (error: SdkError) =>
   CLOCK_SKEW_ERROR_CODES.includes(error.name);
 
 export const isThrottlingError = (error: SdkError) =>
-  THROTTLING_ERROR_CODES.includes(error.name);
+  THROTTLING_ERROR_CODES.includes(error.name) ||
+  error.$retryable?.throttling == true;
 
 export const isTransientError = (error: SdkError) =>
   TRANSIENT_ERROR_CODES.includes(error.name) ||

Diff for: packages/smithy-client/src/exception.ts

@@ -1,3 +1,5 @@
+import { RetryableTrait } from "./retryable-trait";
+
 /**
  * Type that is implemented by all Smithy shapes marked with the
  * error trait.
@@ -17,4 +19,9 @@ export interface SmithyException {
    * The service that encountered the exception.
    */
   readonly $service?: string;
+
+  /**
+   * Indicates that an error MAY be retried by the client.
+   */
+  readonly $retryable?: RetryableTrait;
 }

Diff for: packages/smithy-client/src/index.ts

@@ -10,3 +10,5 @@ export * from "./lazy-json";
 export * from "./date-utils";
 export * from "./split-every";
 export * from "./constants";
+export * from "./retryable-trait";
+export * from "./sdk-error";

Diff for: packages/smithy-client/src/retryable-trait.ts

@@ -0,0 +1,10 @@
+/**
+ * A structure shape with the error trait.
+ * https://awslabs.github.io/smithy/spec/core.html#retryable-trait
+ */
+export interface RetryableTrait {
+  /**
+   * Indicates that the error is a retryable throttling error.
+   */
+  readonly throttling?: boolean;
+}

Diff for: packages/smithy-client/src/sdk-error.ts

@@ -0,0 +1,4 @@
+import { SmithyException } from "./exception";
+import { MetadataBearer } from "@aws-sdk/types";
+
+export type SdkError = Error & SmithyException & MetadataBearer;

Diff for: packages/types/src/util.ts

@@ -53,9 +53,6 @@ export interface BodyLengthCalculator {
   (body: any): number | undefined;
 }
 
-// TODO Unify with the types created for the error parsers
-export type SdkError = Error & MetadataBearer;
-
 /**
  * Interface that specifies the retry behavior
  */