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(core): retry after clock skew correction #1170

Merged
merged 7 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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
7 changes: 7 additions & 0 deletions .changeset/giant-pianos-lie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@smithy/service-error-classification": patch
"@smithy/middleware-retry": patch
"@smithy/types": patch
---

make clock skew correcting errors transient
10 changes: 9 additions & 1 deletion packages/middleware-retry/src/configurations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,22 @@ export interface RetryInputConfig {
retryStrategy?: RetryStrategy | RetryStrategyV2;
}

interface PreviouslyResolved {
/**
* @internal
*/
export interface PreviouslyResolved {
/**
* Specifies provider for retry algorithm to use.
* @internal
*/
retryMode: string | Provider<string>;

systemClockOffset?: number;
}

/**
* @internal
*/
export interface RetryResolvedConfig {
/**
* Resolved value for input config {@link RetryInputConfig.maxAttempts}
Expand Down
30 changes: 23 additions & 7 deletions packages/middleware-retry/src/retryMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@ import {
RetryStrategyV2,
RetryToken,
SdkError,
SkdErrorWithClockSkewMetadata,
} from "@smithy/types";
import { INVOCATION_ID_HEADER, REQUEST_HEADER } from "@smithy/util-retry";
import { v4 } from "uuid";

import { RetryResolvedConfig } from "./configurations";
import { PreviouslyResolved, RetryResolvedConfig } from "./configurations";
import { isStreamingPayload } from "./isStreamingPayload/isStreamingPayload";
import { asSdkError } from "./util";

export const retryMiddleware = (options: RetryResolvedConfig) => <Output extends MetadataBearer = MetadataBearer>(
export const retryMiddleware = (options: RetryResolvedConfig & Partial<PreviouslyResolved>) => <
Output extends MetadataBearer = MetadataBearer
>(
next: FinalizeHandler<any, Output>,
context: HandlerExecutionContext
): FinalizeHandler<any, Output> => async (
Expand All @@ -45,19 +48,31 @@ export const retryMiddleware = (options: RetryResolvedConfig) => <Output extends
if (isRequest) {
request.headers[INVOCATION_ID_HEADER] = v4();
}

syall marked this conversation as resolved.
Show resolved Hide resolved
let initialSystemClockOffset = 0;

while (true) {
try {
if (isRequest) {
request.headers[REQUEST_HEADER] = `attempt=${attempts + 1}; max=${maxAttempts}`;
}
initialSystemClockOffset = options.systemClockOffset ?? 0 | 0;
const { response, output } = await next(args);
retryStrategy.recordSuccess(retryToken);
output.$metadata.attempts = attempts + 1;
output.$metadata.totalRetryDelay = totalRetryDelay;
return { response, output };
} catch (e) {
const retryErrorInfo = getRetryErrorInfo(e);
lastError = asSdkError(e);
} catch (e: unknown) {
const latestSystemClockOffset = options.systemClockOffset ?? 0 | 0;
const clockSkewCorrected = initialSystemClockOffset !== latestSystemClockOffset;

const sdkError = e as SkdErrorWithClockSkewMetadata;
if (clockSkewCorrected && sdkError.$metadata && typeof sdkError.$metadata === "object") {
sdkError.$metadata.clockSkewCorrected = true;
}

const retryErrorInfo = getRetryErrorInfo(sdkError);
lastError = asSdkError(sdkError);

if (isRequest && isStreamingPayload(request)) {
(context.logger instanceof NoOpLogger ? console : context.logger)?.warn(
Expand Down Expand Up @@ -95,8 +110,9 @@ const isRetryStrategyV2 = (retryStrategy: RetryStrategy | RetryStrategyV2) =>
typeof (retryStrategy as RetryStrategyV2).refreshRetryTokenForRetry !== "undefined" &&
typeof (retryStrategy as RetryStrategyV2).recordSuccess !== "undefined";

const getRetryErrorInfo = (error: SdkError): RetryErrorInfo => {
const getRetryErrorInfo = (error: SkdErrorWithClockSkewMetadata): RetryErrorInfo => {
const errorInfo: RetryErrorInfo = {
error,
errorType: getRetryErrorType(error),
};
const retryAfterHint = getRetryAfterHint(error.$response);
Expand All @@ -106,7 +122,7 @@ const getRetryErrorInfo = (error: SdkError): RetryErrorInfo => {
return errorInfo;
};

const getRetryErrorType = (error: SdkError): RetryErrorType => {
const getRetryErrorType = (error: SkdErrorWithClockSkewMetadata): RetryErrorType => {
if (isThrottlingError(error)) return "THROTTLING";
if (isTransientError(error)) return "TRANSIENT";
if (isServerError(error)) return "SERVER_ERROR";
Expand Down
7 changes: 4 additions & 3 deletions packages/service-error-classification/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { SdkError } from "@smithy/types";
import { SdkError, SkdErrorWithClockSkewMetadata } from "@smithy/types";

import {
CLOCK_SKEW_ERROR_CODES,
Expand All @@ -23,12 +23,13 @@ export const isThrottlingError = (error: SdkError) =>
* cause where the NodeHttpHandler does not decorate the Error with
* the name "TimeoutError" to be checked by the TRANSIENT_ERROR_CODES condition.
*/
export const isTransientError = (error: SdkError) =>
export const isTransientError = (error: SdkError | SkdErrorWithClockSkewMetadata) =>
(error as SkdErrorWithClockSkewMetadata).$metadata?.clockSkewCorrected ||
TRANSIENT_ERROR_CODES.includes(error.name) ||
NODEJS_TIMEOUT_ERROR_CODES.includes((error as { code?: string })?.code || "") ||
TRANSIENT_ERROR_STATUS_CODES.includes(error.$metadata?.httpStatusCode || 0);

export const isServerError = (error: SdkError) => {
export const isServerError = (error: SdkError | SkdErrorWithClockSkewMetadata) => {
if (error.$metadata?.httpStatusCode !== undefined) {
const statusCode = error.$metadata.httpStatusCode;
if (500 <= statusCode && statusCode <= 599 && !isTransientError(error)) {
Expand Down
9 changes: 8 additions & 1 deletion packages/types/src/retry.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { SdkError } from "./shapes";

/**
* @public
*/
Expand Down Expand Up @@ -33,14 +35,19 @@ export type RetryErrorType =
* @public
*/
export interface RetryErrorInfo {
/**
* The error thrown during the initial request, if available.
*/
error?: SdkError;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users have requested that the thrown exception is available in the retry strategy implementation. Otherwise, they cannot even check (for example) the http status code of an error when deciding whether to retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, they are forced to write a verbose retry wrapper instead of using a RetryStrategy.


errorType: RetryErrorType;

/**
* Protocol hint. This could come from Http's 'retry-after' header or
* something from MQTT or any other protocol that has the ability to convey
* retry info from a peer.
*
* @returns the Date after which a retry should be attempted.
* The Date after which a retry should be attempted.
*/
retryAfterHint?: Date;
}
Expand Down
11 changes: 11 additions & 0 deletions packages/types/src/shapes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,14 @@ export interface SmithyException {
* a base ServiceException prefixed with the service name.
*/
export type SdkError = Error & Partial<SmithyException> & Partial<MetadataBearer>;

/**
* @internal
*
* @deprecated for same reason as SdkError. Use public client modeled exceptions in application code.
*/
export type SkdErrorWithClockSkewMetadata = SdkError & {
$metadata: SdkError["$metadata"] & {
clockSkewCorrected?: boolean;
};
};
Loading