diff --git a/packages/middleware-retry/src/constants.ts b/packages/middleware-retry/src/constants.ts index 0f00dc659f7b..5f976928e514 100644 --- a/packages/middleware-retry/src/constants.ts +++ b/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(); -[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 --git a/packages/middleware-retry/src/retryDecider.spec.ts b/packages/middleware-retry/src/retryDecider.spec.ts index 91c86c55265c..9ff61d463426 100644 --- a/packages/middleware-retry/src/retryDecider.spec.ts +++ b/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 --git a/packages/middleware-retry/src/retryDecider.ts b/packages/middleware-retry/src/retryDecider.ts index 2d8e7b9fc390..7a7e0b32ad36 100644 --- a/packages/middleware-retry/src/retryDecider.ts +++ b/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 --git a/packages/service-error-classification/package.json b/packages/service-error-classification/package.json index 046c2529b608..555178ebb01d 100644 --- a/packages/service-error-classification/package.json +++ b/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 --git a/packages/service-error-classification/src/constants.ts b/packages/service-error-classification/src/constants.ts index 755f54be5172..f4f2e7c03cbe 100644 --- a/packages/service-error-classification/src/constants.ts +++ b/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 --git a/packages/service-error-classification/src/index.spec.ts b/packages/service-error-classification/src/index.spec.ts index 5a75c0fd3633..a7c6ac627078 100644 --- a/packages/service-error-classification/src/index.spec.ts +++ b/packages/service-error-classification/src/index.spec.ts @@ -1,20 +1,29 @@ -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); }); }); @@ -22,7 +31,7 @@ describe("isClockSkewError", () => { 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,7 +41,7 @@ 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); }); }); @@ -40,7 +49,41 @@ describe("isThrottlingError", () => { 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 --git a/packages/service-error-classification/src/index.ts b/packages/service-error-classification/src/index.ts index 558ea92c589b..04581cf5d7b0 100644 --- a/packages/service-error-classification/src/index.ts +++ b/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 --git a/packages/types/src/util.ts b/packages/types/src/util.ts index f7e5f813f1ac..424ed53f42e9 100644 --- a/packages/types/src/util.ts +++ b/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