Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: retry if retryable trait is set #1238

Merged
merged 8 commits into from
Jun 4, 2020
1 change: 1 addition & 0 deletions packages/middleware-retry/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion packages/middleware-retry/src/defaultStrategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/middleware-retry/src/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
20 changes: 19 additions & 1 deletion packages/middleware-retry/src/retryDecider.spec.ts
Original file line number Diff line number Diff line change
@@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion packages/middleware-retry/src/retryDecider.ts
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
4 changes: 1 addition & 3 deletions packages/service-error-classification/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
48 changes: 42 additions & 6 deletions packages/service-error-classification/src/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`, () => {
Expand Down Expand Up @@ -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", () => {
Expand Down
8 changes: 6 additions & 2 deletions packages/service-error-classification/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
Expand Down
7 changes: 7 additions & 0 deletions packages/smithy-client/src/exception.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { RetryableTrait } from "./retryable-trait";

/**
* Type that is implemented by all Smithy shapes marked with the
* error trait.
Expand All @@ -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;
}
2 changes: 2 additions & 0 deletions packages/smithy-client/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
10 changes: 10 additions & 0 deletions packages/smithy-client/src/retryable-trait.ts
Original file line number Diff line number Diff line change
@@ -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;
}
4 changes: 4 additions & 0 deletions packages/smithy-client/src/sdk-error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import { SmithyException } from "./exception";
import { MetadataBearer } from "@aws-sdk/types";

export type SdkError = Error & SmithyException & MetadataBearer;
3 changes: 0 additions & 3 deletions packages/types/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down