From e329689e2070d76244d01ce12541dd39e2455691 Mon Sep 17 00:00:00 2001 From: Allan Zheng Date: Mon, 3 May 2021 17:21:49 -0700 Subject: [PATCH 1/2] fix(middleware-retry): defaultStrategy handles any error The lower level stacks than throw anything to retry strategy in practice, so casting them to SdkError type is unsafe. With this change the retryStrategy will do best effort to convert the thrown any object to SdkError and then supply to retryDecider and so on. Another change is making SdkError type more general. It previously requires $fault, $metadata, which is obviously incorrect. Error thrown by SDK itself never contains these keys. So I mark them optional. Granted this is a breaking change to function interface like RetryDecider, but anyone depending it being required should already fail anyway. --- packages/middleware-retry/src/defaultStrategy.spec.ts | 10 ++++++++++ packages/middleware-retry/src/defaultStrategy.ts | 10 +++++++++- packages/smithy-client/src/sdk-error.ts | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/middleware-retry/src/defaultStrategy.spec.ts b/packages/middleware-retry/src/defaultStrategy.spec.ts index fb8f69e696aa7..e007a78f9bd2c 100644 --- a/packages/middleware-retry/src/defaultStrategy.spec.ts +++ b/packages/middleware-retry/src/defaultStrategy.spec.ts @@ -100,6 +100,16 @@ describe("defaultStrategy", () => { }); }); + it("handles non-standard errors", () => { + const nonStandardErrors = [undefined, "foo", { foo: "bar" }, 123, false, null]; + const maxAttempts = 1; + const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); + for (const error of nonStandardErrors) { + next = jest.fn().mockRejectedValue(error); + expect(retryStrategy.retry(next, { request: { headers: {} } } as any)).rejects.toBeInstanceOf(Error); + } + }); + describe("retryDecider init", () => { it("sets defaultRetryDecider if options is undefined", () => { const retryStrategy = new StandardRetryStrategy(() => Promise.resolve(maxAttempts)); diff --git a/packages/middleware-retry/src/defaultStrategy.ts b/packages/middleware-retry/src/defaultStrategy.ts index 5557d32538b37..4b07ae3eb6f5e 100644 --- a/packages/middleware-retry/src/defaultStrategy.ts +++ b/packages/middleware-retry/src/defaultStrategy.ts @@ -129,7 +129,8 @@ export class StandardRetryStrategy implements RetryStrategy { output.$metadata.totalRetryDelay = totalDelay; return { response, output }; - } catch (err) { + } catch (e) { + const err = asSdkError(e); attempts++; if (this.shouldRetry(err as SdkError, attempts, maxAttempts)) { retryTokenAmount = this.retryQuota.retrieveRetryTokens(err); @@ -154,3 +155,10 @@ export class StandardRetryStrategy implements RetryStrategy { } } } + +const asSdkError = (error: unknown): SdkError => { + if (error instanceof Error) return error; + if (error instanceof Object) return Object.assign(new Error(), error); + if (typeof error === "string") return new Error(error); + return new Error(`AWS SDK error wrapper for ${error}`); +}; diff --git a/packages/smithy-client/src/sdk-error.ts b/packages/smithy-client/src/sdk-error.ts index 115925155a4b3..683d8d8a14756 100644 --- a/packages/smithy-client/src/sdk-error.ts +++ b/packages/smithy-client/src/sdk-error.ts @@ -2,4 +2,4 @@ import { MetadataBearer } from "@aws-sdk/types"; import { SmithyException } from "./exception"; -export type SdkError = Error & SmithyException & MetadataBearer; +export type SdkError = Error & Partial & Partial; From a1650a7cd36cf3f263c7756127833e42b6247dd4 Mon Sep 17 00:00:00 2001 From: Allan Zheng Date: Tue, 4 May 2021 09:52:16 -0700 Subject: [PATCH 2/2] fix(protocol-test): fake handler should throw error --- protocol_tests/aws-ec2/tests/functional/ec2query.spec.ts | 6 ++++-- protocol_tests/aws-json/tests/functional/awsjson1_1.spec.ts | 6 ++++-- protocol_tests/aws-query/tests/functional/awsquery.spec.ts | 6 ++++-- .../aws-restjson/tests/functional/restjson1.spec.ts | 6 ++++-- protocol_tests/aws-restxml/tests/functional/restxml.spec.ts | 6 ++++-- 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/protocol_tests/aws-ec2/tests/functional/ec2query.spec.ts b/protocol_tests/aws-ec2/tests/functional/ec2query.spec.ts index 73c4026ead93a..ecc2a7140f7e3 100644 --- a/protocol_tests/aws-ec2/tests/functional/ec2query.spec.ts +++ b/protocol_tests/aws-ec2/tests/functional/ec2query.spec.ts @@ -27,8 +27,10 @@ import { Readable } from "stream"; /** * Throws an expected exception that contains the serialized request. */ -class EXPECTED_REQUEST_SERIALIZATION_ERROR { - constructor(readonly request: HttpRequest) {} +class EXPECTED_REQUEST_SERIALIZATION_ERROR extends Error { + constructor(readonly request: HttpRequest) { + super(); + } } /** diff --git a/protocol_tests/aws-json/tests/functional/awsjson1_1.spec.ts b/protocol_tests/aws-json/tests/functional/awsjson1_1.spec.ts index cb3e8fdf50837..eca8059168a83 100644 --- a/protocol_tests/aws-json/tests/functional/awsjson1_1.spec.ts +++ b/protocol_tests/aws-json/tests/functional/awsjson1_1.spec.ts @@ -18,8 +18,10 @@ import { Readable } from "stream"; /** * Throws an expected exception that contains the serialized request. */ -class EXPECTED_REQUEST_SERIALIZATION_ERROR { - constructor(readonly request: HttpRequest) {} +class EXPECTED_REQUEST_SERIALIZATION_ERROR extends Error { + constructor(readonly request: HttpRequest) { + super(); + } } /** diff --git a/protocol_tests/aws-query/tests/functional/awsquery.spec.ts b/protocol_tests/aws-query/tests/functional/awsquery.spec.ts index fcd982413c7ce..facdf7b980c2b 100644 --- a/protocol_tests/aws-query/tests/functional/awsquery.spec.ts +++ b/protocol_tests/aws-query/tests/functional/awsquery.spec.ts @@ -35,8 +35,10 @@ import { Readable } from "stream"; /** * Throws an expected exception that contains the serialized request. */ -class EXPECTED_REQUEST_SERIALIZATION_ERROR { - constructor(readonly request: HttpRequest) {} +class EXPECTED_REQUEST_SERIALIZATION_ERROR extends Error { + constructor(readonly request: HttpRequest) { + super(); + } } /** diff --git a/protocol_tests/aws-restjson/tests/functional/restjson1.spec.ts b/protocol_tests/aws-restjson/tests/functional/restjson1.spec.ts index 7267b9fa0280a..52c2be52bf30f 100644 --- a/protocol_tests/aws-restjson/tests/functional/restjson1.spec.ts +++ b/protocol_tests/aws-restjson/tests/functional/restjson1.spec.ts @@ -51,8 +51,10 @@ import { Readable } from "stream"; /** * Throws an expected exception that contains the serialized request. */ -class EXPECTED_REQUEST_SERIALIZATION_ERROR { - constructor(readonly request: HttpRequest) {} +class EXPECTED_REQUEST_SERIALIZATION_ERROR extends Error { + constructor(readonly request: HttpRequest) { + super(); + } } /** diff --git a/protocol_tests/aws-restxml/tests/functional/restxml.spec.ts b/protocol_tests/aws-restxml/tests/functional/restxml.spec.ts index 0f31c9f4e78a4..3f5567c4bd723 100644 --- a/protocol_tests/aws-restxml/tests/functional/restxml.spec.ts +++ b/protocol_tests/aws-restxml/tests/functional/restxml.spec.ts @@ -60,8 +60,10 @@ import { Readable } from "stream"; /** * Throws an expected exception that contains the serialized request. */ -class EXPECTED_REQUEST_SERIALIZATION_ERROR { - constructor(readonly request: HttpRequest) {} +class EXPECTED_REQUEST_SERIALIZATION_ERROR extends Error { + constructor(readonly request: HttpRequest) { + super(); + } } /**