From 2577a22a54a88afe7fee695c6795b2361cbb72c7 Mon Sep 17 00:00:00 2001 From: George Fu Date: Wed, 24 May 2023 19:59:31 +0000 Subject: [PATCH 1/7] fix(util-retry): use token instances with shared token availability --- .../src/StandardRetryStrategy.spec.ts | 11 ++--- .../util-retry/src/StandardRetryStrategy.ts | 8 ++-- .../util-retry/src/defaultRetryToken.spec.ts | 45 +++++++++++-------- packages/util-retry/src/defaultRetryToken.ts | 16 ++++--- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/packages/util-retry/src/StandardRetryStrategy.spec.ts b/packages/util-retry/src/StandardRetryStrategy.spec.ts index 409ed71c6ea6..9914eecf769f 100644 --- a/packages/util-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/util-retry/src/StandardRetryStrategy.spec.ts @@ -37,18 +37,13 @@ describe(StandardRetryStrategy.name, () => { expect(retryStrategy.mode).toStrictEqual(RETRY_MODES.STANDARD); }); - describe("retryToken init", () => { - it("sets retryToken", () => { - const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); - expect(retryStrategy["retryToken"]).toBe(getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE)); - }); - }); - describe("acquireInitialRetryToken", () => { it("returns default retryToken", async () => { const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); const retryToken = await retryStrategy.acquireInitialRetryToken(retryTokenScope); - expect(retryToken).toEqual(getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE)); + expect(retryToken).toEqual( + getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE) + ); }); }); diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index 1e15cacf87ca..4ecca4833042 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -9,18 +9,17 @@ import { getDefaultRetryToken } from "./defaultRetryToken"; */ export class StandardRetryStrategy implements RetryStrategyV2 { public readonly mode: string = RETRY_MODES.STANDARD; - private retryToken: StandardRetryToken; + private availableCapacityRef = { availableCapacity: INITIAL_RETRY_TOKENS }; private readonly maxAttemptsProvider: Provider; constructor(maxAttempts: number); constructor(maxAttemptsProvider: Provider); constructor(private readonly maxAttempts: number | Provider) { - this.retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); this.maxAttemptsProvider = typeof maxAttempts === "function" ? maxAttempts : async () => maxAttempts; } public async acquireInitialRetryToken(retryTokenScope: string): Promise { - return this.retryToken; + return getDefaultRetryToken(this.availableCapacityRef, DEFAULT_RETRY_DELAY_BASE); } public async refreshRetryTokenForRetry( @@ -37,11 +36,10 @@ export class StandardRetryStrategy implements RetryStrategyV2 { } public recordSuccess(token: StandardRetryToken): void { - this.retryToken.releaseRetryTokens(token.getLastRetryCost()); + token.releaseRetryTokens(token.getLastRetryCost()); } private async getMaxAttempts() { - let maxAttempts: number; try { return await this.maxAttemptsProvider(); } catch (error) { diff --git a/packages/util-retry/src/defaultRetryToken.spec.ts b/packages/util-retry/src/defaultRetryToken.spec.ts index b0c43c8a8ef2..4f748e984a7d 100644 --- a/packages/util-retry/src/defaultRetryToken.spec.ts +++ b/packages/util-retry/src/defaultRetryToken.spec.ts @@ -22,7 +22,7 @@ describe("defaultRetryToken", () => { error: RetryErrorInfo, initialRetryTokens: number = INITIAL_RETRY_TOKENS ) => { - const retryToken = getDefaultRetryToken(initialRetryTokens, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: initialRetryTokens }, DEFAULT_RETRY_DELAY_BASE); let availableCapacity = initialRetryTokens; while (availableCapacity >= targetCapacity) { retryToken.getRetryTokenCount(error); @@ -49,13 +49,13 @@ describe("defaultRetryToken", () => { describe("custom initial retry tokens", () => { it("hasRetryTokens returns false if capacity is not available", () => { const customRetryTokens = 5; - const retryToken = getDefaultRetryToken(customRetryTokens, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: customRetryTokens }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.hasRetryTokens(transientErrorType)).toBe(false); }); it("retrieveRetryToken throws error if retry tokens not available", () => { const customRetryTokens = 5; - const retryToken = getDefaultRetryToken(customRetryTokens, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: customRetryTokens }, DEFAULT_RETRY_DELAY_BASE); expect(() => { retryToken.getRetryTokenCount({ errorType: transientErrorType }); }).toThrowError(new Error("No retry token available")); @@ -65,12 +65,12 @@ describe("defaultRetryToken", () => { describe("hasRetryTokens", () => { describe("returns true if capacity is available", () => { it("when it's transient error", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.hasRetryTokens(transientErrorType)).toBe(true); }); it("when it's not transient error", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.hasRetryTokens(nonTransientErrorType)).toBe(true); }); }); @@ -91,13 +91,13 @@ describe("defaultRetryToken", () => { describe("retrieveRetryToken", () => { describe("returns retry tokens amount if available", () => { it("when it's transient error", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.getRetryTokenCount({ errorType: transientErrorType })).toBe(TIMEOUT_RETRY_COST); expect(retryToken.getLastRetryCost()).toBe(TIMEOUT_RETRY_COST); }); it("when it's not transient error", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.getRetryTokenCount({ errorType: nonTransientErrorType })).toBe(RETRY_COST); expect(retryToken.getLastRetryCost()).toBe(RETRY_COST); }); @@ -122,12 +122,12 @@ describe("defaultRetryToken", () => { describe("getLastRetryCost", () => { it("is undefined before an error is encountered", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.getLastRetryCost()).toBeUndefined(); }); it("is updated with successive errors", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); retryToken.getRetryTokenCount({ errorType: transientErrorType }); expect(retryToken.getLastRetryCost()).toBe(TIMEOUT_RETRY_COST); retryToken.getRetryTokenCount({ errorType: nonTransientErrorType }); @@ -137,18 +137,22 @@ describe("defaultRetryToken", () => { describe("getRetryCount", () => { it("returns 0 when count is not set", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.getRetryCount()).toBe(0); }); it("returns amount set when token is created", () => { const retryCount = 3; - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE, retryCount); + const retryToken = getDefaultRetryToken( + { availableCapacity: INITIAL_RETRY_TOKENS }, + DEFAULT_RETRY_DELAY_BASE, + retryCount + ); expect(retryToken.getRetryCount()).toBe(retryCount); }); it("increments when retries occur", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE, 1); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE, 1); expect(retryToken.getRetryCount()).toBe(1); retryToken.getRetryTokenCount({ errorType: transientErrorType }); expect(retryToken.getRetryCount()).toBe(2); @@ -159,7 +163,7 @@ describe("defaultRetryToken", () => { describe("getRetryDelay", () => { it("returns initial delay", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); expect(retryToken.getRetryDelay()).toBe(DEFAULT_RETRY_DELAY_BASE); }); @@ -175,7 +179,7 @@ describe("defaultRetryToken", () => { setDelayBase, }; (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); [0, 1, 2, 3].forEach((attempts) => { const mockDelayBase = 100; const expectedDelay = Math.floor(2 ** attempts * mockDelayBase); @@ -199,7 +203,7 @@ describe("defaultRetryToken", () => { setDelayBase, }; (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); [0, 1, 2, 3].forEach((attempts) => { const mockDelayBase = 500; const expectedDelay = Math.floor(2 ** attempts * mockDelayBase); @@ -213,7 +217,10 @@ describe("defaultRetryToken", () => { describe(`caps retry delay at ${MAXIMUM_RETRY_DELAY / 1000} seconds`, () => { it("when value exceeded because of high delayBase", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE * 1000); + const retryToken = getDefaultRetryToken( + { availableCapacity: INITIAL_RETRY_TOKENS }, + DEFAULT_RETRY_DELAY_BASE * 1000 + ); expect(retryToken.getRetryDelay()).toBe(MAXIMUM_RETRY_DELAY); }); @@ -226,7 +233,7 @@ describe("defaultRetryToken", () => { (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); const largeAttemptsNumber = Math.ceil(Math.log2(MAXIMUM_RETRY_DELAY)); const retryToken = getDefaultRetryToken( - INITIAL_RETRY_TOKENS * largeAttemptsNumber, + { availableCapacity: INITIAL_RETRY_TOKENS * largeAttemptsNumber }, DEFAULT_RETRY_DELAY_BASE, largeAttemptsNumber ); @@ -236,7 +243,7 @@ describe("defaultRetryToken", () => { }); it("uses retry-after hint", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); // 5 minutes, greater than maximum allowed for normal retry. const expectedDelay = 5 * 60 * 1000; const retryAfterHint = new Date(Date.now() + expectedDelay); @@ -283,7 +290,7 @@ describe("defaultRetryToken", () => { }); it("ensures availableCapacity is maxed at INITIAL_RETRY_TOKENS", () => { - const retryToken = getDefaultRetryToken(INITIAL_RETRY_TOKENS, DEFAULT_RETRY_DELAY_BASE); + const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); const { errorType } = { errorType: nonTransientErrorType }; // release 100 tokens. diff --git a/packages/util-retry/src/defaultRetryToken.ts b/packages/util-retry/src/defaultRetryToken.ts index 479fafaa0935..14e2f1fe3001 100644 --- a/packages/util-retry/src/defaultRetryToken.ts +++ b/packages/util-retry/src/defaultRetryToken.ts @@ -35,17 +35,18 @@ export interface DefaultRetryTokenOptions { * @internal */ export const getDefaultRetryToken = ( - initialRetryTokens: number, + availableCapacityRef: { + availableCapacity: number; + }, initialRetryDelay: number, initialRetryCount?: number, options?: DefaultRetryTokenOptions ): StandardRetryToken => { - const MAX_CAPACITY = initialRetryTokens; + const MAX_CAPACITY = availableCapacityRef.availableCapacity; const retryCost = options?.retryCost ?? RETRY_COST; const timeoutRetryCost = options?.timeoutRetryCost ?? TIMEOUT_RETRY_COST; const retryBackoffStrategy = options?.retryBackoffStrategy ?? getDefaultRetryBackoffStrategy(); - let availableCapacity = initialRetryTokens; let retryDelay = Math.min(MAXIMUM_RETRY_DELAY, initialRetryDelay); let lastRetryCost: number | undefined = undefined; let retryCount = initialRetryCount ?? 0; @@ -58,7 +59,8 @@ export const getDefaultRetryToken = ( const getLastRetryCost = (): number | undefined => lastRetryCost; - const hasRetryTokens = (errorType: RetryErrorType): boolean => getCapacityAmount(errorType) <= availableCapacity; + const hasRetryTokens = (errorType: RetryErrorType): boolean => + getCapacityAmount(errorType) <= availableCapacityRef.availableCapacity; const getRetryTokenCount = (errorInfo: RetryErrorInfo) => { const errorType = errorInfo.errorType; @@ -77,13 +79,13 @@ export const getDefaultRetryToken = ( } retryCount++; lastRetryCost = capacityAmount; - availableCapacity -= capacityAmount; + availableCapacityRef.availableCapacity -= capacityAmount; return capacityAmount; }; const releaseRetryTokens = (releaseAmount?: number) => { - availableCapacity += releaseAmount ?? NO_RETRY_INCREMENT; - availableCapacity = Math.min(availableCapacity, MAX_CAPACITY); + availableCapacityRef.availableCapacity += releaseAmount ?? NO_RETRY_INCREMENT; + availableCapacityRef.availableCapacity = Math.min(availableCapacityRef.availableCapacity, MAX_CAPACITY); }; return { From 61da9afb1004ee59a1c9639fb54be387c4bb5bd6 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 25 May 2023 17:10:45 +0000 Subject: [PATCH 2/7] fix(util-retry): remove mutations in standard retry tokens --- .../src/AdaptiveRetryStrategy.spec.ts | 2 +- .../src/ConfiguredRetryStrategy.spec.ts | 4 +- .../src/StandardRetryStrategy.spec.ts | 30 +- .../util-retry/src/StandardRetryStrategy.ts | 50 +++- .../util-retry/src/defaultRetryToken.spec.ts | 259 +++++------------- packages/util-retry/src/defaultRetryToken.ts | 92 ++----- .../src/ClientRetryTest.spec.ts | 52 +++- 7 files changed, 204 insertions(+), 285 deletions(-) diff --git a/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts b/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts index b28ad91bdbdd..b1a3d0b7891a 100644 --- a/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts +++ b/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts @@ -19,7 +19,7 @@ describe(AdaptiveRetryStrategy.name, () => { const mockRetryToken: StandardRetryToken = { hasRetryTokens: (errorType: RetryErrorType) => true, getLastRetryCost: () => 1, - getRetryTokenCount: (errorInfo: RetryErrorInfo) => 1, + getRetryTokenCount: (errorInfo?: RetryErrorInfo) => 1, releaseRetryTokens: (amount: number) => {}, getRetryCount: () => 1, getRetryDelay: () => 1, diff --git a/packages/util-retry/src/ConfiguredRetryStrategy.spec.ts b/packages/util-retry/src/ConfiguredRetryStrategy.spec.ts index f080d7c7dc31..4d1638e35a77 100644 --- a/packages/util-retry/src/ConfiguredRetryStrategy.spec.ts +++ b/packages/util-retry/src/ConfiguredRetryStrategy.spec.ts @@ -11,7 +11,7 @@ describe(ConfiguredRetryStrategy.name, () => { errorType: "TRANSIENT", }); - expect(retryToken.getRetryCount()).toBe(4); - expect(retryToken.getRetryDelay()).toBe(4000); + expect(retryToken.getRetryCount()).toBe(5); + expect(retryToken.getRetryDelay()).toBe(5000); }); }); diff --git a/packages/util-retry/src/StandardRetryStrategy.spec.ts b/packages/util-retry/src/StandardRetryStrategy.spec.ts index 9914eecf769f..a520a285a480 100644 --- a/packages/util-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/util-retry/src/StandardRetryStrategy.spec.ts @@ -2,7 +2,7 @@ import { RetryErrorInfo, RetryErrorType } from "@aws-sdk/types"; import { RETRY_MODES } from "./config"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS } from "./constants"; -import { getDefaultRetryToken } from "./defaultRetryToken"; +import { createDefaultRetryToken } from "./defaultRetryToken"; import { StandardRetryStrategy } from "./StandardRetryStrategy"; jest.mock("./defaultRetryToken"); @@ -18,7 +18,7 @@ describe(StandardRetryStrategy.name, () => { const errorInfo = { errorType: "TRANSIENT" } as RetryErrorInfo; beforeEach(() => { - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); }); afterEach(() => { @@ -42,28 +42,28 @@ describe(StandardRetryStrategy.name, () => { const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); const retryToken = await retryStrategy.acquireInitialRetryToken(retryTokenScope); expect(retryToken).toEqual( - getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE) + createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }) ); }); }); describe("refreshRetryTokenForRetry", () => { it("refreshes the token", async () => { - const getRetryTokenCount = jest.fn().mockReturnValue(1); const getRetryCount = jest.fn().mockReturnValue(0); const hasRetryTokens = jest.fn().mockReturnValue(true); const mockRetryToken = { getRetryCount, - getRetryTokenCount, hasRetryTokens, }; - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); - const refreshedToken = await retryStrategy.refreshRetryTokenForRetry(token, errorInfo); - expect(getRetryTokenCount).toHaveBeenCalledTimes(1); - expect(getRetryTokenCount).toHaveBeenCalledWith(errorInfo); - expect(getRetryCount).toHaveBeenCalledTimes(1); + await retryStrategy.refreshRetryTokenForRetry(token, errorInfo); + expect(getRetryCount).toHaveBeenCalledTimes(3); expect(hasRetryTokens).toHaveBeenCalledTimes(1); expect(hasRetryTokens).toHaveBeenCalledWith(errorInfo.errorType); }); @@ -73,7 +73,7 @@ describe(StandardRetryStrategy.name, () => { getRetryCount: () => 2, getRetryTokenCount: (errorInfo: any) => 1, }; - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(1)); const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); try { @@ -88,7 +88,7 @@ describe(StandardRetryStrategy.name, () => { getRetryCount: () => 5, getRetryTokenCount: (errorInfo: any) => 1, }; - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(5)); const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); try { @@ -104,7 +104,7 @@ describe(StandardRetryStrategy.name, () => { getRetryTokenCount: (errorInfo: any) => 1, hasRetryTokens: (errorType: RetryErrorType) => false, }; - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); try { @@ -120,7 +120,7 @@ describe(StandardRetryStrategy.name, () => { getRetryTokenCount: (errorInfo: any) => 1, hasRetryTokens: (errorType: RetryErrorType) => true, }; - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); const errorInfo = { @@ -142,7 +142,7 @@ describe(StandardRetryStrategy.name, () => { releaseRetryTokens, getLastRetryCost, }; - (getDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); + (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); retryStrategy.recordSuccess(token); diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index 4ecca4833042..9eab2dd24143 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -1,15 +1,23 @@ import { Provider, RetryErrorInfo, RetryErrorType, RetryStrategyV2, StandardRetryToken } from "@aws-sdk/types"; import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./config"; -import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS } from "./constants"; -import { getDefaultRetryToken } from "./defaultRetryToken"; +import { + DEFAULT_RETRY_DELAY_BASE, + INITIAL_RETRY_TOKENS, + RETRY_COST, + THROTTLING_RETRY_DELAY_BASE, + TIMEOUT_RETRY_COST, +} from "./constants"; +import { getDefaultRetryBackoffStrategy } from "./defaultRetryBackoffStrategy"; +import { createDefaultRetryToken } from "./defaultRetryToken"; /** * @public */ export class StandardRetryStrategy implements RetryStrategyV2 { public readonly mode: string = RETRY_MODES.STANDARD; - private availableCapacityRef = { availableCapacity: INITIAL_RETRY_TOKENS }; + private readonly capacityBucket = { availableCapacity: INITIAL_RETRY_TOKENS }; + private readonly retryBackoffStrategy = getDefaultRetryBackoffStrategy(); private readonly maxAttemptsProvider: Provider; constructor(maxAttempts: number); @@ -19,19 +27,45 @@ export class StandardRetryStrategy implements RetryStrategyV2 { } public async acquireInitialRetryToken(retryTokenScope: string): Promise { - return getDefaultRetryToken(this.availableCapacityRef, DEFAULT_RETRY_DELAY_BASE); + return createDefaultRetryToken({ + capacityBucket: this.capacityBucket, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }); } public async refreshRetryTokenForRetry( - tokenToRenew: StandardRetryToken, + token: StandardRetryToken, errorInfo: RetryErrorInfo ): Promise { const maxAttempts = await this.getMaxAttempts(); - if (this.shouldRetry(tokenToRenew, errorInfo, maxAttempts)) { - tokenToRenew.getRetryTokenCount(errorInfo); - return tokenToRenew; + const retryCost = RETRY_COST; + const timeoutRetryCost = TIMEOUT_RETRY_COST; + const getCapacityCost = (errorType: RetryErrorType) => (errorType === "TRANSIENT" ? timeoutRetryCost : retryCost); + + if (this.shouldRetry(token, errorInfo, maxAttempts)) { + const errorType = errorInfo.errorType; + const capacityCost = getCapacityCost(errorType); + const delayBase = errorType === "THROTTLING" ? THROTTLING_RETRY_DELAY_BASE : DEFAULT_RETRY_DELAY_BASE; + this.retryBackoffStrategy.setDelayBase(delayBase); + const delayFromErrorType = this.retryBackoffStrategy.computeNextBackoffDelay(token.getRetryCount()); + let retryDelay: number; + if (errorInfo.retryAfterHint) { + const delayFromRetryAfterHint = errorInfo.retryAfterHint.getTime() - Date.now(); + retryDelay = Math.max(delayFromRetryAfterHint || 0, delayFromErrorType); + } else { + retryDelay = delayFromErrorType; + } + this.capacityBucket.availableCapacity -= capacityCost; + return createDefaultRetryToken({ + capacityBucket: this.capacityBucket, + retryDelay, + retryCount: token.getRetryCount() + 1, + lastRetryCost: capacityCost, + }); } + throw new Error("No retry token available"); } diff --git a/packages/util-retry/src/defaultRetryToken.spec.ts b/packages/util-retry/src/defaultRetryToken.spec.ts index 4f748e984a7d..946c9cd41007 100644 --- a/packages/util-retry/src/defaultRetryToken.spec.ts +++ b/packages/util-retry/src/defaultRetryToken.spec.ts @@ -1,4 +1,4 @@ -import { RetryErrorInfo, RetryErrorType, SdkError } from "@aws-sdk/types"; +import { RetryErrorType } from "@aws-sdk/types"; import { DEFAULT_RETRY_DELAY_BASE, @@ -6,10 +6,9 @@ import { MAXIMUM_RETRY_DELAY, NO_RETRY_INCREMENT, RETRY_COST, - TIMEOUT_RETRY_COST, } from "./constants"; import { getDefaultRetryBackoffStrategy } from "./defaultRetryBackoffStrategy"; -import { getDefaultRetryToken } from "./defaultRetryToken"; +import { createDefaultRetryToken } from "./defaultRetryToken"; jest.mock("./defaultRetryBackoffStrategy"); @@ -17,17 +16,12 @@ describe("defaultRetryToken", () => { const transientErrorType = "TRANSIENT" as RetryErrorType; const nonTransientErrorType = "THROTTLING" as RetryErrorType; - const getDrainedRetryToken = ( - targetCapacity: number, - error: RetryErrorInfo, - initialRetryTokens: number = INITIAL_RETRY_TOKENS - ) => { - const retryToken = getDefaultRetryToken({ availableCapacity: initialRetryTokens }, DEFAULT_RETRY_DELAY_BASE); - let availableCapacity = initialRetryTokens; - while (availableCapacity >= targetCapacity) { - retryToken.getRetryTokenCount(error); - availableCapacity -= targetCapacity; - } + const getDrainedRetryToken = (targetCapacity: number) => { + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: targetCapacity }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }); return retryToken; }; const mathDotRandom = Math.random; @@ -49,219 +43,113 @@ describe("defaultRetryToken", () => { describe("custom initial retry tokens", () => { it("hasRetryTokens returns false if capacity is not available", () => { const customRetryTokens = 5; - const retryToken = getDefaultRetryToken({ availableCapacity: customRetryTokens }, DEFAULT_RETRY_DELAY_BASE); - expect(retryToken.hasRetryTokens(transientErrorType)).toBe(false); - }); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: customRetryTokens }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }); - it("retrieveRetryToken throws error if retry tokens not available", () => { - const customRetryTokens = 5; - const retryToken = getDefaultRetryToken({ availableCapacity: customRetryTokens }, DEFAULT_RETRY_DELAY_BASE); - expect(() => { - retryToken.getRetryTokenCount({ errorType: transientErrorType }); - }).toThrowError(new Error("No retry token available")); + expect(retryToken.hasRetryTokens(transientErrorType)).toBe(false); }); }); describe("hasRetryTokens", () => { describe("returns true if capacity is available", () => { it("when it's transient error", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }); expect(retryToken.hasRetryTokens(transientErrorType)).toBe(true); }); it("when it's not transient error", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }); expect(retryToken.hasRetryTokens(nonTransientErrorType)).toBe(true); }); }); - describe("returns false if capacity is not available", () => { - it("when it's transient error", () => { - const retryToken = getDrainedRetryToken(TIMEOUT_RETRY_COST, { errorType: transientErrorType }); - expect(retryToken.hasRetryTokens(transientErrorType)).toBe(false); - }); - - it("when it's not transient error", () => { - const retryToken = getDrainedRetryToken(RETRY_COST, { errorType: nonTransientErrorType }); - expect(retryToken.hasRetryTokens(nonTransientErrorType)).toBe(false); - }); + it("returns false if capacity is not available", () => { + const retryToken = getDrainedRetryToken(0); + expect(retryToken.hasRetryTokens(nonTransientErrorType)).toBe(false); }); }); - describe("retrieveRetryToken", () => { - describe("returns retry tokens amount if available", () => { - it("when it's transient error", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - expect(retryToken.getRetryTokenCount({ errorType: transientErrorType })).toBe(TIMEOUT_RETRY_COST); - expect(retryToken.getLastRetryCost()).toBe(TIMEOUT_RETRY_COST); - }); - - it("when it's not transient error", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - expect(retryToken.getRetryTokenCount({ errorType: nonTransientErrorType })).toBe(RETRY_COST); - expect(retryToken.getLastRetryCost()).toBe(RETRY_COST); - }); - }); - - describe("throws error if retry tokens not available", () => { - it("when it's transient error", () => { - const retryToken = getDrainedRetryToken(TIMEOUT_RETRY_COST, { errorType: transientErrorType }); - expect(() => { - retryToken.getRetryTokenCount({ errorType: transientErrorType }); - }).toThrowError(new Error("No retry token available")); - }); - - it("when it's not transient error", () => { - const retryToken = getDrainedRetryToken(RETRY_COST, { errorType: nonTransientErrorType }); - expect(() => { - retryToken.getRetryTokenCount({ errorType: nonTransientErrorType }); - }).toThrowError(new Error("No retry token available")); + describe("getRetryTokenCount", () => { + it("returns retry tokens amount", () => { + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: 123 }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, }); + expect(retryToken.getRetryTokenCount()).toBe(123); }); }); describe("getLastRetryCost", () => { it("is undefined before an error is encountered", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + }); expect(retryToken.getLastRetryCost()).toBeUndefined(); }); - it("is updated with successive errors", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - retryToken.getRetryTokenCount({ errorType: transientErrorType }); - expect(retryToken.getLastRetryCost()).toBe(TIMEOUT_RETRY_COST); - retryToken.getRetryTokenCount({ errorType: nonTransientErrorType }); - expect(retryToken.getLastRetryCost()).toBe(RETRY_COST); + it("returns set value", () => { + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, + lastRetryCost: 25, + }); + expect(retryToken.getLastRetryCost()).toBe(25); }); }); describe("getRetryCount", () => { - it("returns 0 when count is not set", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - expect(retryToken.getRetryCount()).toBe(0); - }); - it("returns amount set when token is created", () => { const retryCount = 3; - const retryToken = getDefaultRetryToken( - { availableCapacity: INITIAL_RETRY_TOKENS }, - DEFAULT_RETRY_DELAY_BASE, - retryCount - ); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount, + }); expect(retryToken.getRetryCount()).toBe(retryCount); }); - - it("increments when retries occur", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE, 1); - expect(retryToken.getRetryCount()).toBe(1); - retryToken.getRetryTokenCount({ errorType: transientErrorType }); - expect(retryToken.getRetryCount()).toBe(2); - retryToken.getRetryTokenCount({ errorType: nonTransientErrorType }); - expect(retryToken.getRetryCount()).toBe(3); - }); }); describe("getRetryDelay", () => { it("returns initial delay", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - expect(retryToken.getRetryDelay()).toBe(DEFAULT_RETRY_DELAY_BASE); - }); - - describe("retry delay increases exponentially with attempt number for non-throttling error", () => { - const computeNextBackoffDelay = jest - .fn() - .mockReturnValueOnce(100) - .mockReturnValueOnce(200) - .mockReturnValueOnce(400) - .mockReturnValueOnce(800); - const mockRetryBackoffStrategy = { - computeNextBackoffDelay, - setDelayBase, - }; - (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - [0, 1, 2, 3].forEach((attempts) => { - const mockDelayBase = 100; - const expectedDelay = Math.floor(2 ** attempts * mockDelayBase); - it(`(${mockDelayBase}, ${attempts}) returns ${expectedDelay}`, () => { - retryToken.getRetryTokenCount({ errorType: transientErrorType }); - expect(retryToken.getRetryDelay()).toBe(expectedDelay); - expect(computeNextBackoffDelay).toHaveBeenCalledTimes(attempts + 1); - }); - }); - }); - - describe("retry delay increases exponentially with attempt number for throttling error", () => { - const computeNextBackoffDelay = jest - .fn() - .mockReturnValueOnce(500) - .mockReturnValueOnce(1000) - .mockReturnValueOnce(2000) - .mockReturnValueOnce(4000); - const mockRetryBackoffStrategy = { - computeNextBackoffDelay, - setDelayBase, - }; - (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - [0, 1, 2, 3].forEach((attempts) => { - const mockDelayBase = 500; - const expectedDelay = Math.floor(2 ** attempts * mockDelayBase); - it(`(${mockDelayBase}, ${attempts}) returns ${expectedDelay}`, () => { - retryToken.getRetryTokenCount({ errorType: nonTransientErrorType }); - expect(retryToken.getRetryDelay()).toBe(expectedDelay); - expect(computeNextBackoffDelay).toHaveBeenCalledTimes(attempts + 1); - }); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, }); + expect(retryToken.getRetryDelay()).toBe(DEFAULT_RETRY_DELAY_BASE); }); describe(`caps retry delay at ${MAXIMUM_RETRY_DELAY / 1000} seconds`, () => { it("when value exceeded because of high delayBase", () => { - const retryToken = getDefaultRetryToken( - { availableCapacity: INITIAL_RETRY_TOKENS }, - DEFAULT_RETRY_DELAY_BASE * 1000 - ); - expect(retryToken.getRetryDelay()).toBe(MAXIMUM_RETRY_DELAY); - }); - - it("when value exceeded because of high attempts number", () => { - const computeNextBackoffDelay = jest.fn().mockReturnValue(MAXIMUM_RETRY_DELAY); - const mockRetryBackoffStrategy = { - computeNextBackoffDelay, - setDelayBase, - }; - (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); - const largeAttemptsNumber = Math.ceil(Math.log2(MAXIMUM_RETRY_DELAY)); - const retryToken = getDefaultRetryToken( - { availableCapacity: INITIAL_RETRY_TOKENS * largeAttemptsNumber }, - DEFAULT_RETRY_DELAY_BASE, - largeAttemptsNumber - ); - retryToken.getRetryTokenCount({ errorType: transientErrorType }); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + retryDelay: DEFAULT_RETRY_DELAY_BASE * 1000, + retryCount: 0, + }); expect(retryToken.getRetryDelay()).toBe(MAXIMUM_RETRY_DELAY); }); }); - - it("uses retry-after hint", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - // 5 minutes, greater than maximum allowed for normal retry. - const expectedDelay = 5 * 60 * 1000; - const retryAfterHint = new Date(Date.now() + expectedDelay); - const errorInfo: RetryErrorInfo = { - errorType: "TRANSIENT", - retryAfterHint, - }; - retryToken.getRetryTokenCount(errorInfo); - // Subtract small offset on expectedDelay to account for delta to when - // Date.now() is invoked. - expect(retryToken.getRetryDelay()).toBeGreaterThan(expectedDelay - 50); - }); }); describe("releaseRetryToken", () => { it("adds capacityReleaseAmount if passed", () => { const { errorType } = { errorType: nonTransientErrorType }; - const retryToken = getDrainedRetryToken(RETRY_COST, { errorType: nonTransientErrorType }); + const retryToken = getDrainedRetryToken(0); // Ensure that retry tokens are not available. expect(retryToken.hasRetryTokens(errorType)).toBe(false); @@ -270,12 +158,11 @@ describe("defaultRetryToken", () => { retryToken.releaseRetryTokens(RETRY_COST); expect(retryToken.hasRetryTokens(errorType)).toBe(true); expect(retryToken.getRetryTokenCount({ errorType: nonTransientErrorType })).toBe(RETRY_COST); - expect(retryToken.hasRetryTokens(errorType)).toBe(false); }); it("adds NO_RETRY_INCREMENT if capacityReleaseAmount not passed", () => { const { errorType } = { errorType: nonTransientErrorType }; - const retryToken = getDrainedRetryToken(RETRY_COST, { errorType: nonTransientErrorType }); + const retryToken = getDrainedRetryToken(0); // retry tokens will not be available till NO_RETRY_INCREMENT is added // till it's equal to RETRY_COST - (INITIAL_RETRY_TOKENS % RETRY_COST) @@ -290,21 +177,13 @@ describe("defaultRetryToken", () => { }); it("ensures availableCapacity is maxed at INITIAL_RETRY_TOKENS", () => { - const retryToken = getDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS }, DEFAULT_RETRY_DELAY_BASE); - const { errorType } = { errorType: nonTransientErrorType }; - - // release 100 tokens. - [...Array(100).keys()].forEach(() => { - retryToken.releaseRetryTokens(); + const retryToken = createDefaultRetryToken({ + capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS * 1000 }, + retryDelay: DEFAULT_RETRY_DELAY_BASE, + retryCount: 0, }); - // availableCapacity is still maxed at INITIAL_RETRY_TOKENS - // hasRetryTokens would be true only till INITIAL_RETRY_TOKENS/RETRY_COST times - [...Array(Math.floor(INITIAL_RETRY_TOKENS / RETRY_COST)).keys()].forEach(() => { - expect(retryToken.hasRetryTokens(errorType)).toBe(true); - retryToken.getRetryTokenCount({ errorType: nonTransientErrorType }); - }); - expect(retryToken.hasRetryTokens(errorType)).toBe(false); + expect(retryToken.getRetryTokenCount()).toBe(INITIAL_RETRY_TOKENS); }); }); }); diff --git a/packages/util-retry/src/defaultRetryToken.ts b/packages/util-retry/src/defaultRetryToken.ts index 14e2f1fe3001..cbd1b76c8e3c 100644 --- a/packages/util-retry/src/defaultRetryToken.ts +++ b/packages/util-retry/src/defaultRetryToken.ts @@ -1,91 +1,53 @@ -import { RetryErrorInfo, RetryErrorType, StandardRetryBackoffStrategy, StandardRetryToken } from "@aws-sdk/types"; +import { RetryErrorInfo, RetryErrorType, StandardRetryToken } from "@aws-sdk/types"; import { - DEFAULT_RETRY_DELAY_BASE, + INITIAL_RETRY_TOKENS, MAXIMUM_RETRY_DELAY, NO_RETRY_INCREMENT, RETRY_COST, - THROTTLING_RETRY_DELAY_BASE, TIMEOUT_RETRY_COST, } from "./constants"; -import { getDefaultRetryBackoffStrategy } from "./defaultRetryBackoffStrategy"; - -/** - * @public - */ -export interface DefaultRetryTokenOptions { - /** - * The total amount of retry tokens to be decremented from retry token balance. - */ - retryCost?: number; - - /** - * The total amount of retry tokens to be decremented from retry token balance - * when a throttling error is encountered. - */ - timeoutRetryCost?: number; - - /** - * - */ - retryBackoffStrategy?: StandardRetryBackoffStrategy; -} /** * @internal */ -export const getDefaultRetryToken = ( - availableCapacityRef: { +export const createDefaultRetryToken = ({ + capacityBucket, + retryDelay, + retryCount, + lastRetryCost, +}: { + capacityBucket: { availableCapacity: number; - }, - initialRetryDelay: number, - initialRetryCount?: number, - options?: DefaultRetryTokenOptions -): StandardRetryToken => { - const MAX_CAPACITY = availableCapacityRef.availableCapacity; - const retryCost = options?.retryCost ?? RETRY_COST; - const timeoutRetryCost = options?.timeoutRetryCost ?? TIMEOUT_RETRY_COST; - const retryBackoffStrategy = options?.retryBackoffStrategy ?? getDefaultRetryBackoffStrategy(); - - let retryDelay = Math.min(MAXIMUM_RETRY_DELAY, initialRetryDelay); - let lastRetryCost: number | undefined = undefined; - let retryCount = initialRetryCount ?? 0; + }; + retryDelay: number; + retryCount: number; + lastRetryCost?: number; +}): StandardRetryToken => { + const MAX_CAPACITY = INITIAL_RETRY_TOKENS; - const getCapacityAmount = (errorType: RetryErrorType) => (errorType === "TRANSIENT" ? timeoutRetryCost : retryCost); + const getCapacityAmount = (errorType: RetryErrorType) => + errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; const getRetryCount = (): number => retryCount; - const getRetryDelay = (): number => retryDelay; + const getRetryDelay = (): number => Math.min(MAXIMUM_RETRY_DELAY, retryDelay); const getLastRetryCost = (): number | undefined => lastRetryCost; - const hasRetryTokens = (errorType: RetryErrorType): boolean => - getCapacityAmount(errorType) <= availableCapacityRef.availableCapacity; + const hasRetryTokens = (errorType: RetryErrorType): boolean => { + capacityBucket.availableCapacity = Math.min(capacityBucket.availableCapacity, MAX_CAPACITY); + return getCapacityAmount(errorType) <= capacityBucket.availableCapacity; + }; - const getRetryTokenCount = (errorInfo: RetryErrorInfo) => { - const errorType = errorInfo.errorType; - if (!hasRetryTokens(errorType)) { - throw new Error("No retry token available"); - } - const capacityAmount = getCapacityAmount(errorType); - const delayBase = errorType === "THROTTLING" ? THROTTLING_RETRY_DELAY_BASE : DEFAULT_RETRY_DELAY_BASE; - retryBackoffStrategy.setDelayBase(delayBase); - const delayFromErrorType = retryBackoffStrategy.computeNextBackoffDelay(retryCount); - if (errorInfo.retryAfterHint) { - const delayFromRetryAfterHint = errorInfo.retryAfterHint.getTime() - Date.now(); - retryDelay = Math.max(delayFromRetryAfterHint || 0, delayFromErrorType); - } else { - retryDelay = delayFromErrorType; - } - retryCount++; - lastRetryCost = capacityAmount; - availableCapacityRef.availableCapacity -= capacityAmount; - return capacityAmount; + const getRetryTokenCount = (errorInfo?: RetryErrorInfo) => { + capacityBucket.availableCapacity = Math.min(capacityBucket.availableCapacity, MAX_CAPACITY); + return capacityBucket.availableCapacity; }; const releaseRetryTokens = (releaseAmount?: number) => { - availableCapacityRef.availableCapacity += releaseAmount ?? NO_RETRY_INCREMENT; - availableCapacityRef.availableCapacity = Math.min(availableCapacityRef.availableCapacity, MAX_CAPACITY); + capacityBucket.availableCapacity += releaseAmount ?? NO_RETRY_INCREMENT; + capacityBucket.availableCapacity = Math.min(capacityBucket.availableCapacity, MAX_CAPACITY); }; return { diff --git a/private/aws-client-retry-test/src/ClientRetryTest.spec.ts b/private/aws-client-retry-test/src/ClientRetryTest.spec.ts index 1aa486b2a61e..330865dc7682 100644 --- a/private/aws-client-retry-test/src/ClientRetryTest.spec.ts +++ b/private/aws-client-retry-test/src/ClientRetryTest.spec.ts @@ -1,10 +1,21 @@ -import { HeadObjectCommand, S3Client, S3ServiceException } from "@aws-sdk/client-s3"; +import { HeadObjectCommand, S3, S3Client, S3ServiceException } from "@aws-sdk/client-s3"; import { HttpResponse } from "@aws-sdk/protocol-http"; import { RequestHandlerOutput } from "@aws-sdk/types"; -import { StandardRetryStrategy } from "@aws-sdk/util-retry"; +import { ConfiguredRetryStrategy, StandardRetryStrategy } from "@aws-sdk/util-retry"; import { Readable } from "stream"; -describe("Middleware-retry integration tests", () => { +class MockRequestHandler { + async handle() { + return { + response: new HttpResponse({ + statusCode: 429, + body: Buffer.from(""), + }), + }; + } +} + +describe("util-retry integration tests", () => { const mockThrottled: RequestHandlerOutput = { response: new HttpResponse({ statusCode: 429, @@ -22,6 +33,7 @@ describe("Middleware-retry integration tests", () => { Bucket: "TEST_BUCKET", Key: "TEST_KEY", }); + it("should not retry on 200", async () => { const client = new S3Client({ requestHandler: { @@ -34,6 +46,7 @@ describe("Middleware-retry integration tests", () => { expect(response.$metadata.attempts).toBe(1); expect(response.$metadata.totalRetryDelay).toBe(0); }); + it("should retry until success", async () => { const mockHandle = jest .fn() @@ -52,7 +65,8 @@ describe("Middleware-retry integration tests", () => { expect(response.$metadata.attempts).toBe(3); expect(response.$metadata.totalRetryDelay).toBeGreaterThan(0); }); - it("should retry until attemps are exhausted", async () => { + + it("should retry until attempts are exhausted", async () => { const expectedException = new S3ServiceException({ $metadata: { httpStatusCode: 429, @@ -79,4 +93,34 @@ describe("Middleware-retry integration tests", () => { expect(error.$metadata.totalRetryDelay).toBeGreaterThan(0); } }); + + it("should use a shared capacity for retries", async () => { + const expectedInitialCapacity = 500; + const expectedDrainPerAttempt = 5; + const expectedRetryAttemptsPerRequest = 7; + const delayPerRetry = 1; + const expectedRequests = 4; + const expectedRemainingCapacity = + expectedInitialCapacity - expectedDrainPerAttempt * expectedRetryAttemptsPerRequest * expectedRequests; + + const retryStrategy = new ConfiguredRetryStrategy(expectedRetryAttemptsPerRequest, delayPerRetry); + const s3 = new S3({ + requestHandler: new MockRequestHandler(), + retryStrategy, + }); + + expect((await retryStrategy.acquireInitialRetryToken("")).getRetryTokenCount()).toEqual(expectedInitialCapacity); + + await Promise.all([ + s3.headBucket({ Bucket: "undefined" }), + s3.headBucket({ Bucket: "undefined" }), + s3.headBucket({ Bucket: "undefined" }), + s3.headBucket({ Bucket: "undefined" }), + ]).catch((e) => { + expect(e.$metadata.attempts).toBe(1 + expectedRetryAttemptsPerRequest); + expect(e.$metadata.totalRetryDelay).toBe(expectedRetryAttemptsPerRequest * delayPerRetry); + }); + + expect((await retryStrategy.acquireInitialRetryToken("")).getRetryTokenCount()).toEqual(expectedRemainingCapacity); + }); }); From db177768e3e3b3b57a58499d1667ad45d3ae032a Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 25 May 2023 18:15:12 +0000 Subject: [PATCH 3/7] fix(util-retry): make standard retry token immutable, deprecate some methods on standard retry token --- packages/middleware-retry/src/types.ts | 6 +- packages/types/src/retry.ts | 21 ++++-- .../src/StandardRetryStrategy.spec.ts | 23 +----- .../util-retry/src/StandardRetryStrategy.ts | 26 +++++-- .../util-retry/src/defaultRetryToken.spec.ts | 73 ++++--------------- packages/util-retry/src/defaultRetryToken.ts | 38 ++++------ .../src/ClientRetryTest.spec.ts | 4 +- 7 files changed, 69 insertions(+), 122 deletions(-) diff --git a/packages/middleware-retry/src/types.ts b/packages/middleware-retry/src/types.ts index e6d76b951b7f..445d64db18c8 100644 --- a/packages/middleware-retry/src/types.ts +++ b/packages/middleware-retry/src/types.ts @@ -4,7 +4,7 @@ import { SdkError } from "@aws-sdk/types"; * Determines whether an error is retryable based on the number of retries * already attempted, the HTTP status code, and the error received (if any). * - * @param error The error encountered. + * @param error - The error encountered. */ export interface RetryDecider { (error: SdkError): boolean; @@ -13,8 +13,8 @@ export interface RetryDecider { /** * Determines the number of milliseconds to wait before retrying an action. * - * @param delayBase The base delay (in milliseconds). - * @param attempts The number of times the action has already been tried. + * @param delayBase - The base delay (in milliseconds). + * @param attempts - The number of times the action has already been tried. */ export interface DelayDecider { (delayBase: number, attempts: number): number; diff --git a/packages/types/src/retry.ts b/packages/types/src/retry.ts index 7c856c4afdae..6034cd8abb3b 100644 --- a/packages/types/src/retry.ts +++ b/packages/types/src/retry.ts @@ -95,24 +95,29 @@ export interface RetryToken { */ export interface StandardRetryToken extends RetryToken { /** - * @returns wheather token has remaining tokens. + * @returns the cost of the last retry attempt. */ - hasRetryTokens(errorType: RetryErrorType): boolean; + getLastRetryCost(): number | undefined; /** - * @returns the number of available tokens. + * @deprecated StandardRetryStrategy will track retry capacity. + * + * @param errorType - derives the cost of the retry to check whether capacity remains. + * @returns wheather there is enough remaining capacity to retry the given error. */ - getRetryTokenCount(errorInfo: RetryErrorInfo): number; + hasRetryTokens(errorType: RetryErrorType): boolean; /** - * @returns the cost of the last retry attemp. + * @deprecated StandardRetryStrategy will track retry capacity. + * @returns the number of available tokens. */ - getLastRetryCost(): number | undefined; + getRetryTokenCount(errorInfo: RetryErrorInfo): number; /** - * Releases a number of tokens. + * @deprecated use \@aws-sdk/util-retry's StandardRetryStrategy#recordSuccess. * - * @param amount - of tokens to release. + * Warning: this method is no-op on StandardRetryTokens provided by + * StandardRetryStrategy in \@aws-sdk/util-retry. */ releaseRetryTokens(amount?: number): void; } diff --git a/packages/util-retry/src/StandardRetryStrategy.spec.ts b/packages/util-retry/src/StandardRetryStrategy.spec.ts index a520a285a480..02dee1328f02 100644 --- a/packages/util-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/util-retry/src/StandardRetryStrategy.spec.ts @@ -43,7 +43,7 @@ describe(StandardRetryStrategy.name, () => { const retryToken = await retryStrategy.acquireInitialRetryToken(retryTokenScope); expect(retryToken).toEqual( createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }) @@ -64,8 +64,6 @@ describe(StandardRetryStrategy.name, () => { const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); await retryStrategy.refreshRetryTokenForRetry(token, errorInfo); expect(getRetryCount).toHaveBeenCalledTimes(3); - expect(hasRetryTokens).toHaveBeenCalledTimes(1); - expect(hasRetryTokens).toHaveBeenCalledWith(errorInfo.errorType); }); it("throws when attempts exceeds maxAttempts", async () => { @@ -132,24 +130,5 @@ describe(StandardRetryStrategy.name, () => { expect(error).toStrictEqual(noRetryTokenAvailableError); } }); - - describe("recordSuccess", () => { - it("releases tokens", async () => { - const retryCost = 1; - const releaseRetryTokens = jest.fn(); - const getLastRetryCost = jest.fn().mockReturnValue(retryCost); - const mockRetryToken = { - releaseRetryTokens, - getLastRetryCost, - }; - (createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken); - const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); - const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope); - retryStrategy.recordSuccess(token); - expect(releaseRetryTokens).toHaveBeenCalledTimes(1); - expect(releaseRetryTokens).toHaveBeenCalledWith(retryCost); - expect(getLastRetryCost).toHaveBeenCalledTimes(1); - }); - }); }); }); diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index 9eab2dd24143..681722406d9f 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -4,6 +4,7 @@ import { DEFAULT_MAX_ATTEMPTS, RETRY_MODES } from "./config"; import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, + NO_RETRY_INCREMENT, RETRY_COST, THROTTLING_RETRY_DELAY_BASE, TIMEOUT_RETRY_COST, @@ -16,7 +17,7 @@ import { createDefaultRetryToken } from "./defaultRetryToken"; */ export class StandardRetryStrategy implements RetryStrategyV2 { public readonly mode: string = RETRY_MODES.STANDARD; - private readonly capacityBucket = { availableCapacity: INITIAL_RETRY_TOKENS }; + private capacity: number = INITIAL_RETRY_TOKENS; private readonly retryBackoffStrategy = getDefaultRetryBackoffStrategy(); private readonly maxAttemptsProvider: Provider; @@ -28,7 +29,7 @@ export class StandardRetryStrategy implements RetryStrategyV2 { public async acquireInitialRetryToken(retryTokenScope: string): Promise { return createDefaultRetryToken({ - capacityBucket: this.capacityBucket, + availableCapacity: this.capacity, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -57,9 +58,9 @@ export class StandardRetryStrategy implements RetryStrategyV2 { } else { retryDelay = delayFromErrorType; } - this.capacityBucket.availableCapacity -= capacityCost; + this.capacity -= capacityCost; return createDefaultRetryToken({ - capacityBucket: this.capacityBucket, + availableCapacity: this.capacity, retryDelay, retryCount: token.getRetryCount() + 1, lastRetryCost: capacityCost, @@ -70,7 +71,16 @@ export class StandardRetryStrategy implements RetryStrategyV2 { } public recordSuccess(token: StandardRetryToken): void { - token.releaseRetryTokens(token.getLastRetryCost()); + this.capacity = Math.max(INITIAL_RETRY_TOKENS, this.capacity + (token.getLastRetryCost() ?? NO_RETRY_INCREMENT)); + } + + /** + * @returns the current available retry capacity. + * + * This number decreases when retries are executed and refills when requests or retries succeed. + */ + public getCapacity(): number { + return this.capacity; } private async getMaxAttempts() { @@ -84,9 +94,13 @@ export class StandardRetryStrategy implements RetryStrategyV2 { private shouldRetry(tokenToRenew: StandardRetryToken, errorInfo: RetryErrorInfo, maxAttempts: number): boolean { const attempts = tokenToRenew.getRetryCount(); + + const getCapacityAmount = (errorType: RetryErrorType) => + errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; + return ( attempts < maxAttempts && - tokenToRenew.hasRetryTokens(errorInfo.errorType) && + this.capacity >= getCapacityAmount(errorInfo.errorType) && this.isRetryableError(errorInfo.errorType) ); } diff --git a/packages/util-retry/src/defaultRetryToken.spec.ts b/packages/util-retry/src/defaultRetryToken.spec.ts index 946c9cd41007..0627a9999840 100644 --- a/packages/util-retry/src/defaultRetryToken.spec.ts +++ b/packages/util-retry/src/defaultRetryToken.spec.ts @@ -7,7 +7,6 @@ import { NO_RETRY_INCREMENT, RETRY_COST, } from "./constants"; -import { getDefaultRetryBackoffStrategy } from "./defaultRetryBackoffStrategy"; import { createDefaultRetryToken } from "./defaultRetryToken"; jest.mock("./defaultRetryBackoffStrategy"); @@ -18,33 +17,18 @@ describe("defaultRetryToken", () => { const getDrainedRetryToken = (targetCapacity: number) => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: targetCapacity }, + availableCapacity: targetCapacity, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); return retryToken; }; - const mathDotRandom = Math.random; - const setDelayBase = jest.fn(); - const mockRetryBackoffStrategy = { - computeNextBackoffDelay: (attempts: number) => 100, - setDelayBase, - }; - - beforeEach(() => { - Math.random = jest.fn().mockReturnValue(1); - (getDefaultRetryBackoffStrategy as jest.Mock).mockReturnValue(mockRetryBackoffStrategy); - }); - - afterEach(() => { - Math.random = mathDotRandom; - }); describe("custom initial retry tokens", () => { it("hasRetryTokens returns false if capacity is not available", () => { const customRetryTokens = 5; const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: customRetryTokens }, + availableCapacity: customRetryTokens, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -57,7 +41,7 @@ describe("defaultRetryToken", () => { describe("returns true if capacity is available", () => { it("when it's transient error", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -66,7 +50,7 @@ describe("defaultRetryToken", () => { it("when it's not transient error", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -83,18 +67,18 @@ describe("defaultRetryToken", () => { describe("getRetryTokenCount", () => { it("returns retry tokens amount", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: 123 }, + availableCapacity: 123, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); - expect(retryToken.getRetryTokenCount()).toBe(123); + expect(retryToken.getRetryTokenCount({ errorType: "TRANSIENT" })).toBe(123); }); }); describe("getLastRetryCost", () => { it("is undefined before an error is encountered", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -103,7 +87,7 @@ describe("defaultRetryToken", () => { it("returns set value", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, lastRetryCost: 25, @@ -116,7 +100,7 @@ describe("defaultRetryToken", () => { it("returns amount set when token is created", () => { const retryCount = 3; const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount, }); @@ -127,7 +111,7 @@ describe("defaultRetryToken", () => { describe("getRetryDelay", () => { it("returns initial delay", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -137,7 +121,7 @@ describe("defaultRetryToken", () => { describe(`caps retry delay at ${MAXIMUM_RETRY_DELAY / 1000} seconds`, () => { it("when value exceeded because of high delayBase", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE * 1000, retryCount: 0, }); @@ -147,43 +131,16 @@ describe("defaultRetryToken", () => { }); describe("releaseRetryToken", () => { - it("adds capacityReleaseAmount if passed", () => { - const { errorType } = { errorType: nonTransientErrorType }; - const retryToken = getDrainedRetryToken(0); - - // Ensure that retry tokens are not available. - expect(retryToken.hasRetryTokens(errorType)).toBe(false); - - // Release RETRY_COST tokens. - retryToken.releaseRetryTokens(RETRY_COST); - expect(retryToken.hasRetryTokens(errorType)).toBe(true); - expect(retryToken.getRetryTokenCount({ errorType: nonTransientErrorType })).toBe(RETRY_COST); - }); - - it("adds NO_RETRY_INCREMENT if capacityReleaseAmount not passed", () => { - const { errorType } = { errorType: nonTransientErrorType }; - const retryToken = getDrainedRetryToken(0); - - // retry tokens will not be available till NO_RETRY_INCREMENT is added - // till it's equal to RETRY_COST - (INITIAL_RETRY_TOKENS % RETRY_COST) - let tokensReleased = 0; - const tokensToBeReleased = RETRY_COST - (INITIAL_RETRY_TOKENS % RETRY_COST); - while (tokensReleased < tokensToBeReleased) { - expect(retryToken.hasRetryTokens(errorType)).toBe(false); - retryToken.releaseRetryTokens(); - tokensReleased += NO_RETRY_INCREMENT; - } - expect(retryToken.hasRetryTokens(errorType)).toBe(true); - }); - it("ensures availableCapacity is maxed at INITIAL_RETRY_TOKENS", () => { const retryToken = createDefaultRetryToken({ - capacityBucket: { availableCapacity: INITIAL_RETRY_TOKENS * 1000 }, + availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); - expect(retryToken.getRetryTokenCount()).toBe(INITIAL_RETRY_TOKENS); + retryToken.releaseRetryTokens(); + + expect(retryToken.getRetryTokenCount({ errorType: "TRANSIENT" })).toBe(INITIAL_RETRY_TOKENS); }); }); }); diff --git a/packages/util-retry/src/defaultRetryToken.ts b/packages/util-retry/src/defaultRetryToken.ts index cbd1b76c8e3c..8f06e55f691b 100644 --- a/packages/util-retry/src/defaultRetryToken.ts +++ b/packages/util-retry/src/defaultRetryToken.ts @@ -1,54 +1,46 @@ import { RetryErrorInfo, RetryErrorType, StandardRetryToken } from "@aws-sdk/types"; -import { - INITIAL_RETRY_TOKENS, - MAXIMUM_RETRY_DELAY, - NO_RETRY_INCREMENT, - RETRY_COST, - TIMEOUT_RETRY_COST, -} from "./constants"; +import { MAXIMUM_RETRY_DELAY, RETRY_COST, TIMEOUT_RETRY_COST } from "./constants"; /** * @internal */ export const createDefaultRetryToken = ({ - capacityBucket, + availableCapacity, retryDelay, retryCount, lastRetryCost, }: { - capacityBucket: { - availableCapacity: number; - }; + availableCapacity: number; retryDelay: number; retryCount: number; lastRetryCost?: number; }): StandardRetryToken => { - const MAX_CAPACITY = INITIAL_RETRY_TOKENS; - const getCapacityAmount = (errorType: RetryErrorType) => errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; const getRetryCount = (): number => retryCount; - const getRetryDelay = (): number => Math.min(MAXIMUM_RETRY_DELAY, retryDelay); - const getLastRetryCost = (): number | undefined => lastRetryCost; + /** + * @deprecated managed by StandardRetryStrategy. + */ const hasRetryTokens = (errorType: RetryErrorType): boolean => { - capacityBucket.availableCapacity = Math.min(capacityBucket.availableCapacity, MAX_CAPACITY); - return getCapacityAmount(errorType) <= capacityBucket.availableCapacity; + return getCapacityAmount(errorType) <= availableCapacity; }; + /** + * @deprecated managed by StandardRetryStrategy. + */ const getRetryTokenCount = (errorInfo?: RetryErrorInfo) => { - capacityBucket.availableCapacity = Math.min(capacityBucket.availableCapacity, MAX_CAPACITY); - return capacityBucket.availableCapacity; + return availableCapacity; }; - const releaseRetryTokens = (releaseAmount?: number) => { - capacityBucket.availableCapacity += releaseAmount ?? NO_RETRY_INCREMENT; - capacityBucket.availableCapacity = Math.min(capacityBucket.availableCapacity, MAX_CAPACITY); - }; + /** + * @deprecated this is done in StandardRetryStrategy.recordSuccess. + */ + const releaseRetryTokens = (releaseAmount?: number) => {}; return { getRetryCount, diff --git a/private/aws-client-retry-test/src/ClientRetryTest.spec.ts b/private/aws-client-retry-test/src/ClientRetryTest.spec.ts index 330865dc7682..a4820d4db6a3 100644 --- a/private/aws-client-retry-test/src/ClientRetryTest.spec.ts +++ b/private/aws-client-retry-test/src/ClientRetryTest.spec.ts @@ -109,7 +109,7 @@ describe("util-retry integration tests", () => { retryStrategy, }); - expect((await retryStrategy.acquireInitialRetryToken("")).getRetryTokenCount()).toEqual(expectedInitialCapacity); + expect(retryStrategy.getCapacity()).toEqual(expectedInitialCapacity); await Promise.all([ s3.headBucket({ Bucket: "undefined" }), @@ -121,6 +121,6 @@ describe("util-retry integration tests", () => { expect(e.$metadata.totalRetryDelay).toBe(expectedRetryAttemptsPerRequest * delayPerRetry); }); - expect((await retryStrategy.acquireInitialRetryToken("")).getRetryTokenCount()).toEqual(expectedRemainingCapacity); + expect(retryStrategy.getCapacity()).toEqual(expectedRemainingCapacity); }); }); From 396effa21d21e76053394f936de20d348d0ce497 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 25 May 2023 19:01:01 +0000 Subject: [PATCH 4/7] fix(util-retry): remove deprecated parts of StandardRetryToken --- packages/types/src/retry.ts | 24 +---- .../src/AdaptiveRetryStrategy.spec.ts | 7 +- .../util-retry/src/StandardRetryStrategy.ts | 2 +- .../util-retry/src/defaultRetryToken.spec.ts | 93 +------------------ packages/util-retry/src/defaultRetryToken.ts | 34 +------ 5 files changed, 12 insertions(+), 148 deletions(-) diff --git a/packages/types/src/retry.ts b/packages/types/src/retry.ts index 6034cd8abb3b..ee3ba0b72708 100644 --- a/packages/types/src/retry.ts +++ b/packages/types/src/retry.ts @@ -97,29 +97,7 @@ export interface StandardRetryToken extends RetryToken { /** * @returns the cost of the last retry attempt. */ - getLastRetryCost(): number | undefined; - - /** - * @deprecated StandardRetryStrategy will track retry capacity. - * - * @param errorType - derives the cost of the retry to check whether capacity remains. - * @returns wheather there is enough remaining capacity to retry the given error. - */ - hasRetryTokens(errorType: RetryErrorType): boolean; - - /** - * @deprecated StandardRetryStrategy will track retry capacity. - * @returns the number of available tokens. - */ - getRetryTokenCount(errorInfo: RetryErrorInfo): number; - - /** - * @deprecated use \@aws-sdk/util-retry's StandardRetryStrategy#recordSuccess. - * - * Warning: this method is no-op on StandardRetryTokens provided by - * StandardRetryStrategy in \@aws-sdk/util-retry. - */ - releaseRetryTokens(amount?: number): void; + getRetryCost(): number | undefined; } /** diff --git a/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts b/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts index b1a3d0b7891a..27220d4ca084 100644 --- a/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts +++ b/packages/util-retry/src/AdaptiveRetryStrategy.spec.ts @@ -1,4 +1,4 @@ -import { RetryErrorInfo, RetryErrorType, StandardRetryToken } from "@aws-sdk/types"; +import { RetryErrorInfo, StandardRetryToken } from "@aws-sdk/types"; import { AdaptiveRetryStrategy } from "./AdaptiveRetryStrategy"; import { RETRY_MODES } from "./config"; @@ -17,10 +17,7 @@ describe(AdaptiveRetryStrategy.name, () => { updateClientSendingRate: jest.fn(), }; const mockRetryToken: StandardRetryToken = { - hasRetryTokens: (errorType: RetryErrorType) => true, - getLastRetryCost: () => 1, - getRetryTokenCount: (errorInfo?: RetryErrorInfo) => 1, - releaseRetryTokens: (amount: number) => {}, + getRetryCost: () => 1, getRetryCount: () => 1, getRetryDelay: () => 1, }; diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index 681722406d9f..bf98131c789d 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -71,7 +71,7 @@ export class StandardRetryStrategy implements RetryStrategyV2 { } public recordSuccess(token: StandardRetryToken): void { - this.capacity = Math.max(INITIAL_RETRY_TOKENS, this.capacity + (token.getLastRetryCost() ?? NO_RETRY_INCREMENT)); + this.capacity = Math.max(INITIAL_RETRY_TOKENS, this.capacity + (token.getRetryCost() ?? NO_RETRY_INCREMENT)); } /** diff --git a/packages/util-retry/src/defaultRetryToken.spec.ts b/packages/util-retry/src/defaultRetryToken.spec.ts index 0627a9999840..e4706dddfd86 100644 --- a/packages/util-retry/src/defaultRetryToken.spec.ts +++ b/packages/util-retry/src/defaultRetryToken.spec.ts @@ -1,88 +1,17 @@ -import { RetryErrorType } from "@aws-sdk/types"; - -import { - DEFAULT_RETRY_DELAY_BASE, - INITIAL_RETRY_TOKENS, - MAXIMUM_RETRY_DELAY, - NO_RETRY_INCREMENT, - RETRY_COST, -} from "./constants"; +import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, MAXIMUM_RETRY_DELAY } from "./constants"; import { createDefaultRetryToken } from "./defaultRetryToken"; jest.mock("./defaultRetryBackoffStrategy"); describe("defaultRetryToken", () => { - const transientErrorType = "TRANSIENT" as RetryErrorType; - const nonTransientErrorType = "THROTTLING" as RetryErrorType; - - const getDrainedRetryToken = (targetCapacity: number) => { - const retryToken = createDefaultRetryToken({ - availableCapacity: targetCapacity, - retryDelay: DEFAULT_RETRY_DELAY_BASE, - retryCount: 0, - }); - return retryToken; - }; - - describe("custom initial retry tokens", () => { - it("hasRetryTokens returns false if capacity is not available", () => { - const customRetryTokens = 5; - const retryToken = createDefaultRetryToken({ - availableCapacity: customRetryTokens, - retryDelay: DEFAULT_RETRY_DELAY_BASE, - retryCount: 0, - }); - - expect(retryToken.hasRetryTokens(transientErrorType)).toBe(false); - }); - }); - - describe("hasRetryTokens", () => { - describe("returns true if capacity is available", () => { - it("when it's transient error", () => { - const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, - retryDelay: DEFAULT_RETRY_DELAY_BASE, - retryCount: 0, - }); - expect(retryToken.hasRetryTokens(transientErrorType)).toBe(true); - }); - - it("when it's not transient error", () => { - const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, - retryDelay: DEFAULT_RETRY_DELAY_BASE, - retryCount: 0, - }); - expect(retryToken.hasRetryTokens(nonTransientErrorType)).toBe(true); - }); - }); - - it("returns false if capacity is not available", () => { - const retryToken = getDrainedRetryToken(0); - expect(retryToken.hasRetryTokens(nonTransientErrorType)).toBe(false); - }); - }); - - describe("getRetryTokenCount", () => { - it("returns retry tokens amount", () => { - const retryToken = createDefaultRetryToken({ - availableCapacity: 123, - retryDelay: DEFAULT_RETRY_DELAY_BASE, - retryCount: 0, - }); - expect(retryToken.getRetryTokenCount({ errorType: "TRANSIENT" })).toBe(123); - }); - }); - - describe("getLastRetryCost", () => { + describe("getRetryCost", () => { it("is undefined before an error is encountered", () => { const retryToken = createDefaultRetryToken({ availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); - expect(retryToken.getLastRetryCost()).toBeUndefined(); + expect(retryToken.getRetryCost()).toBeUndefined(); }); it("returns set value", () => { @@ -92,7 +21,7 @@ describe("defaultRetryToken", () => { retryCount: 0, lastRetryCost: 25, }); - expect(retryToken.getLastRetryCost()).toBe(25); + expect(retryToken.getRetryCost()).toBe(25); }); }); @@ -129,18 +58,4 @@ describe("defaultRetryToken", () => { }); }); }); - - describe("releaseRetryToken", () => { - it("ensures availableCapacity is maxed at INITIAL_RETRY_TOKENS", () => { - const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, - retryDelay: DEFAULT_RETRY_DELAY_BASE, - retryCount: 0, - }); - - retryToken.releaseRetryTokens(); - - expect(retryToken.getRetryTokenCount({ errorType: "TRANSIENT" })).toBe(INITIAL_RETRY_TOKENS); - }); - }); }); diff --git a/packages/util-retry/src/defaultRetryToken.ts b/packages/util-retry/src/defaultRetryToken.ts index 8f06e55f691b..367f4350f0ff 100644 --- a/packages/util-retry/src/defaultRetryToken.ts +++ b/packages/util-retry/src/defaultRetryToken.ts @@ -1,12 +1,11 @@ -import { RetryErrorInfo, RetryErrorType, StandardRetryToken } from "@aws-sdk/types"; +import { StandardRetryToken } from "@aws-sdk/types"; -import { MAXIMUM_RETRY_DELAY, RETRY_COST, TIMEOUT_RETRY_COST } from "./constants"; +import { MAXIMUM_RETRY_DELAY } from "./constants"; /** * @internal */ export const createDefaultRetryToken = ({ - availableCapacity, retryDelay, retryCount, lastRetryCost, @@ -16,38 +15,13 @@ export const createDefaultRetryToken = ({ retryCount: number; lastRetryCost?: number; }): StandardRetryToken => { - const getCapacityAmount = (errorType: RetryErrorType) => - errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; - const getRetryCount = (): number => retryCount; const getRetryDelay = (): number => Math.min(MAXIMUM_RETRY_DELAY, retryDelay); - const getLastRetryCost = (): number | undefined => lastRetryCost; - - /** - * @deprecated managed by StandardRetryStrategy. - */ - const hasRetryTokens = (errorType: RetryErrorType): boolean => { - return getCapacityAmount(errorType) <= availableCapacity; - }; - - /** - * @deprecated managed by StandardRetryStrategy. - */ - const getRetryTokenCount = (errorInfo?: RetryErrorInfo) => { - return availableCapacity; - }; - - /** - * @deprecated this is done in StandardRetryStrategy.recordSuccess. - */ - const releaseRetryTokens = (releaseAmount?: number) => {}; + const getRetryCost = (): number | undefined => lastRetryCost; return { getRetryCount, getRetryDelay, - getLastRetryCost, - hasRetryTokens, - getRetryTokenCount, - releaseRetryTokens, + getRetryCost, }; }; From 4fcaff1812d08dddae4655b894deac3ff37388ef Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Thu, 25 May 2023 13:38:01 -0700 Subject: [PATCH 5/7] chore: remove redundant variables retryCost and timeoutRetryCost --- packages/util-retry/src/StandardRetryStrategy.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index bf98131c789d..9eeb5ec050e3 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -41,9 +41,8 @@ export class StandardRetryStrategy implements RetryStrategyV2 { ): Promise { const maxAttempts = await this.getMaxAttempts(); - const retryCost = RETRY_COST; - const timeoutRetryCost = TIMEOUT_RETRY_COST; - const getCapacityCost = (errorType: RetryErrorType) => (errorType === "TRANSIENT" ? timeoutRetryCost : retryCost); + const getCapacityCost = (errorType: RetryErrorType) => + errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; if (this.shouldRetry(token, errorInfo, maxAttempts)) { const errorType = errorInfo.errorType; From cdbe6a0ec53d649dcbfd81f377bc2a51d655464f Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 25 May 2023 16:57:24 -0400 Subject: [PATCH 6/7] fix(util-retry): packages/util-retry/src/StandardRetryStrategy.ts Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> --- .../util-retry/src/StandardRetryStrategy.ts | 31 +++++++++---------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index 9eeb5ec050e3..af0b981574f9 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -41,22 +41,18 @@ export class StandardRetryStrategy implements RetryStrategyV2 { ): Promise { const maxAttempts = await this.getMaxAttempts(); - const getCapacityCost = (errorType: RetryErrorType) => - errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; - if (this.shouldRetry(token, errorInfo, maxAttempts)) { const errorType = errorInfo.errorType; - const capacityCost = getCapacityCost(errorType); - const delayBase = errorType === "THROTTLING" ? THROTTLING_RETRY_DELAY_BASE : DEFAULT_RETRY_DELAY_BASE; - this.retryBackoffStrategy.setDelayBase(delayBase); + this.retryBackoffStrategy.setDelayBase( + errorType === "THROTTLING" ? THROTTLING_RETRY_DELAY_BASE : DEFAULT_RETRY_DELAY_BASE + ); + const delayFromErrorType = this.retryBackoffStrategy.computeNextBackoffDelay(token.getRetryCount()); - let retryDelay: number; - if (errorInfo.retryAfterHint) { - const delayFromRetryAfterHint = errorInfo.retryAfterHint.getTime() - Date.now(); - retryDelay = Math.max(delayFromRetryAfterHint || 0, delayFromErrorType); - } else { - retryDelay = delayFromErrorType; - } + const retryDelay = errorInfo.retryAfterHint + ? Math.max(errorInfo.retryAfterHint.getTime() - Date.now() || 0, delayFromErrorType) + : delayFromErrorType; + + const capacityCost = this.getCapacityCost(errorType); this.capacity -= capacityCost; return createDefaultRetryToken({ availableCapacity: this.capacity, @@ -94,16 +90,17 @@ export class StandardRetryStrategy implements RetryStrategyV2 { private shouldRetry(tokenToRenew: StandardRetryToken, errorInfo: RetryErrorInfo, maxAttempts: number): boolean { const attempts = tokenToRenew.getRetryCount(); - const getCapacityAmount = (errorType: RetryErrorType) => - errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; - return ( attempts < maxAttempts && - this.capacity >= getCapacityAmount(errorInfo.errorType) && + this.capacity >= this.getCapacityCost(errorInfo.errorType) && this.isRetryableError(errorInfo.errorType) ); } + private getCapacityCost(errorType: RetryErrorType) { + return errorType === "TRANSIENT" ? TIMEOUT_RETRY_COST : RETRY_COST; + } + private isRetryableError(errorType: RetryErrorType): boolean { return errorType === "THROTTLING" || errorType === "TRANSIENT"; } From 32f0a8895cb9233143dc7389a88b8f644d1efd77 Mon Sep 17 00:00:00 2001 From: George Fu Date: Thu, 25 May 2023 21:08:03 +0000 Subject: [PATCH 7/7] fix(util-retry): remove unnecessary input --- packages/util-retry/src/StandardRetryStrategy.spec.ts | 1 - packages/util-retry/src/StandardRetryStrategy.ts | 4 +--- packages/util-retry/src/defaultRetryToken.spec.ts | 9 ++------- packages/util-retry/src/defaultRetryToken.ts | 7 +++---- 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/packages/util-retry/src/StandardRetryStrategy.spec.ts b/packages/util-retry/src/StandardRetryStrategy.spec.ts index 02dee1328f02..cd6bd3e153d4 100644 --- a/packages/util-retry/src/StandardRetryStrategy.spec.ts +++ b/packages/util-retry/src/StandardRetryStrategy.spec.ts @@ -43,7 +43,6 @@ describe(StandardRetryStrategy.name, () => { const retryToken = await retryStrategy.acquireInitialRetryToken(retryTokenScope); expect(retryToken).toEqual( createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }) diff --git a/packages/util-retry/src/StandardRetryStrategy.ts b/packages/util-retry/src/StandardRetryStrategy.ts index af0b981574f9..05140f3462f7 100644 --- a/packages/util-retry/src/StandardRetryStrategy.ts +++ b/packages/util-retry/src/StandardRetryStrategy.ts @@ -29,7 +29,6 @@ export class StandardRetryStrategy implements RetryStrategyV2 { public async acquireInitialRetryToken(retryTokenScope: string): Promise { return createDefaultRetryToken({ - availableCapacity: this.capacity, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -55,10 +54,9 @@ export class StandardRetryStrategy implements RetryStrategyV2 { const capacityCost = this.getCapacityCost(errorType); this.capacity -= capacityCost; return createDefaultRetryToken({ - availableCapacity: this.capacity, retryDelay, retryCount: token.getRetryCount() + 1, - lastRetryCost: capacityCost, + retryCost: capacityCost, }); } diff --git a/packages/util-retry/src/defaultRetryToken.spec.ts b/packages/util-retry/src/defaultRetryToken.spec.ts index e4706dddfd86..e0604d4a79c1 100644 --- a/packages/util-retry/src/defaultRetryToken.spec.ts +++ b/packages/util-retry/src/defaultRetryToken.spec.ts @@ -1,4 +1,4 @@ -import { DEFAULT_RETRY_DELAY_BASE, INITIAL_RETRY_TOKENS, MAXIMUM_RETRY_DELAY } from "./constants"; +import { DEFAULT_RETRY_DELAY_BASE, MAXIMUM_RETRY_DELAY } from "./constants"; import { createDefaultRetryToken } from "./defaultRetryToken"; jest.mock("./defaultRetryBackoffStrategy"); @@ -7,7 +7,6 @@ describe("defaultRetryToken", () => { describe("getRetryCost", () => { it("is undefined before an error is encountered", () => { const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -16,10 +15,9 @@ describe("defaultRetryToken", () => { it("returns set value", () => { const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, - lastRetryCost: 25, + retryCost: 25, }); expect(retryToken.getRetryCost()).toBe(25); }); @@ -29,7 +27,6 @@ describe("defaultRetryToken", () => { it("returns amount set when token is created", () => { const retryCount = 3; const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount, }); @@ -40,7 +37,6 @@ describe("defaultRetryToken", () => { describe("getRetryDelay", () => { it("returns initial delay", () => { const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE, retryCount: 0, }); @@ -50,7 +46,6 @@ describe("defaultRetryToken", () => { describe(`caps retry delay at ${MAXIMUM_RETRY_DELAY / 1000} seconds`, () => { it("when value exceeded because of high delayBase", () => { const retryToken = createDefaultRetryToken({ - availableCapacity: INITIAL_RETRY_TOKENS, retryDelay: DEFAULT_RETRY_DELAY_BASE * 1000, retryCount: 0, }); diff --git a/packages/util-retry/src/defaultRetryToken.ts b/packages/util-retry/src/defaultRetryToken.ts index 367f4350f0ff..e5ab316ae2d4 100644 --- a/packages/util-retry/src/defaultRetryToken.ts +++ b/packages/util-retry/src/defaultRetryToken.ts @@ -8,16 +8,15 @@ import { MAXIMUM_RETRY_DELAY } from "./constants"; export const createDefaultRetryToken = ({ retryDelay, retryCount, - lastRetryCost, + retryCost, }: { - availableCapacity: number; retryDelay: number; retryCount: number; - lastRetryCost?: number; + retryCost?: number; }): StandardRetryToken => { const getRetryCount = (): number => retryCount; const getRetryDelay = (): number => Math.min(MAXIMUM_RETRY_DELAY, retryDelay); - const getRetryCost = (): number | undefined => lastRetryCost; + const getRetryCost = (): number | undefined => retryCost; return { getRetryCount,