From 648171eac2678969b7e77cbcd130d91e220818fb Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 28 May 2021 12:53:32 +0000 Subject: [PATCH 01/10] chore(middleware-retry): rename defaultStrategy to StandardRetryStrategy --- ...{defaultStrategy.spec.ts => StandardRetryStrategy.spec.ts} | 2 +- .../src/{defaultStrategy.ts => StandardRetryStrategy.ts} | 0 packages/middleware-retry/src/configurations.spec.ts | 4 ++-- packages/middleware-retry/src/configurations.ts | 2 +- packages/middleware-retry/src/index.ts | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) rename packages/middleware-retry/src/{defaultStrategy.spec.ts => StandardRetryStrategy.spec.ts} (99%) rename packages/middleware-retry/src/{defaultStrategy.ts => StandardRetryStrategy.ts} (100%) diff --git a/packages/middleware-retry/src/defaultStrategy.spec.ts b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts similarity index 99% rename from packages/middleware-retry/src/defaultStrategy.spec.ts rename to packages/middleware-retry/src/StandardRetryStrategy.spec.ts index 8093b5e7d41af..76a30f5b33909 100644 --- a/packages/middleware-retry/src/defaultStrategy.spec.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts @@ -5,9 +5,9 @@ import { v4 } from "uuid"; import { DEFAULT_MAX_ATTEMPTS } from "./configurations"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, THROTTLING_RETRY_DELAY_BASE } from "./constants"; import { getDefaultRetryQuota } from "./defaultRetryQuota"; -import { StandardRetryStrategy } from "./defaultStrategy"; import { defaultDelayDecider } from "./delayDecider"; import { defaultRetryDecider } from "./retryDecider"; +import { StandardRetryStrategy } from "./StandardRetryStrategy"; import { RetryQuota } from "./types"; jest.mock("@aws-sdk/service-error-classification"); diff --git a/packages/middleware-retry/src/defaultStrategy.ts b/packages/middleware-retry/src/StandardRetryStrategy.ts similarity index 100% rename from packages/middleware-retry/src/defaultStrategy.ts rename to packages/middleware-retry/src/StandardRetryStrategy.ts diff --git a/packages/middleware-retry/src/configurations.spec.ts b/packages/middleware-retry/src/configurations.spec.ts index cb79a04913756..ebe1b384fe2cc 100644 --- a/packages/middleware-retry/src/configurations.spec.ts +++ b/packages/middleware-retry/src/configurations.spec.ts @@ -5,9 +5,9 @@ import { NODE_MAX_ATTEMPT_CONFIG_OPTIONS, resolveRetryConfig, } from "./configurations"; -import { StandardRetryStrategy } from "./defaultStrategy"; +import { StandardRetryStrategy } from "./StandardRetryStrategy"; -jest.mock("./defaultStrategy"); +jest.mock("./StandardRetryStrategy"); describe("resolveRetryConfig", () => { afterEach(() => { diff --git a/packages/middleware-retry/src/configurations.ts b/packages/middleware-retry/src/configurations.ts index ff9b97cbf8e45..1155ee13aa0a3 100644 --- a/packages/middleware-retry/src/configurations.ts +++ b/packages/middleware-retry/src/configurations.ts @@ -1,7 +1,7 @@ import { LoadedConfigSelectors } from "@aws-sdk/node-config-provider"; import { Provider, RetryStrategy } from "@aws-sdk/types"; -import { StandardRetryStrategy } from "./defaultStrategy"; +import { StandardRetryStrategy } from "./StandardRetryStrategy"; export const ENV_MAX_ATTEMPTS = "AWS_MAX_ATTEMPTS"; export const CONFIG_MAX_ATTEMPTS = "max_attempts"; diff --git a/packages/middleware-retry/src/index.ts b/packages/middleware-retry/src/index.ts index 6013d7d0e74c7..889eb954cb554 100644 --- a/packages/middleware-retry/src/index.ts +++ b/packages/middleware-retry/src/index.ts @@ -1,6 +1,6 @@ export * from "./retryMiddleware"; export * from "./omitRetryHeadersMiddleware"; -export * from "./defaultStrategy"; +export * from "./StandardRetryStrategy"; export * from "./configurations"; export * from "./delayDecider"; export * from "./retryDecider"; From a8740e87ccbb595fe31849f4af2c45f9f2ed2b8b Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 28 May 2021 12:57:18 +0000 Subject: [PATCH 02/10] chore(middleware-retry): add beforeRequest and afterRequest hooks in StandardRetryStrategy --- .../middleware-retry/src/StandardRetryStrategy.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/middleware-retry/src/StandardRetryStrategy.ts b/packages/middleware-retry/src/StandardRetryStrategy.ts index ab621b0687d7a..83d876d168def 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.ts @@ -72,7 +72,10 @@ export class StandardRetryStrategy implements RetryStrategy { if (HttpRequest.isInstance(request)) { request.headers[REQUEST_HEADER] = `attempt=${attempts + 1}; max=${maxAttempts}`; } + + await this.beforeRequest(); const { response, output } = await next(args); + this.afterRequest(); this.retryQuota.releaseRetryTokens(retryTokenAmount); output.$metadata.attempts = attempts + 1; @@ -104,6 +107,14 @@ export class StandardRetryStrategy implements RetryStrategy { } } } + + protected async beforeRequest() { + // No-op for standard mode + } + + protected afterRequest() { + // No-op for standard mode + } } const asSdkError = (error: unknown): SdkError => { From 17b52e717f3800865ff54838fb7a40c8d3c8d378 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 28 May 2021 16:32:28 +0000 Subject: [PATCH 03/10] chore(middleware-retry): add RETRY_MODES with adaptive --- packages/middleware-retry/src/configurations.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/middleware-retry/src/configurations.ts b/packages/middleware-retry/src/configurations.ts index 1155ee13aa0a3..0034308524c20 100644 --- a/packages/middleware-retry/src/configurations.ts +++ b/packages/middleware-retry/src/configurations.ts @@ -6,6 +6,11 @@ import { StandardRetryStrategy } from "./StandardRetryStrategy"; export const ENV_MAX_ATTEMPTS = "AWS_MAX_ATTEMPTS"; export const CONFIG_MAX_ATTEMPTS = "max_attempts"; +export enum RETRY_MODES { + standard = "standard", + adaptive = "adaptive", +} + /** * The default value for how many HTTP requests an SDK should make for a * single SDK operation invocation before giving up @@ -15,7 +20,7 @@ export const DEFAULT_MAX_ATTEMPTS = 3; /** * The default retry algorithm to use. */ -export const DEFAULT_RETRY_MODE = "standard"; +export const DEFAULT_RETRY_MODE = RETRY_MODES.standard; export const NODE_MAX_ATTEMPT_CONFIG_OPTIONS: LoadedConfigSelectors = { environmentVariableSelector: (env) => { From b90b531df325f78ae501e7371d46553e7567c4de Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 28 May 2021 16:42:01 +0000 Subject: [PATCH 04/10] feat(middleware-retry): add AdaptiveRetryStrategy --- .../src/AdaptiveRetryStrategy.ts | 34 +++++++++++++++++++ .../src/StandardRetryStrategy.ts | 8 ++--- 2 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 packages/middleware-retry/src/AdaptiveRetryStrategy.ts diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts new file mode 100644 index 0000000000000..9e5f72e5f0003 --- /dev/null +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts @@ -0,0 +1,34 @@ +import { Provider } from "@aws-sdk/types"; + +import { RETRY_MODES } from "./configurations"; +import { DefaultRateLimiter } from "./DefaultRateLimiter"; +import { StandardRetryStrategy, StandardRetryStrategyOptions } from "./StandardRetryStrategy"; +import { RateLimiter } from "./types"; + +/** + * Strategy options to be passed to AdaptiveRetryStrategy + */ +export interface AdaptiveRetryStrategyOptions extends StandardRetryStrategyOptions { + rateLimiter?: RateLimiter; +} + +export class AdaptiveRetryStrategy extends StandardRetryStrategy { + private rateLimiter: RateLimiter; + + constructor(maxAttemptsProvider: Provider, options?: AdaptiveRetryStrategyOptions) { + const { rateLimiter, ...superOptions } = options ?? {}; + super(maxAttemptsProvider, superOptions); + this.rateLimiter = rateLimiter ?? new DefaultRateLimiter(); + this.mode = RETRY_MODES.adaptive; + } + + protected async beforeRequest() { + await super.beforeRequest(); + await this.rateLimiter.getSendToken(); + } + + protected afterRequest(response: any) { + super.afterRequest(response); + this.rateLimiter.updateClientSendingRate(response); + } +} diff --git a/packages/middleware-retry/src/StandardRetryStrategy.ts b/packages/middleware-retry/src/StandardRetryStrategy.ts index 83d876d168def..f4fb495e8287d 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.ts @@ -4,7 +4,7 @@ import { SdkError } from "@aws-sdk/smithy-client"; import { FinalizeHandler, FinalizeHandlerArguments, MetadataBearer, Provider, RetryStrategy } from "@aws-sdk/types"; import { v4 } from "uuid"; -import { DEFAULT_MAX_ATTEMPTS, DEFAULT_RETRY_MODE } from "./configurations"; +import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./configurations"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, @@ -30,7 +30,7 @@ export class StandardRetryStrategy implements RetryStrategy { private retryDecider: RetryDecider; private delayDecider: DelayDecider; private retryQuota: RetryQuota; - public readonly mode = DEFAULT_RETRY_MODE; + public mode: string = RETRY_MODES.standard; constructor(private readonly maxAttemptsProvider: Provider, options?: StandardRetryStrategyOptions) { this.retryDecider = options?.retryDecider ?? defaultRetryDecider; @@ -75,7 +75,7 @@ export class StandardRetryStrategy implements RetryStrategy { await this.beforeRequest(); const { response, output } = await next(args); - this.afterRequest(); + this.afterRequest(response); this.retryQuota.releaseRetryTokens(retryTokenAmount); output.$metadata.attempts = attempts + 1; @@ -112,7 +112,7 @@ export class StandardRetryStrategy implements RetryStrategy { // No-op for standard mode } - protected afterRequest() { + protected afterRequest(response: any) { // No-op for standard mode } } From 20b76882f9b0d671332816800789ccf5a381abc9 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 28 May 2021 17:30:28 +0000 Subject: [PATCH 05/10] test(middleware-retry): add AdaptiveRetryStrategy --- .../src/AdaptiveRetryStrategy.spec.ts | 86 +++++++++++++++++++ .../src/StandardRetryStrategy.spec.ts | 7 +- 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts new file mode 100644 index 0000000000000..7a263d05ba5d8 --- /dev/null +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts @@ -0,0 +1,86 @@ +import { AdaptiveRetryStrategy } from "./AdaptiveRetryStrategy"; +import { RETRY_MODES } from "./configurations"; +import { DefaultRateLimiter } from "./DefaultRateLimiter"; +import { StandardRetryStrategy } from "./StandardRetryStrategy"; +import { RateLimiter, RetryQuota } from "./types"; + +jest.mock("./StandardRetryStrategy"); +jest.mock("./DefaultRateLimiter"); + +describe(AdaptiveRetryStrategy.name, () => { + const maxAttemptsProvider = jest.fn(); + const mockDefaultRateLimiter = { + getSendToken: jest.fn(), + updateClientSendingRate: jest.fn(), + }; + + beforeEach(() => { + (DefaultRateLimiter as jest.Mock).mockReturnValue(mockDefaultRateLimiter); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe("constructor", () => { + it("calls super constructor", () => { + const retryDecider = jest.fn(); + const delayDecider = jest.fn(); + const retryQuota = {} as RetryQuota; + const rateLimiter = {} as RateLimiter; + + new AdaptiveRetryStrategy(maxAttemptsProvider, { + retryDecider, + delayDecider, + retryQuota, + rateLimiter, + }); + expect(StandardRetryStrategy).toHaveBeenCalledWith(maxAttemptsProvider, { + retryDecider, + delayDecider, + retryQuota, + }); + }); + + it(`sets mode=${RETRY_MODES.adaptive}`, () => { + const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); + expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.adaptive); + }); + + describe("rateLimiter init", () => { + it("sets getDefaultrateLimiter if options is undefined", () => { + const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); + expect(retryStrategy["rateLimiter"]).toBe(mockDefaultRateLimiter); + }); + + it("sets getDefaultrateLimiter if options.delayDecider undefined", () => { + const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider, {}); + expect(retryStrategy["rateLimiter"]).toBe(mockDefaultRateLimiter); + }); + + it("sets options.rateLimiter if defined", () => { + const rateLimiter = {} as RateLimiter; + const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider, { + rateLimiter, + }); + expect(retryStrategy["rateLimiter"]).toBe(rateLimiter); + }); + }); + }); + + describe("beforeRequest", () => { + it("calls rateLimiter.getSendToken", async () => { + const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); + await retryStrategy["beforeRequest"](); + expect(mockDefaultRateLimiter.getSendToken).toHaveBeenCalledTimes(1); + }); + }); + + describe("afterRequest", () => { + it("calls rateLimiter.updateClientSendingRate", async () => { + const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); + retryStrategy["afterRequest"]({}); + expect(mockDefaultRateLimiter.updateClientSendingRate).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/packages/middleware-retry/src/StandardRetryStrategy.spec.ts b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts index 76a30f5b33909..08438f14be214 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts @@ -2,7 +2,7 @@ import { HttpRequest } from "@aws-sdk/protocol-http"; import { isThrottlingError } from "@aws-sdk/service-error-classification"; import { v4 } from "uuid"; -import { DEFAULT_MAX_ATTEMPTS } from "./configurations"; +import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./configurations"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, THROTTLING_RETRY_DELAY_BASE } from "./constants"; import { getDefaultRetryQuota } from "./defaultRetryQuota"; import { defaultDelayDecider } from "./delayDecider"; @@ -102,6 +102,11 @@ describe("defaultStrategy", () => { }); }); + it(`sets mode=${RETRY_MODES.standard}`, () => { + const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); + expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.standard); + }); + it("handles non-standard errors", () => { const nonStandardErrors = [undefined, "foo", { foo: "bar" }, 123, false, null]; const maxAttempts = 1; From b8d7d166d43e604848e61cfb8c8bf3f964b9a453 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 1 Jun 2021 03:58:41 +0000 Subject: [PATCH 06/10] chore: export AdaptiveRetryStrategy --- packages/middleware-retry/src/index.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/middleware-retry/src/index.ts b/packages/middleware-retry/src/index.ts index 889eb954cb554..86094d7c04e83 100644 --- a/packages/middleware-retry/src/index.ts +++ b/packages/middleware-retry/src/index.ts @@ -1,6 +1,7 @@ export * from "./retryMiddleware"; export * from "./omitRetryHeadersMiddleware"; export * from "./StandardRetryStrategy"; +export * from "./AdaptiveRetryStrategy"; export * from "./configurations"; export * from "./delayDecider"; export * from "./retryDecider"; From 0432fcec5f714d0a3417709cd9aed94fe7c2dd5a Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 1 Jun 2021 14:02:50 +0000 Subject: [PATCH 07/10] fix(middleware-retry): circular dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $ madge --warning --circular --extensions js dist/cjs/ Processed 12 files (426ms) ✖ Found 2 circular dependencies! 1) AdaptiveRetryStrategy.js > StandardRetryStrategy.js > configurations.js 2) StandardRetryStrategy.js > configurations.js --- .../src/AdaptiveRetryStrategy.spec.ts | 2 +- .../src/AdaptiveRetryStrategy.ts | 2 +- .../src/StandardRetryStrategy.spec.ts | 24 +++++++++---------- .../src/StandardRetryStrategy.ts | 2 +- packages/middleware-retry/src/config.ts | 15 ++++++++++++ .../src/configurations.spec.ts | 2 +- .../middleware-retry/src/configurations.ts | 17 +------------ packages/middleware-retry/src/index.ts | 1 + 8 files changed, 33 insertions(+), 32 deletions(-) create mode 100644 packages/middleware-retry/src/config.ts diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts index 7a263d05ba5d8..c8f6a5e50999e 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts @@ -1,5 +1,5 @@ import { AdaptiveRetryStrategy } from "./AdaptiveRetryStrategy"; -import { RETRY_MODES } from "./configurations"; +import { RETRY_MODES } from "./config"; import { DefaultRateLimiter } from "./DefaultRateLimiter"; import { StandardRetryStrategy } from "./StandardRetryStrategy"; import { RateLimiter, RetryQuota } from "./types"; diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts index 9e5f72e5f0003..44268d96f0a0f 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts @@ -1,6 +1,6 @@ import { Provider } from "@aws-sdk/types"; -import { RETRY_MODES } from "./configurations"; +import { RETRY_MODES } from "./config"; import { DefaultRateLimiter } from "./DefaultRateLimiter"; import { StandardRetryStrategy, StandardRetryStrategyOptions } from "./StandardRetryStrategy"; import { RateLimiter } from "./types"; diff --git a/packages/middleware-retry/src/StandardRetryStrategy.spec.ts b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts index 08438f14be214..cd62a1043b359 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts @@ -2,7 +2,7 @@ import { HttpRequest } from "@aws-sdk/protocol-http"; import { isThrottlingError } from "@aws-sdk/service-error-classification"; import { v4 } from "uuid"; -import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./configurations"; +import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./config"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, THROTTLING_RETRY_DELAY_BASE } from "./constants"; import { getDefaultRetryQuota } from "./defaultRetryQuota"; import { defaultDelayDecider } from "./delayDecider"; @@ -85,7 +85,7 @@ describe("defaultStrategy", () => { (defaultDelayDecider as jest.Mock).mockReturnValue(0); (defaultRetryDecider as jest.Mock).mockReturnValue(true); (getDefaultRetryQuota as jest.Mock).mockReturnValue(mockDefaultRetryQuota); - (HttpRequest as unknown as jest.Mock).mockReturnValue({ + ((HttpRequest as unknown) as jest.Mock).mockReturnValue({ isInstance: jest.fn().mockReturnValue(false), }); (v4 as jest.Mock).mockReturnValue("42"); @@ -234,8 +234,8 @@ describe("defaultStrategy", () => { expect(defaultDelayDecider as jest.Mock).toHaveBeenCalledTimes(maxAttempts - 1); expect(setTimeout).toHaveBeenCalledTimes(maxAttempts - 1); - expect((setTimeout as unknown as jest.Mock).mock.calls[0][1]).toBe(FIRST_DELAY); - expect((setTimeout as unknown as jest.Mock).mock.calls[1][1]).toBe(SECOND_DELAY); + expect(((setTimeout as unknown) as jest.Mock).mock.calls[0][1]).toBe(FIRST_DELAY); + expect(((setTimeout as unknown) as jest.Mock).mock.calls[1][1]).toBe(SECOND_DELAY); }); }); @@ -405,7 +405,7 @@ describe("defaultStrategy", () => { it("uses a unique header for every SDK operation invocation", async () => { const { isInstance } = HttpRequest; - (isInstance as unknown as jest.Mock).mockReturnValue(true); + ((isInstance as unknown) as jest.Mock).mockReturnValue(true); const uuidForInvocationOne = "uuid-invocation-1"; const uuidForInvocationTwo = "uuid-invocation-2"; @@ -424,12 +424,12 @@ describe("defaultStrategy", () => { expect(next.mock.calls[0][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocationOne); expect(next.mock.calls[1][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocationTwo); - (isInstance as unknown as jest.Mock).mockReturnValue(false); + ((isInstance as unknown) as jest.Mock).mockReturnValue(false); }); it("uses same value for additional HTTP requests associated with an SDK operation", async () => { const { isInstance } = HttpRequest; - (isInstance as unknown as jest.Mock).mockReturnValueOnce(true); + ((isInstance as unknown) as jest.Mock).mockReturnValueOnce(true); const uuidForInvocation = "uuid-invocation-1"; (v4 as jest.Mock).mockReturnValueOnce(uuidForInvocation); @@ -440,7 +440,7 @@ describe("defaultStrategy", () => { expect(next.mock.calls[0][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocation); expect(next.mock.calls[1][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocation); - (isInstance as unknown as jest.Mock).mockReturnValue(false); + ((isInstance as unknown) as jest.Mock).mockReturnValue(false); }); }); @@ -471,7 +471,7 @@ describe("defaultStrategy", () => { it("adds header for each attempt", async () => { const { isInstance } = HttpRequest; - (isInstance as unknown as jest.Mock).mockReturnValue(true); + ((isInstance as unknown) as jest.Mock).mockReturnValue(true); const mockError = new Error("mockError"); next = jest.fn((args) => { @@ -491,14 +491,14 @@ describe("defaultStrategy", () => { } expect(next).toHaveBeenCalledTimes(maxAttempts); - (isInstance as unknown as jest.Mock).mockReturnValue(false); + ((isInstance as unknown) as jest.Mock).mockReturnValue(false); }); }); describe("defaults maxAttempts to DEFAULT_MAX_ATTEMPTS", () => { it("when maxAttemptsProvider throws error", async () => { const { isInstance } = HttpRequest; - (isInstance as unknown as jest.Mock).mockReturnValue(true); + ((isInstance as unknown) as jest.Mock).mockReturnValue(true); next = jest.fn((args) => { expect(args.request.headers["amz-sdk-request"]).toBe(`attempt=1; max=${DEFAULT_MAX_ATTEMPTS}`); @@ -512,7 +512,7 @@ describe("defaultStrategy", () => { await retryStrategy.retry(next, { request: { headers: {} } } as any); expect(next).toHaveBeenCalledTimes(1); - (isInstance as unknown as jest.Mock).mockReturnValue(false); + ((isInstance as unknown) as jest.Mock).mockReturnValue(false); }); }); }); diff --git a/packages/middleware-retry/src/StandardRetryStrategy.ts b/packages/middleware-retry/src/StandardRetryStrategy.ts index f4fb495e8287d..5e5515b0c2232 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.ts @@ -4,7 +4,7 @@ import { SdkError } from "@aws-sdk/smithy-client"; import { FinalizeHandler, FinalizeHandlerArguments, MetadataBearer, Provider, RetryStrategy } from "@aws-sdk/types"; import { v4 } from "uuid"; -import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./configurations"; +import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./config"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, diff --git a/packages/middleware-retry/src/config.ts b/packages/middleware-retry/src/config.ts new file mode 100644 index 0000000000000..5039f438776e2 --- /dev/null +++ b/packages/middleware-retry/src/config.ts @@ -0,0 +1,15 @@ +export enum RETRY_MODES { + standard = "standard", + adaptive = "adaptive", +} + +/** + * The default value for how many HTTP requests an SDK should make for a + * single SDK operation invocation before giving up + */ +export const DEFAULT_MAX_ATTEMPTS = 3; + +/** + * The default retry algorithm to use. + */ +export const DEFAULT_RETRY_MODE = RETRY_MODES.standard; diff --git a/packages/middleware-retry/src/configurations.spec.ts b/packages/middleware-retry/src/configurations.spec.ts index ebe1b384fe2cc..0f0a7a31d0d28 100644 --- a/packages/middleware-retry/src/configurations.spec.ts +++ b/packages/middleware-retry/src/configurations.spec.ts @@ -1,6 +1,6 @@ +import { DEFAULT_MAX_ATTEMPTS } from "./config"; import { CONFIG_MAX_ATTEMPTS, - DEFAULT_MAX_ATTEMPTS, ENV_MAX_ATTEMPTS, NODE_MAX_ATTEMPT_CONFIG_OPTIONS, resolveRetryConfig, diff --git a/packages/middleware-retry/src/configurations.ts b/packages/middleware-retry/src/configurations.ts index 0034308524c20..3fb9a8f5f9a55 100644 --- a/packages/middleware-retry/src/configurations.ts +++ b/packages/middleware-retry/src/configurations.ts @@ -1,27 +1,12 @@ import { LoadedConfigSelectors } from "@aws-sdk/node-config-provider"; import { Provider, RetryStrategy } from "@aws-sdk/types"; +import { DEFAULT_MAX_ATTEMPTS, DEFAULT_RETRY_MODE } from "./config"; import { StandardRetryStrategy } from "./StandardRetryStrategy"; export const ENV_MAX_ATTEMPTS = "AWS_MAX_ATTEMPTS"; export const CONFIG_MAX_ATTEMPTS = "max_attempts"; -export enum RETRY_MODES { - standard = "standard", - adaptive = "adaptive", -} - -/** - * The default value for how many HTTP requests an SDK should make for a - * single SDK operation invocation before giving up - */ -export const DEFAULT_MAX_ATTEMPTS = 3; - -/** - * The default retry algorithm to use. - */ -export const DEFAULT_RETRY_MODE = RETRY_MODES.standard; - export const NODE_MAX_ATTEMPT_CONFIG_OPTIONS: LoadedConfigSelectors = { environmentVariableSelector: (env) => { const value = env[ENV_MAX_ATTEMPTS]; diff --git a/packages/middleware-retry/src/index.ts b/packages/middleware-retry/src/index.ts index 86094d7c04e83..45562797a295b 100644 --- a/packages/middleware-retry/src/index.ts +++ b/packages/middleware-retry/src/index.ts @@ -2,6 +2,7 @@ export * from "./retryMiddleware"; export * from "./omitRetryHeadersMiddleware"; export * from "./StandardRetryStrategy"; export * from "./AdaptiveRetryStrategy"; +export * from "./config"; export * from "./configurations"; export * from "./delayDecider"; export * from "./retryDecider"; From b2040946bcaaf6cd0c2813a9dc8570f6a17b0728 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 1 Jun 2021 20:43:18 +0000 Subject: [PATCH 08/10] chore: change beforeRequest/afterRequest to callbacks --- .../src/AdaptiveRetryStrategy.ts | 18 ++++++++++----- .../src/StandardRetryStrategy.ts | 22 +++++++++---------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts index 44268d96f0a0f..6fa834dd13812 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts @@ -1,4 +1,4 @@ -import { Provider } from "@aws-sdk/types"; +import { FinalizeHandler, FinalizeHandlerArguments, MetadataBearer, Provider } from "@aws-sdk/types"; import { RETRY_MODES } from "./config"; import { DefaultRateLimiter } from "./DefaultRateLimiter"; @@ -22,13 +22,21 @@ export class AdaptiveRetryStrategy extends StandardRetryStrategy { this.mode = RETRY_MODES.adaptive; } - protected async beforeRequest() { - await super.beforeRequest(); + async retry( + next: FinalizeHandler, + args: FinalizeHandlerArguments + ) { + return super.retry(next, args, { + beforeRequest: this.beforeRequest, + afterRequest: this.afterRequest, + }); + } + + private async beforeRequest() { await this.rateLimiter.getSendToken(); } - protected afterRequest(response: any) { - super.afterRequest(response); + private afterRequest(response: any) { this.rateLimiter.updateClientSendingRate(response); } } diff --git a/packages/middleware-retry/src/StandardRetryStrategy.ts b/packages/middleware-retry/src/StandardRetryStrategy.ts index 5e5515b0c2232..754e1aa6721e1 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.ts @@ -54,7 +54,11 @@ export class StandardRetryStrategy implements RetryStrategy { async retry( next: FinalizeHandler, - args: FinalizeHandlerArguments + args: FinalizeHandlerArguments, + options?: { + beforeRequest: Function; + afterRequest: Function; + } ) { let retryTokenAmount; let attempts = 0; @@ -73,9 +77,13 @@ export class StandardRetryStrategy implements RetryStrategy { request.headers[REQUEST_HEADER] = `attempt=${attempts + 1}; max=${maxAttempts}`; } - await this.beforeRequest(); + if (options?.beforeRequest) { + await options.beforeRequest(); + } const { response, output } = await next(args); - this.afterRequest(response); + if (options?.afterRequest) { + options.afterRequest(response); + } this.retryQuota.releaseRetryTokens(retryTokenAmount); output.$metadata.attempts = attempts + 1; @@ -107,14 +115,6 @@ export class StandardRetryStrategy implements RetryStrategy { } } } - - protected async beforeRequest() { - // No-op for standard mode - } - - protected afterRequest(response: any) { - // No-op for standard mode - } } const asSdkError = (error: unknown): SdkError => { From 878c6178748f66d545e530c517979d98ae69e941 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 1 Jun 2021 20:46:09 +0000 Subject: [PATCH 09/10] chore: use CAPS for standard and adaptive enum keys --- .../src/AdaptiveRetryStrategy.spec.ts | 4 +-- .../src/AdaptiveRetryStrategy.ts | 2 +- .../src/StandardRetryStrategy.spec.ts | 26 +++++++++---------- .../src/StandardRetryStrategy.ts | 2 +- packages/middleware-retry/src/config.ts | 6 ++--- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts index c8f6a5e50999e..7189b00f63f3d 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts @@ -42,9 +42,9 @@ describe(AdaptiveRetryStrategy.name, () => { }); }); - it(`sets mode=${RETRY_MODES.adaptive}`, () => { + it(`sets mode=${RETRY_MODES.ADAPTIVE}`, () => { const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); - expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.adaptive); + expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.ADAPTIVE); }); describe("rateLimiter init", () => { diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts index 6fa834dd13812..31f08aafea142 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts @@ -19,7 +19,7 @@ export class AdaptiveRetryStrategy extends StandardRetryStrategy { const { rateLimiter, ...superOptions } = options ?? {}; super(maxAttemptsProvider, superOptions); this.rateLimiter = rateLimiter ?? new DefaultRateLimiter(); - this.mode = RETRY_MODES.adaptive; + this.mode = RETRY_MODES.ADAPTIVE; } async retry( diff --git a/packages/middleware-retry/src/StandardRetryStrategy.spec.ts b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts index cd62a1043b359..f5089137dc0c1 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.spec.ts @@ -85,7 +85,7 @@ describe("defaultStrategy", () => { (defaultDelayDecider as jest.Mock).mockReturnValue(0); (defaultRetryDecider as jest.Mock).mockReturnValue(true); (getDefaultRetryQuota as jest.Mock).mockReturnValue(mockDefaultRetryQuota); - ((HttpRequest as unknown) as jest.Mock).mockReturnValue({ + (HttpRequest as unknown as jest.Mock).mockReturnValue({ isInstance: jest.fn().mockReturnValue(false), }); (v4 as jest.Mock).mockReturnValue("42"); @@ -102,9 +102,9 @@ describe("defaultStrategy", () => { }); }); - it(`sets mode=${RETRY_MODES.standard}`, () => { + it(`sets mode=${RETRY_MODES.STANDARD}`, () => { const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); - expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.standard); + expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.STANDARD); }); it("handles non-standard errors", () => { @@ -234,8 +234,8 @@ describe("defaultStrategy", () => { expect(defaultDelayDecider as jest.Mock).toHaveBeenCalledTimes(maxAttempts - 1); expect(setTimeout).toHaveBeenCalledTimes(maxAttempts - 1); - expect(((setTimeout as unknown) as jest.Mock).mock.calls[0][1]).toBe(FIRST_DELAY); - expect(((setTimeout as unknown) as jest.Mock).mock.calls[1][1]).toBe(SECOND_DELAY); + expect((setTimeout as unknown as jest.Mock).mock.calls[0][1]).toBe(FIRST_DELAY); + expect((setTimeout as unknown as jest.Mock).mock.calls[1][1]).toBe(SECOND_DELAY); }); }); @@ -405,7 +405,7 @@ describe("defaultStrategy", () => { it("uses a unique header for every SDK operation invocation", async () => { const { isInstance } = HttpRequest; - ((isInstance as unknown) as jest.Mock).mockReturnValue(true); + (isInstance as unknown as jest.Mock).mockReturnValue(true); const uuidForInvocationOne = "uuid-invocation-1"; const uuidForInvocationTwo = "uuid-invocation-2"; @@ -424,12 +424,12 @@ describe("defaultStrategy", () => { expect(next.mock.calls[0][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocationOne); expect(next.mock.calls[1][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocationTwo); - ((isInstance as unknown) as jest.Mock).mockReturnValue(false); + (isInstance as unknown as jest.Mock).mockReturnValue(false); }); it("uses same value for additional HTTP requests associated with an SDK operation", async () => { const { isInstance } = HttpRequest; - ((isInstance as unknown) as jest.Mock).mockReturnValueOnce(true); + (isInstance as unknown as jest.Mock).mockReturnValueOnce(true); const uuidForInvocation = "uuid-invocation-1"; (v4 as jest.Mock).mockReturnValueOnce(uuidForInvocation); @@ -440,7 +440,7 @@ describe("defaultStrategy", () => { expect(next.mock.calls[0][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocation); expect(next.mock.calls[1][0].request.headers["amz-sdk-invocation-id"]).toBe(uuidForInvocation); - ((isInstance as unknown) as jest.Mock).mockReturnValue(false); + (isInstance as unknown as jest.Mock).mockReturnValue(false); }); }); @@ -471,7 +471,7 @@ describe("defaultStrategy", () => { it("adds header for each attempt", async () => { const { isInstance } = HttpRequest; - ((isInstance as unknown) as jest.Mock).mockReturnValue(true); + (isInstance as unknown as jest.Mock).mockReturnValue(true); const mockError = new Error("mockError"); next = jest.fn((args) => { @@ -491,14 +491,14 @@ describe("defaultStrategy", () => { } expect(next).toHaveBeenCalledTimes(maxAttempts); - ((isInstance as unknown) as jest.Mock).mockReturnValue(false); + (isInstance as unknown as jest.Mock).mockReturnValue(false); }); }); describe("defaults maxAttempts to DEFAULT_MAX_ATTEMPTS", () => { it("when maxAttemptsProvider throws error", async () => { const { isInstance } = HttpRequest; - ((isInstance as unknown) as jest.Mock).mockReturnValue(true); + (isInstance as unknown as jest.Mock).mockReturnValue(true); next = jest.fn((args) => { expect(args.request.headers["amz-sdk-request"]).toBe(`attempt=1; max=${DEFAULT_MAX_ATTEMPTS}`); @@ -512,7 +512,7 @@ describe("defaultStrategy", () => { await retryStrategy.retry(next, { request: { headers: {} } } as any); expect(next).toHaveBeenCalledTimes(1); - ((isInstance as unknown) as jest.Mock).mockReturnValue(false); + (isInstance as unknown as jest.Mock).mockReturnValue(false); }); }); }); diff --git a/packages/middleware-retry/src/StandardRetryStrategy.ts b/packages/middleware-retry/src/StandardRetryStrategy.ts index 754e1aa6721e1..6d13bafde436d 100644 --- a/packages/middleware-retry/src/StandardRetryStrategy.ts +++ b/packages/middleware-retry/src/StandardRetryStrategy.ts @@ -30,7 +30,7 @@ export class StandardRetryStrategy implements RetryStrategy { private retryDecider: RetryDecider; private delayDecider: DelayDecider; private retryQuota: RetryQuota; - public mode: string = RETRY_MODES.standard; + public mode: string = RETRY_MODES.STANDARD; constructor(private readonly maxAttemptsProvider: Provider, options?: StandardRetryStrategyOptions) { this.retryDecider = options?.retryDecider ?? defaultRetryDecider; diff --git a/packages/middleware-retry/src/config.ts b/packages/middleware-retry/src/config.ts index 5039f438776e2..96e4f9169cfbd 100644 --- a/packages/middleware-retry/src/config.ts +++ b/packages/middleware-retry/src/config.ts @@ -1,6 +1,6 @@ export enum RETRY_MODES { - standard = "standard", - adaptive = "adaptive", + STANDARD = "standard", + ADAPTIVE = "adaptive", } /** @@ -12,4 +12,4 @@ export const DEFAULT_MAX_ATTEMPTS = 3; /** * The default retry algorithm to use. */ -export const DEFAULT_RETRY_MODE = RETRY_MODES.standard; +export const DEFAULT_RETRY_MODE = RETRY_MODES.STANDARD; From 4cd2e0dfd55621c0209af794c0e4b5132c8d6af8 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 1 Jun 2021 21:07:41 +0000 Subject: [PATCH 10/10] fix: remove private methods beforeRequest/afterRequest the methods were executed in super with incorrect execution context --- .../src/AdaptiveRetryStrategy.spec.ts | 27 +++++++++++++------ .../src/AdaptiveRetryStrategy.ts | 16 +++++------ 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts index 7189b00f63f3d..27eee8021ffae 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.spec.ts @@ -68,18 +68,29 @@ describe(AdaptiveRetryStrategy.name, () => { }); }); - describe("beforeRequest", () => { - it("calls rateLimiter.getSendToken", async () => { + describe("retry", () => { + const mockedSuperRetry = jest.spyOn(StandardRetryStrategy.prototype, "retry"); + + beforeEach(async () => { + const next = jest.fn(); const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); - await retryStrategy["beforeRequest"](); + await retryStrategy.retry(next, { request: { headers: {} } } as any); + expect(mockedSuperRetry).toHaveBeenCalledTimes(1); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it("calls rateLimiter.getSendToken in beforeRequest", async () => { + expect(mockDefaultRateLimiter.getSendToken).toHaveBeenCalledTimes(0); + await mockedSuperRetry.mock.calls[0][2].beforeRequest(); expect(mockDefaultRateLimiter.getSendToken).toHaveBeenCalledTimes(1); }); - }); - describe("afterRequest", () => { - it("calls rateLimiter.updateClientSendingRate", async () => { - const retryStrategy = new AdaptiveRetryStrategy(maxAttemptsProvider); - retryStrategy["afterRequest"]({}); + it("calls rateLimiter.updateClientSendingRate in afterRequest", async () => { + expect(mockDefaultRateLimiter.updateClientSendingRate).toHaveBeenCalledTimes(0); + await mockedSuperRetry.mock.calls[0][2].afterRequest(); expect(mockDefaultRateLimiter.updateClientSendingRate).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts index 31f08aafea142..31423fdce1c35 100644 --- a/packages/middleware-retry/src/AdaptiveRetryStrategy.ts +++ b/packages/middleware-retry/src/AdaptiveRetryStrategy.ts @@ -27,16 +27,12 @@ export class AdaptiveRetryStrategy extends StandardRetryStrategy { args: FinalizeHandlerArguments ) { return super.retry(next, args, { - beforeRequest: this.beforeRequest, - afterRequest: this.afterRequest, + beforeRequest: async () => { + return this.rateLimiter.getSendToken(); + }, + afterRequest: (response: any) => { + this.rateLimiter.updateClientSendingRate(response); + }, }); } - - private async beforeRequest() { - await this.rateLimiter.getSendToken(); - } - - private afterRequest(response: any) { - this.rateLimiter.updateClientSendingRate(response); - } }