Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix(util-retry): correct attempts count on StandardRetryStrategy #4891

Merged
merged 1 commit into from
Jun 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/util-retry/src/ConfiguredRetryStrategy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ describe(ConfiguredRetryStrategy.name, () => {
const strategy = new ConfiguredRetryStrategy(5, (attempt) => attempt * 1000);

const token = await strategy.acquireInitialRetryToken("");
token.getRetryCount = () => 4;
token.getRetryCount = () => 3;

const retryToken = await strategy.refreshRetryTokenForRetry(token, {
errorType: "TRANSIENT",
});

expect(retryToken.getRetryCount()).toBe(5);
expect(retryToken.getRetryDelay()).toBe(5000);
expect(retryToken.getRetryCount()).toBe(4);
expect(retryToken.getRetryDelay()).toBe(4000);
});
});
25 changes: 14 additions & 11 deletions packages/util-retry/src/StandardRetryStrategy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe(StandardRetryStrategy.name, () => {
});

afterEach(() => {
jest.clearAllMocks;
jest.clearAllMocks();
});

it("sets maxAttemptsProvider as a class member variable", () => {
Expand Down Expand Up @@ -65,47 +65,49 @@ describe(StandardRetryStrategy.name, () => {
expect(getRetryCount).toHaveBeenCalledTimes(3);
});

it("throws when attempts exceeds maxAttempts", async () => {
it("disables any retries when maxAttempts is 1", async () => {
const mockRetryToken = {
getRetryCount: () => 2,
getRetryCount: () => 0,
getRetryTokenCount: (errorInfo: any) => 1,
};
(createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken);
const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(1));
const retryStrategy = new StandardRetryStrategy(1);
const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope);
try {
await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
fail(`expected ${noRetryTokenAvailableError}`);
} catch (error) {
expect(error).toStrictEqual(noRetryTokenAvailableError);
}
});

it("throws when attempts exceeds default max attempts (3)", async () => {
it("throws when attempts exceeds maxAttempts", async () => {
const mockRetryToken = {
getRetryCount: () => 5,
getRetryCount: () => 2,
getRetryTokenCount: (errorInfo: any) => 1,
};
(createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken);
const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(5));
const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(1));
const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope);
try {
await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
fail(`expected ${noRetryTokenAvailableError}`);
} catch (error) {
expect(error).toStrictEqual(noRetryTokenAvailableError);
}
});

it("throws when no tokens are available", async () => {
it("throws when attempts exceeds default max attempts (3)", async () => {
const mockRetryToken = {
getRetryCount: () => 0,
getRetryCount: () => 5,
getRetryTokenCount: (errorInfo: any) => 1,
hasRetryTokens: (errorType: RetryErrorType) => false,
};
(createDefaultRetryToken as jest.Mock).mockReturnValue(mockRetryToken);
const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts));
const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(5));
const token = await retryStrategy.acquireInitialRetryToken(retryTokenScope);
try {
await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
fail(`expected ${noRetryTokenAvailableError}`);
} catch (error) {
expect(error).toStrictEqual(noRetryTokenAvailableError);
}
Expand All @@ -125,6 +127,7 @@ describe(StandardRetryStrategy.name, () => {
} as RetryErrorInfo;
try {
await retryStrategy.refreshRetryTokenForRetry(token, errorInfo);
fail(`expected ${noRetryTokenAvailableError}`);
} catch (error) {
expect(error).toStrictEqual(noRetryTokenAvailableError);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/util-retry/src/StandardRetryStrategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export class StandardRetryStrategy implements RetryStrategyV2 {
}

private shouldRetry(tokenToRenew: StandardRetryToken, errorInfo: RetryErrorInfo, maxAttempts: number): boolean {
const attempts = tokenToRenew.getRetryCount();
const attempts = tokenToRenew.getRetryCount() + 1;

return (
attempts < maxAttempts &&
Expand Down
8 changes: 5 additions & 3 deletions private/aws-client-retry-test/src/ClientRetryTest.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,21 +89,23 @@ describe("util-retry integration tests", () => {
} catch (error) {
expect(error).toStrictEqual(expectedException);
expect(error.$metadata.httpStatusCode).toBe(429);
expect(error.$metadata.attempts).toBe(4);
expect(error.$metadata.attempts).toBe(3);
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 expectedRetryAttemptsPerRequest = 6;
// Set maxAttempts to one more than the number of retries (initial request + retries)
const maxAttempts = 7;
const delayPerRetry = 1;
const expectedRequests = 4;
const expectedRemainingCapacity =
expectedInitialCapacity - expectedDrainPerAttempt * expectedRetryAttemptsPerRequest * expectedRequests;

const retryStrategy = new ConfiguredRetryStrategy(expectedRetryAttemptsPerRequest, delayPerRetry);
const retryStrategy = new ConfiguredRetryStrategy(maxAttempts, delayPerRetry);
const s3 = new S3({
requestHandler: new MockRequestHandler(),
retryStrategy,
Expand Down