Skip to content

Commit

Permalink
fix(util-retry): make standard retry token immutable, deprecate some …
Browse files Browse the repository at this point in the history 
…methods on standard retry token
  • Loading branch information
@kuhe
kuhe committed May 25, 2023
1 parent b20eff1 commit 3b4ad9a
Show file tree
Hide file tree
Showing 6 changed files with 66 additions and 120 deletions.
6 changes: 3 additions & 3 deletions packages/middleware-retry/src/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
21 changes: 13 additions & 8 deletions packages/types/src/retry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
23 changes: 1 addition & 22 deletions packages/util-retry/src/StandardRetryStrategy.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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);
});
});
});
});
25 changes: 19 additions & 6 deletions packages/util-retry/src/StandardRetryStrategy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,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<number>;

Expand All @@ -28,7 +28,7 @@ export class StandardRetryStrategy implements RetryStrategyV2 {

public async acquireInitialRetryToken(retryTokenScope: string): Promise<StandardRetryToken> {
return createDefaultRetryToken({
capacityBucket: this.capacityBucket,
availableCapacity: this.capacity,
retryDelay: DEFAULT_RETRY_DELAY_BASE,
retryCount: 0,
});
Expand Down Expand Up @@ -57,9 +57,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,
Expand All @@ -70,7 +70,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());
}

/**
* @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() {
Expand All @@ -84,9 +93,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)
);
}
Expand Down
73 changes: 15 additions & 58 deletions packages/util-retry/src/defaultRetryToken.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
NO_RETRY_INCREMENT,
RETRY_COST,
} from "./constants";
import { getDefaultRetryBackoffStrategy } from "./defaultRetryBackoffStrategy";
import { createDefaultRetryToken } from "./defaultRetryToken";

jest.mock("./defaultRetryBackoffStrategy");
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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,
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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,
});
Expand All @@ -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);
});
});
});

0 comments on commit 3b4ad9a

Please sign in to comment.