feat: add check for isTransientError (#1222)

Author: trivikr

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

@@ -10,14 +10,6 @@ export const DEFAULT_RETRY_DELAY_BASE = 100;
  */
 export const MAXIMUM_RETRY_DELAY = 20 * 1000;
 
-/**
- * HTTP status codes that indicate the operation may be retried.
- */
-export const RETRYABLE_STATUS_CODES = new Set<number>();
-[429, 500, 502, 503, 504, 509].forEach(code =>
-  RETRYABLE_STATUS_CODES.add(code)
-);
-
 /**
  * The retry delay base (in milliseconds) to use when a throttling error is
  * encountered.

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

@@ -1,37 +1,58 @@
+import {
+  isClockSkewError,
+  isThrottlingError,
+  isTransientError
+} from "@aws-sdk/service-error-classification";
 import { defaultRetryDecider } from "./retryDecider";
 
+jest.mock("@aws-sdk/service-error-classification", () => ({
+  isClockSkewError: jest.fn().mockReturnValue(false),
+  isThrottlingError: jest.fn().mockReturnValue(false),
+  isTransientError: jest.fn().mockReturnValue(false)
+}));
+
 describe("defaultRetryDecider", () => {
+  const createMockError = () => Object.assign(new Error(), { $metadata: {} });
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
   it("should return false when the provided error is falsy", () => {
     expect(defaultRetryDecider(null as any)).toBe(false);
+    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 if the error was tagged as a connection error", () => {
-    const err: Error & { connectionError?: boolean } = new Error();
-    err.connectionError = true;
-    expect(defaultRetryDecider(err)).toBe(true);
+  it("should return true for ClockSkewError", () => {
+    (isClockSkewError as jest.Mock).mockReturnValueOnce(true);
+    expect(defaultRetryDecider(createMockError())).toBe(true);
+    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);
   });
 
-  for (const httpStatusCode of [429, 500, 502, 503, 504, 509]) {
-    it(`should return true if the error represents a service response with an HTTP status code of ${httpStatusCode}`, () => {
-      const err: any = new Error();
-      err.$metadata = { httpStatusCode };
-      expect(defaultRetryDecider(err)).toBe(true);
-    });
-  }
-
-  it('should return true if the response represents a "still processing" error', () => {
-    const err = new Error();
-    err.name = "PriorRequestNotComplete";
-    expect(defaultRetryDecider(err)).toBe(true);
+  it("should return true for ThrottlingError", () => {
+    (isThrottlingError as jest.Mock).mockReturnValueOnce(true);
+    expect(defaultRetryDecider(createMockError())).toBe(true);
+    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);
   });
 
-  it("should return true if the response represents a throttling error", () => {
-    const err = new Error();
-    err.name = "TooManyRequestsException";
-    expect(defaultRetryDecider(err)).toBe(true);
+  it("should return true for TransientError", () => {
+    (isTransientError as jest.Mock).mockReturnValueOnce(true);
+    expect(defaultRetryDecider(createMockError())).toBe(true);
+    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(new Error())).toBe(false);
+    expect(defaultRetryDecider(createMockError())).toBe(false);
+    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,30 +1,18 @@
-import { RETRYABLE_STATUS_CODES } from "./constants";
 import {
   isClockSkewError,
-  isThrottlingError
+  isThrottlingError,
+  isTransientError
 } from "@aws-sdk/service-error-classification";
-import { MetadataBearer, SdkError } from "@aws-sdk/types";
+import { SdkError } from "@aws-sdk/types";
 
 export const defaultRetryDecider = (error: SdkError) => {
   if (!error) {
     return false;
   }
 
-  if (error.connectionError) {
-    return true;
-  }
-
-  if (
-    hasMetadata(error) &&
-    error.$metadata.httpStatusCode &&
-    RETRYABLE_STATUS_CODES.has(error.$metadata.httpStatusCode)
-  ) {
-    return true;
-  }
-
-  return isThrottlingError(error) || isClockSkewError(error);
+  return (
+    isClockSkewError(error) ||
+    isThrottlingError(error) ||
+    isTransientError(error)
+  );
 };
-
-function hasMetadata(error: any): error is MetadataBearer {
-  return error?.$metadata;
-}

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

@@ -13,6 +13,9 @@
     "url": "https://aws.amazon.com/javascript/"
   },
   "license": "Apache-2.0",
+  "dependencies": {
+    "@aws-sdk/types": "1.0.0-gamma.1"
+  },
   "devDependencies": {
     "@types/jest": "^25.1.4",
     "jest": "^25.1.0",

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

@@ -35,3 +35,18 @@ export const THROTTLING_ERROR_CODES = [
   "PriorRequestNotComplete",
   "EC2ThrottledException"
 ];
+
+/**
+ * Error codes that indicate transient issues
+ */
+export const TRANSIENT_ERROR_CODES = [
+  "AbortError",
+  "TimeoutError",
+  "RequestTimeout",
+  "RequestTimeoutException"
+];
+
+/**
+ * Error codes that indicate transient issues
+ */
+export const TRANSIENT_ERROR_STATUS_CODES = [500, 502, 503, 504];

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

@@ -1,28 +1,37 @@
-import { CLOCK_SKEW_ERROR_CODES, THROTTLING_ERROR_CODES } from "./constants";
-import { isClockSkewError, isThrottlingError } from "./index";
+import {
+  CLOCK_SKEW_ERROR_CODES,
+  THROTTLING_ERROR_CODES,
+  TRANSIENT_ERROR_CODES,
+  TRANSIENT_ERROR_STATUS_CODES
+} from "./constants";
+import { isClockSkewError, isThrottlingError, isTransientError } from "./index";
+import { SdkError } from "@aws-sdk/types";
 
 const checkForErrorType = (
-  isErrorTypeFunc: (error: Error) => boolean,
-  errorName: string,
+  isErrorTypeFunc: (error: SdkError) => boolean,
+  options: { name?: string; httpStatusCode?: number },
   errorTypeResult: boolean
 ) => {
-  const error = new Error();
-  error.name = errorName;
+  const { name, httpStatusCode } = options;
+  const error = Object.assign(new Error(), {
+    name,
+    $metadata: { httpStatusCode }
+  });
   expect(isErrorTypeFunc(error)).toBe(errorTypeResult);
 };
 
 describe("isClockSkewError", () => {
   CLOCK_SKEW_ERROR_CODES.forEach(name => {
     it(`should declare error with the name "${name}" to be a ClockSkew error`, () => {
-      checkForErrorType(isClockSkewError, name, true);
+      checkForErrorType(isClockSkewError, { name }, true);
     });
   });
 
   while (true) {
     const name = Math.random().toString(36).substring(2);
     if (!CLOCK_SKEW_ERROR_CODES.includes(name)) {
       it(`should not declare error with the name "${name}" to be a ClockSkew error`, () => {
-        checkForErrorType(isClockSkewError, name, false);
+        checkForErrorType(isClockSkewError, { name }, false);
       });
       break;
     }
@@ -32,15 +41,49 @@ describe("isClockSkewError", () => {
 describe("isThrottlingError", () => {
   THROTTLING_ERROR_CODES.forEach(name => {
     it(`should declare error with the name "${name}" to be a Throttling error`, () => {
-      checkForErrorType(isThrottlingError, name, true);
+      checkForErrorType(isThrottlingError, { name }, true);
     });
   });
 
   while (true) {
     const name = Math.random().toString(36).substring(2);
     if (!THROTTLING_ERROR_CODES.includes(name)) {
       it(`should not declare error with the name "${name}" to be a Throttling error`, () => {
-        checkForErrorType(isThrottlingError, name, false);
+        checkForErrorType(isThrottlingError, { name }, false);
+      });
+      break;
+    }
+  }
+});
+
+describe("isTransientError", () => {
+  TRANSIENT_ERROR_CODES.forEach(name => {
+    it(`should declare error with the name "${name}" to be a Throttling error`, () => {
+      checkForErrorType(isTransientError, { name }, true);
+    });
+  });
+
+  TRANSIENT_ERROR_STATUS_CODES.forEach(httpStatusCode => {
+    it(`should declare error with the HTTP Status Code "${httpStatusCode}" to be a Throttling error`, () => {
+      checkForErrorType(isTransientError, { httpStatusCode }, true);
+    });
+  });
+
+  while (true) {
+    const name = Math.random().toString(36).substring(2);
+    if (!TRANSIENT_ERROR_CODES.includes(name)) {
+      it(`should not declare error with the name "${name}" to be a Throttling error`, () => {
+        checkForErrorType(isTransientError, { name }, false);
+      });
+      break;
+    }
+  }
+
+  while (true) {
+    const httpStatusCode = Math.ceil(Math.random() * 10 ** 3);
+    if (!TRANSIENT_ERROR_STATUS_CODES.includes(httpStatusCode)) {
+      it(`should declare error with the HTTP Status Code "${httpStatusCode}" to be a Throttling error`, () => {
+        checkForErrorType(isTransientError, { httpStatusCode }, false);
       });
       break;
     }

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

@@ -1,7 +1,17 @@
-import { CLOCK_SKEW_ERROR_CODES, THROTTLING_ERROR_CODES } from "./constants";
+import {
+  CLOCK_SKEW_ERROR_CODES,
+  THROTTLING_ERROR_CODES,
+  TRANSIENT_ERROR_CODES,
+  TRANSIENT_ERROR_STATUS_CODES
+} from "./constants";
+import { SdkError } from "@aws-sdk/types";
 
-export const isClockSkewError = (error: Error) =>
+export const isClockSkewError = (error: SdkError) =>
   CLOCK_SKEW_ERROR_CODES.includes(error.name);
 
-export const isThrottlingError = (error: Error) =>
+export const isThrottlingError = (error: SdkError) =>
   THROTTLING_ERROR_CODES.includes(error.name);
+
+export const isTransientError = (error: SdkError) =>
+  TRANSIENT_ERROR_CODES.includes(error.name) ||
+  TRANSIENT_ERROR_STATUS_CODES.includes(error.$metadata?.httpStatusCode || 0);

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

@@ -54,7 +54,7 @@ export interface BodyLengthCalculator {
 }
 
 // TODO Unify with the types created for the error parsers
-export type SdkError = Error & { connectionError?: boolean };
+export type SdkError = Error & MetadataBearer;
 
 /**
  * Interface that specifies the retry behavior