aws / aws-sdk-js-v3

Modularized AWS SDK for JavaScript.
Apache License 2.0
3.08k stars 575 forks source link

When SDK introduces client side rate limiting in default RetryStrategy, the error is swallowed #4753

Open trivikr opened 1 year ago

trivikr commented 1 year ago

Checkboxes for prior research

Describe the bug

When SDK introduces client side rate limiting in Retry Strategy, the error is swallowed

SDK version number

All, used v3.335.0 in testing

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

All, used v16.20.0 in testing

Reproduction Steps

Here is a repro which adds custom Handler which returns a Timeout for every call, and a retryStrategy with just 100ms delay between retries.

import { Kinesis } from "@aws-sdk/client-kinesis"; // v3.335.0
import { NodeHttp2Handler } from "@aws-sdk/node-http-handler";
import { ConfiguredRetryStrategy } from "@aws-sdk/util-retry";

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

const client = new Kinesis({
  region: "us-west-2",
  requestHandler: new NodeHttp2HandlerReturnsTimeout(),
  retryStrategy: new ConfiguredRetryStrategy(
    4, // max attempts.
    () => 100 // delay only for 100ms between retries to speed up client side rate limiting.
  ),
});

while (true) {
  try {
    await client.listStreams({});
  } catch (error) {
    if (error.$metadata.attempts === 1) {
      // SDK introduced client side rate limiting.
      throw error;
    }
  }
}

Observed Behavior

Error is thrown by the SDK with no info on why the error was thrown

file:///Users/trivikr/workspace/test/test.mjs:8
    const timeoutError = new Error("Request timed out");
                         ^

Error [TimeoutError]: Request timed out
    at NodeHttp2HandlerReturnsTimeout.handle (file:///Users/trivikr/workspace/test/test.mjs:8:26)
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/client-kinesis/dist-cjs/commands/ListStreamsCommand.js:36:58
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-serde/dist-cjs/deserializerMiddleware.js:5:32
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-signing/dist-cjs/middleware.js:14:26
    at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:27:46
    at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
    at async file:///Users/trivikr/workspace/test/test.mjs:27:5 {
  '$metadata': { attempts: 1, totalRetryDelay: 0 }
}

Expected Behavior

Error thrown by the SDK with info on why the error was thrown

/Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:40
                    const errorToThrow = new Error(refreshError.message, { cause: lastError });
                                         ^

Error: No retry token available
    at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:40:42
    at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
    at async file:///Users/trivikr/workspace/test/test.mjs:27:5 {
  '$metadata': { attempts: 1, totalRetryDelay: 0 },
  [cause]: Error [TimeoutError]: Request timed out
      at NodeHttp2HandlerReturnsTimeout.handle (file:///Users/trivikr/workspace/test/test.mjs:8:26)
      at /Users/trivikr/workspace/test/node_modules/@aws-sdk/client-kinesis/dist-cjs/commands/ListStreamsCommand.js:36:58
      at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-serde/dist-cjs/deserializerMiddleware.js:5:32
      at /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-signing/dist-cjs/middleware.js:14:26
      at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:27:46
      at async /Users/trivikr/workspace/test/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:7:26
      at async file:///Users/trivikr/workspace/test/test.mjs:27:5
}

This error was thrown by updating the catch clause in node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js as follows:

                catch (refreshError) {
                    const errorToThrow = new Error(refreshError.message, { cause: lastError });
                    errorToThrow.$metadata = {
                        attempts: attempts + 1,
                        totalRetryDelay,
                    };
                    throw errorToThrow;
                }

Possible Solution

Update the catch clause in retryMiddleware to not swallow the error thrown by refreshRetryTokenForRetry

Additional Information/Context

github-actions[bot] commented 4 months ago

Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue.