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): use different retry token instances per request #4755

Merged
merged 7 commits into from
May 25, 2023

Conversation

kuhe
Copy link
Contributor

@kuhe kuhe commented May 24, 2023

Issue

#4754

Description

Use different instances of the retry token when new requests are made from the same client. They will however share the token availability if coming from the same client and RetryStrategy instance.

Currently, because the retry token object and its closure persist between calls of the same client using the same StandardRetryStrategy instance, the retry count stays at its max value.

Testing

import { S3 } from "@aws-sdk/client-s3";
import { HttpResponse } from "@aws-sdk/protocol-http";
import { ConfiguredRetryStrategy } from "@aws-sdk/util-retry";

class MockRequestHandler {
  // Returns 429 error to cause a retry in the client.
  // No network request is made.
  async handle() {
    console.log("    Request (mock) responding with 429 throttling error.");
    return {
      response: new HttpResponse({
        statusCode: 429,
        body: Buffer.from(""),
      }),
    };
  }
}

let n = 0;
console.log("Time:", n++);
const i = setInterval(() => {
  console.log("Time:", n++); // print a time marker to the console to show time between retries.
}, 1000);

// initialize client with customized retry strategy and mock request handler.
const s3 = new S3({
  retryStrategy: new ConfiguredRetryStrategy(3, (_: number) => _ * 1000),
  requestHandler: new MockRequestHandler(),
});

const go = () =>
  s3
    .listBuckets({})
    .then((r) => console.log(r.$metadata))
    .catch((r) => console.error(r.$metadata))
    .then(async () => {
      console.log("Retries exhausted.");
    });

Promise.all([go(), go(), go()]).then(() => clearInterval(i));


// with the bug, multiple requests from the same instance will exhaust each others' retry counts

@kuhe kuhe requested review from a team as code owners May 24, 2023 20:01
@kuhe kuhe force-pushed the fix/retry branch 2 times, most recently from d3cb733 to 2269a41 Compare May 24, 2023 20:28
@kuhe kuhe changed the title fix(util-retry): reset retry count when acquiring initial token fix(util-retry): use different retry token instances per request May 24, 2023
@trivikr trivikr requested a review from siddsriv May 24, 2023 20:51
@trivikr
Copy link
Member

trivikr commented May 25, 2023

In the offline discussion, we decided to deprecate the following functions from StandardRetryToken

  • hasRetryTokens
  • getRetryTokenCount
  • releaseRetryTokens

Although getLastRetryCost should be renamed to getRetryCost as RetryToken is immutable, we decided to keep it.

They have access to availableCapacity, and can modify it.
Anything which modifies availableCapacity should be in retryStrategy.

@kuhe kuhe force-pushed the fix/retry branch 3 times, most recently from 2bd3423 to cdaf93d Compare May 25, 2023 19:04
packages/util-retry/src/defaultRetryToken.ts Outdated Show resolved Hide resolved
packages/util-retry/src/StandardRetryStrategy.ts Outdated Show resolved Hide resolved
packages/util-retry/src/StandardRetryStrategy.ts Outdated Show resolved Hide resolved
@trivikr trivikr marked this pull request as ready for review May 25, 2023 19:30
@trivikr
Copy link
Member

trivikr commented May 25, 2023

Verified the fix by printing capacityAmount with the following code which calls two operations for two clients.

import {
  S3Client,
  ListBucketsCommand,
  ListObjectsCommand,
} from "../aws-sdk-js-v3/clients/client-s3/dist-cjs/index.js";
import {
  DynamoDBClient,
  ListTablesCommand,
  ListBackupsCommand,
} from "../aws-sdk-js-v3/clients/client-dynamodb/dist-cjs/index.js";
import { NodeHttpHandler } from "../aws-sdk-js-v3/packages/node-http-handler/dist-cjs/index.js";
import { ConfiguredRetryStrategy } from "../aws-sdk-js-v3/packages/util-retry/dist-cjs/index.js";

// Simulate a timeout error.
class NodeHttpHandlerReturnsTimeout extends NodeHttpHandler {
  async handle(request, options) {
    const timeoutError = new Error("Request timed out");
    timeoutError.name = "TimeoutError";
    throw timeoutError;
  }
}

const s3Client = new S3Client({
  region: "us-west-2",
  requestHandler: new NodeHttpHandlerReturnsTimeout(),
  retryStrategy: new ConfiguredRetryStrategy(2, () => 100),
});

const dynamodbClient = new DynamoDBClient({
  region: "us-west-2",
  requestHandler: new NodeHttpHandlerReturnsTimeout(),
  retryStrategy: new ConfiguredRetryStrategy(3, () => 50),
});

const calls = {};

const callRequest = async (client, command, operation) => {
  if (!calls[operation]) {
    calls[operation] = 0;
  }

  while (true) {
    try {
      await client.send(command);
    } catch (error) {
      const metadata = error.$metadata;
      calls[operation] = calls[operation] + 1;
      console.log(
        `[${operation}]: Call #${calls[operation]}, Attempts: ${metadata.attempts}, Retry delay: ${metadata.totalRetryDelay}ms`
      );

      if (calls[operation] === 3) {
        break;
      }
    }
  }
};

await Promise.all([
  callRequest(s3Client, new ListBucketsCommand({}), "s3:listBuckets"),
  callRequest(
    s3Client,
    new ListObjectsCommand({ Bucket: "foo" }),
    "s3:listObjects"
  ),
  callRequest(dynamodbClient, new ListTablesCommand({}), "ddb:listTables"),
  callRequest(dynamodbClient, new ListBackupsCommand({}), "ddb:listBackups"),
]);

@kuhe kuhe merged commit d666720 into aws:main May 25, 2023
@kuhe kuhe deleted the fix/retry branch May 25, 2023 21:43
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants