From a6e366d5268d4cd2ff3445133462dba81296d3ff Mon Sep 17 00:00:00 2001 From: Minh-Anh Phan <111523473+minhanh-phan@users.noreply.github.com> Date: Thu, 8 Sep 2022 17:08:36 -0700 Subject: [PATCH] [core-util] Update "delay" method in core-util with abort options (#23019) ### Packages impacted by this PR core-util ### Issues associated with this PR #15930 ### Describe the problem that is addressed by this PR - Update "delay" in core-util to support abort message - Unduplicate "delay" function in core-http and core-amqp ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? - Choose the implementation with "options" parameters to keep the flexibility in case we want to add extra functionality in the future ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ #15832 ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [x ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --- sdk/core/core-amqp/src/retry.ts | 12 ++-- sdk/core/core-amqp/src/util/utils.ts | 47 +++----------- sdk/core/core-http/review/core-http.api.md | 7 +-- sdk/core/core-http/src/coreHttp.ts | 2 +- .../bearerTokenAuthenticationPolicy.ts | 2 +- .../src/policies/exponentialRetryPolicy.ts | 2 +- .../src/policies/rpRegistrationPolicy.ts | 2 +- .../src/policies/systemErrorRetryPolicy.ts | 2 +- .../src/policies/throttlingRetryPolicy.ts | 4 +- sdk/core/core-http/src/util/delay.ts | 63 ------------------- sdk/core/core-util/CHANGELOG.md | 2 + sdk/core/core-util/package.json | 1 + sdk/core/core-util/review/core-util.api.md | 10 ++- sdk/core/core-util/src/delay.ts | 58 ++++++++++++++++- sdk/core/core-util/src/index.ts | 2 +- sdk/core/core-util/test/public/delay.spec.ts | 19 ++++++ .../src/receivers/receiverCommon.ts | 12 ++-- 17 files changed, 117 insertions(+), 130 deletions(-) delete mode 100644 sdk/core/core-http/src/util/delay.ts diff --git a/sdk/core/core-amqp/src/retry.ts b/sdk/core/core-amqp/src/retry.ts index 91f897806ba9..c4af872afe48 100644 --- a/sdk/core/core-amqp/src/retry.ts +++ b/sdk/core/core-amqp/src/retry.ts @@ -6,7 +6,7 @@ import { MessagingError, translate } from "./errors"; import { AbortSignalLike } from "@azure/abort-controller"; import { Constants } from "./util/constants"; import { checkNetworkConnection } from "./util/checkNetworkConnection"; -import { delay } from "./util/utils"; +import { delay } from "@azure/core-util"; import { logger } from "./log"; /** @@ -263,11 +263,11 @@ export async function retry(config: RetryConfig): Promise { targetDelayInMs, updatedConfig.operationType ); - await delay( - targetDelayInMs, - updatedConfig.abortSignal, - `The retry operation has been cancelled by the user.` - ); + await delay(targetDelayInMs, { + abortSignal: updatedConfig.abortSignal, + abortErrorMsg: `The retry operation has been cancelled by the user.`, + }); + continue; } else { break; diff --git a/sdk/core/core-amqp/src/util/utils.ts b/sdk/core/core-amqp/src/util/utils.ts index a8724a3bc964..b3415e3837d8 100644 --- a/sdk/core/core-amqp/src/util/utils.ts +++ b/sdk/core/core-amqp/src/util/utils.ts @@ -1,11 +1,10 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. -import { AbortError, AbortSignalLike } from "@azure/abort-controller"; import { CancellableAsyncLock, CancellableAsyncLockImpl } from "./lock"; -import { StandardAbortMessage } from "./constants"; +import { AbortSignalLike } from "@azure/abort-controller"; import { WebSocketImpl } from "rhea-promise"; -import { isDefined } from "@azure/core-util"; +import { delay as wrapperDelay } from "@azure/core-util"; /** * @internal @@ -176,47 +175,19 @@ export class Timeout { * @param value - The value to be resolved with after a timeout of t milliseconds. * @returns - Resolved promise */ -export function delay( +export async function delay( delayInMs: number, abortSignal?: AbortSignalLike, abortErrorMsg?: string, value?: T ): Promise { - return new Promise((resolve, reject) => { - let timer: ReturnType | undefined = undefined; - let onAborted: (() => void) | undefined = undefined; - - const rejectOnAbort = (): void => { - return reject(new AbortError(abortErrorMsg ? abortErrorMsg : StandardAbortMessage)); - }; - - const removeListeners = (): void => { - if (abortSignal && onAborted) { - abortSignal.removeEventListener("abort", onAborted); - } - }; - - onAborted = (): void => { - if (isDefined(timer)) { - clearTimeout(timer); - } - removeListeners(); - return rejectOnAbort(); - }; - - if (abortSignal && abortSignal.aborted) { - return rejectOnAbort(); - } - - timer = setTimeout(() => { - removeListeners(); - resolve(value); - }, delayInMs); - - if (abortSignal) { - abortSignal.addEventListener("abort", onAborted); - } + await wrapperDelay(delayInMs, { + abortSignal: abortSignal, + abortErrorMsg: abortErrorMsg, }); + if (value !== undefined) { + return value; + } } /** diff --git a/sdk/core/core-http/review/core-http.api.md b/sdk/core/core-http/review/core-http.api.md index 838198395f80..7cd9044beb2a 100644 --- a/sdk/core/core-http/review/core-http.api.md +++ b/sdk/core/core-http/review/core-http.api.md @@ -10,6 +10,7 @@ import { AbortSignalLike } from '@azure/abort-controller'; import { AccessToken } from '@azure/core-auth'; import { Context } from '@azure/core-tracing'; import { Debugger } from '@azure/logger'; +import { delay } from '@azure/core-util'; import { GetTokenOptions } from '@azure/core-auth'; import { isTokenCredential } from '@azure/core-auth'; import { OperationTracingOptions } from '@azure/core-tracing'; @@ -181,11 +182,7 @@ export class DefaultHttpClient implements HttpClient { sendRequest(httpRequest: WebResourceLike): Promise; } -// @public -export function delay(delayInMs: number, value?: T, options?: { - abortSignal?: AbortSignalLike; - abortErrorMsg?: string; -}): Promise; +export { delay } // @public export interface DeserializationContentTypes { diff --git a/sdk/core/core-http/src/coreHttp.ts b/sdk/core/core-http/src/coreHttp.ts index 2fef3890e962..ebecf6ceb778 100644 --- a/sdk/core/core-http/src/coreHttp.ts +++ b/sdk/core/core-http/src/coreHttp.ts @@ -113,7 +113,7 @@ export { } from "./util/utils"; export { URLBuilder, URLQuery } from "./url"; export { AbortSignalLike } from "@azure/abort-controller"; -export { delay } from "./util/delay"; +export { delay } from "@azure/core-util"; // legacy exports. Use core-tracing instead (and remove on next major version update of core-http). export { createSpanFunction, SpanConfig } from "./createSpanLegacy"; diff --git a/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts b/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts index 21fab7010868..7ff800d5c974 100644 --- a/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts +++ b/sdk/core/core-http/src/policies/bearerTokenAuthenticationPolicy.ts @@ -11,7 +11,7 @@ import { import { Constants } from "../util/constants"; import { HttpOperationResponse } from "../httpOperationResponse"; import { WebResourceLike } from "../webResource"; -import { delay } from "../util/delay"; +import { delay } from "@azure/core-util"; // #region Access Token Cycler diff --git a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts index 41f1877926b0..5641b05ce575 100644 --- a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts @@ -21,7 +21,7 @@ import { Constants } from "../util/constants"; import { HttpOperationResponse } from "../httpOperationResponse"; import { RestError } from "../restError"; import { WebResourceLike } from "../webResource"; -import { delay } from "../util/delay"; +import { delay } from "@azure/core-util"; import { logger } from "../log"; /** diff --git a/sdk/core/core-http/src/policies/rpRegistrationPolicy.ts b/sdk/core/core-http/src/policies/rpRegistrationPolicy.ts index d27021e354f4..e43dbbe6063e 100644 --- a/sdk/core/core-http/src/policies/rpRegistrationPolicy.ts +++ b/sdk/core/core-http/src/policies/rpRegistrationPolicy.ts @@ -10,7 +10,7 @@ import { } from "./requestPolicy"; import { HttpOperationResponse } from "../httpOperationResponse"; import { WebResourceLike } from "../webResource"; -import { delay } from "../util/delay"; +import { delay } from "@azure/core-util"; export function rpRegistrationPolicy(retryTimeout = 30): RequestPolicyFactory { return { diff --git a/sdk/core/core-http/src/policies/systemErrorRetryPolicy.ts b/sdk/core/core-http/src/policies/systemErrorRetryPolicy.ts index cd2bedc0bf13..0ab93cfc581f 100644 --- a/sdk/core/core-http/src/policies/systemErrorRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/systemErrorRetryPolicy.ts @@ -20,7 +20,7 @@ import { } from "../util/exponentialBackoffStrategy"; import { HttpOperationResponse } from "../httpOperationResponse"; import { WebResourceLike } from "../webResource"; -import { delay } from "../util/delay"; +import { delay } from "@azure/core-util"; /** * A policy that retries when there's a system error, identified by the codes "ETIMEDOUT", "ESOCKETTIMEDOUT", "ECONNREFUSED", "ECONNRESET" or "ENOENT". diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index 8b620b046ffb..5e8a4c841491 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -12,7 +12,7 @@ import { Constants } from "../util/constants"; import { DEFAULT_CLIENT_MAX_RETRY_COUNT } from "../util/throttlingRetryStrategy"; import { HttpOperationResponse } from "../httpOperationResponse"; import { WebResourceLike } from "../webResource"; -import { delay } from "../util/delay"; +import { delay } from "@azure/core-util"; type ResponseHandler = ( httpRequest: WebResourceLike, @@ -88,7 +88,7 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { if (delayInMs) { this.numberOfRetries += 1; - await delay(delayInMs, undefined, { + await delay(delayInMs, { abortSignal: httpRequest.abortSignal, abortErrorMsg: StandardAbortMessage, }); diff --git a/sdk/core/core-http/src/util/delay.ts b/sdk/core/core-http/src/util/delay.ts deleted file mode 100644 index f1330061adc2..000000000000 --- a/sdk/core/core-http/src/util/delay.ts +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -import { AbortError, AbortSignalLike } from "@azure/abort-controller"; -import { isDefined } from "@azure/core-util"; - -const StandardAbortMessage = "The operation was aborted."; - -/** - * A wrapper for setTimeout that resolves a promise after delayInMs milliseconds. - * @param delayInMs - The number of milliseconds to be delayed. - * @param value - The value to be resolved with after a timeout of t milliseconds. - * @param options - The options for delay - currently abort options - * @param abortSignal - The abortSignal associated with containing operation. - * @param abortErrorMsg - The abort error message associated with containing operation. - * @returns - Resolved promise - */ -export function delay( - delayInMs: number, - value?: T, - options?: { - abortSignal?: AbortSignalLike; - abortErrorMsg?: string; - } -): Promise { - return new Promise((resolve, reject) => { - let timer: ReturnType | undefined = undefined; - let onAborted: (() => void) | undefined = undefined; - - const rejectOnAbort = (): void => { - return reject( - new AbortError(options?.abortErrorMsg ? options?.abortErrorMsg : StandardAbortMessage) - ); - }; - - const removeListeners = (): void => { - if (options?.abortSignal && onAborted) { - options.abortSignal.removeEventListener("abort", onAborted); - } - }; - - onAborted = (): void => { - if (isDefined(timer)) { - clearTimeout(timer); - } - removeListeners(); - return rejectOnAbort(); - }; - - if (options?.abortSignal && options.abortSignal.aborted) { - return rejectOnAbort(); - } - - timer = setTimeout(() => { - removeListeners(); - resolve(value); - }, delayInMs); - - if (options?.abortSignal) { - options.abortSignal.addEventListener("abort", onAborted); - } - }); -} diff --git a/sdk/core/core-util/CHANGELOG.md b/sdk/core/core-util/CHANGELOG.md index fed85929c247..5655e81554c0 100644 --- a/sdk/core/core-util/CHANGELOG.md +++ b/sdk/core/core-util/CHANGELOG.md @@ -4,6 +4,8 @@ ### Features Added +- Update `delay` method to handle `abortSignal` functionality + ### Breaking Changes ### Bugs Fixed diff --git a/sdk/core/core-util/package.json b/sdk/core/core-util/package.json index d5a7037c2abc..ad031773738d 100644 --- a/sdk/core/core-util/package.json +++ b/sdk/core/core-util/package.json @@ -69,6 +69,7 @@ "sideEffects": false, "prettier": "@azure/eslint-plugin-azure-sdk/prettier.json", "dependencies": { + "@azure/abort-controller": "^1.0.0", "tslib": "^2.2.0" }, "devDependencies": { diff --git a/sdk/core/core-util/review/core-util.api.md b/sdk/core/core-util/review/core-util.api.md index 943276bbd752..e8dd571f4281 100644 --- a/sdk/core/core-util/review/core-util.api.md +++ b/sdk/core/core-util/review/core-util.api.md @@ -4,6 +4,8 @@ ```ts +import { AbortSignalLike } from '@azure/abort-controller'; + // @public export function computeSha256Hash(content: string, encoding: "base64" | "hex"): Promise; @@ -11,7 +13,13 @@ export function computeSha256Hash(content: string, encoding: "base64" | "hex"): export function computeSha256Hmac(key: string, stringToSign: string, encoding: "base64" | "hex"): Promise; // @public -export function delay(timeInMs: number): Promise; +export function delay(timeInMs: number, options?: DelayOptions): Promise; + +// @public +export interface DelayOptions { + abortErrorMsg?: string; + abortSignal?: AbortSignalLike; +} // @public export function getErrorMessage(e: unknown): string; diff --git a/sdk/core/core-util/src/delay.ts b/sdk/core/core-util/src/delay.ts index ac739ba69f80..07f9b42a7534 100644 --- a/sdk/core/core-util/src/delay.ts +++ b/sdk/core/core-util/src/delay.ts @@ -1,11 +1,65 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +import { AbortError, AbortSignalLike } from "@azure/abort-controller"; +import { isDefined } from "./typeGuards"; + +const StandardAbortMessage = "The operation was aborted."; + +/** + * Options for support abort functionality for the delay method + */ +export interface DelayOptions { + /** + * The abortSignal associated with containing operation. + */ + abortSignal?: AbortSignalLike; + /** + * The abort error message associated with containing operation. + */ + abortErrorMsg?: string; +} + /** * A wrapper for setTimeout that resolves a promise after timeInMs milliseconds. * @param timeInMs - The number of milliseconds to be delayed. + * @param options - The options for delay - currently abort options * @returns Promise that is resolved after timeInMs */ -export function delay(timeInMs: number): Promise { - return new Promise((resolve) => setTimeout(() => resolve(), timeInMs)); +export function delay(timeInMs: number, options?: DelayOptions): Promise { + return new Promise((resolve, reject) => { + let timer: ReturnType | undefined = undefined; + let onAborted: (() => void) | undefined = undefined; + + const rejectOnAbort = (): void => { + return reject(new AbortError(options?.abortErrorMsg ?? StandardAbortMessage)); + }; + + const removeListeners = (): void => { + if (options?.abortSignal && onAborted) { + options.abortSignal.removeEventListener("abort", onAborted); + } + }; + + onAborted = (): void => { + if (isDefined(timer)) { + clearTimeout(timer); + } + removeListeners(); + return rejectOnAbort(); + }; + + if (options?.abortSignal && options.abortSignal.aborted) { + return rejectOnAbort(); + } + + timer = setTimeout(() => { + removeListeners(); + resolve(); + }, timeInMs); + + if (options?.abortSignal) { + options.abortSignal.addEventListener("abort", onAborted); + } + }); } diff --git a/sdk/core/core-util/src/index.ts b/sdk/core/core-util/src/index.ts index f91afcf8af6f..7a4b1e9d191e 100644 --- a/sdk/core/core-util/src/index.ts +++ b/sdk/core/core-util/src/index.ts @@ -2,7 +2,7 @@ // Licensed under the MIT license. export { isNode } from "./isNode"; -export { delay } from "./delay"; +export { delay, DelayOptions } from "./delay"; export { getRandomIntegerInclusive } from "./random"; export { isObject, UnknownObject } from "./object"; export { isError, getErrorMessage } from "./error"; diff --git a/sdk/core/core-util/test/public/delay.spec.ts b/sdk/core/core-util/test/public/delay.spec.ts index fab6fc29a243..f1003d97b513 100644 --- a/sdk/core/core-util/test/public/delay.spec.ts +++ b/sdk/core/core-util/test/public/delay.spec.ts @@ -2,6 +2,7 @@ // Licensed under the MIT license. import * as sinon from "sinon"; +import { AbortController } from "@azure/abort-controller"; import { assert } from "chai"; import { delay } from "../../src"; @@ -20,4 +21,22 @@ describe("delay", function () { // should be resolved, so we can await it and it will resolve next tick await delayPromise; }); + + it("should return when the abort signal is called", async function () { + const delayTime = 2500; + const controller = new AbortController(); + const StandardAbortMessage = "The operation was aborted."; + const delayPromise = delay(delayTime, { + abortSignal: controller.signal, + abortErrorMsg: StandardAbortMessage, + }); + try { + controller.abort(); + await delayPromise; + assert.fail(); + } catch (err: any) { + assert.strictEqual(err.name, "AbortError"); + assert.strictEqual(err.message, StandardAbortMessage); + } + }); }); diff --git a/sdk/servicebus/service-bus/src/receivers/receiverCommon.ts b/sdk/servicebus/service-bus/src/receivers/receiverCommon.ts index 8d92c75056bd..6dfe18c1f79f 100644 --- a/sdk/servicebus/service-bus/src/receivers/receiverCommon.ts +++ b/sdk/servicebus/service-bus/src/receivers/receiverCommon.ts @@ -17,7 +17,6 @@ import { ConnectionContext } from "../connectionContext"; import { Constants, ErrorNameConditionMapper, - delay, retry, RetryConfig, RetryMode, @@ -25,7 +24,7 @@ import { RetryOptions, } from "@azure/core-amqp"; import { MessageAlreadySettled } from "../util/errors"; -import { isDefined } from "@azure/core-util"; +import { delay, isDefined } from "@azure/core-util"; /** * @internal @@ -381,11 +380,10 @@ export async function retryForever( delayInMs, config.operationType ); - await delay( - delayInMs, - config.abortSignal, - "Retry cycle has been cancelled by the user." - ); + await delay(delayInMs, { + abortSignal: config.abortSignal, + abortErrorMsg: "Retry cycle has been cancelled by the user.", + }); continue; }