[core-http] Throttling retry policy fix in core-http

sdk/appconfiguration/app-configuration/package.json

@@ -91,6 +91,7 @@
     "@azure/core-http": "^1.2.0",
     "@azure/core-paging": "^1.1.1",
     "@azure/core-tracing": "1.0.0-preview.11",
+    "@azure/core-util": "^1.0.0-beta.1",
     "tslib": "^2.2.0"
   },
   "devDependencies": {

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { AbortError, AbortSignalLike } from "@azure/abort-controller";
+import { AbortError } from "@azure/abort-controller";
 import {
   BaseRequestPolicy,
   RequestPolicy,
@@ -12,7 +12,7 @@ import {
   Constants,
   RestError
 } from "@azure/core-http";
-import { isDefined } from "../internal/typeguards";
+import { delay } from "@azure/core-util";
 
 /**
  * @internal
@@ -27,55 +27,6 @@ export function throttlingRetryPolicy(): RequestPolicyFactory {
 
 const StandardAbortMessage = "The operation was aborted.";
 
-/**
- * A wrapper for setTimeout that resolves a promise after t milliseconds.
- * @param delayInMs - The number of milliseconds to be delayed.
- * @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,
-  abortSignal?: AbortSignalLike,
-  abortErrorMsg?: string
-): Promise<void> {
-  return new Promise((resolve, reject) => {
-    let timer: ReturnType<typeof setTimeout> | 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();
-    }, delayInMs);
-
-    if (abortSignal) {
-      abortSignal.addEventListener("abort", onAborted);
-    }
-  });
-}
-
 /**
  * This policy is a close copy of the ThrottlingRetryPolicy class from
  * core-http with modifications to work with how AppConfig is currently

sdk/core/core-amqp/package.json

@@ -70,6 +70,7 @@
   },
   "dependencies": {
     "@azure/abort-controller": "^1.0.0",
+    "@azure/core-util": "^1.0.0-beta.1",
     "@azure/core-auth": "^1.3.0",
     "@azure/logger": "^1.0.0",
     "buffer": "^5.2.1",

sdk/core/core-amqp/src/index.ts

@@ -31,12 +31,12 @@ export {
   StandardAbortMessage
 } from "./errors";
 export {
-  delay,
   parseConnectionString,
   defaultCancellableLock,
   ParsedOutput,
   WebSocketOptions
 } from "./util/utils";
+export { delay } from "@azure/core-util";
 export { AmqpAnnotatedMessage } from "./amqpAnnotatedMessage";
 export { logger } from "./log";
 export * from "./internals";

sdk/core/core-amqp/src/util/utils.ts

@@ -1,10 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { AbortError, AbortSignalLike } from "@azure/abort-controller";
 import { WebSocketImpl } from "rhea-promise";
-import { isDefined } from "./typeGuards";
-import { StandardAbortMessage } from "../errors";
 import { CancellableAsyncLock, CancellableAsyncLockImpl } from "./lock";
 
 /**
@@ -168,57 +165,6 @@ export class Timeout {
   }
 }
 
-/**
- * A wrapper for setTimeout that resolves a promise after t milliseconds.
- * @param delayInMs - The number of milliseconds to be delayed.
- * @param abortSignal - The abortSignal associated with containing operation.
- * @param abortErrorMsg - The abort error message associated with containing operation.
- * @param value - The value to be resolved with after a timeout of t milliseconds.
- * @returns - Resolved promise
- */
-export function delay<T>(
-  delayInMs: number,
-  abortSignal?: AbortSignalLike,
-  abortErrorMsg?: string,
-  value?: T
-): Promise<T | void> {
-  return new Promise((resolve, reject) => {
-    let timer: ReturnType<typeof setTimeout> | 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);
-    }
-  });
-}
-
 /**
  * @internal
  *

sdk/core/core-http/CHANGELOG.md

@@ -11,6 +11,7 @@
 ### Fixed
 
 - Fixed an issue where `proxySettings` does not work when there is username but no password [Issue 15720](https://github.com/Azure/azure-sdk-for-js/issues/15720)
+- Throttling retry policy respects abort signal [#15796](https://github.com/Azure/azure-sdk-for-js/issues/15796)
 
 ## 1.2.6 (2021-06-14)
 

sdk/core/core-http/package.json

@@ -130,6 +130,7 @@
     "@azure/core-asynciterator-polyfill": "^1.0.0",
     "@azure/core-auth": "^1.3.0",
     "@azure/core-tracing": "1.0.0-preview.11",
+    "@azure/core-util": "^1.0.0-beta.1",
     "@azure/logger": "^1.0.0",
     "@types/node-fetch": "^2.5.0",
     "@types/tunnel": "^0.0.1",
@@ -179,6 +180,7 @@
     "karma-sourcemap-loader": "^0.3.8",
     "mocha": "^7.1.1",
     "mocha-junit-reporter": "^1.18.0",
+    "nock": "^12.0.3",
     "npm-run-all": "^4.1.5",
     "nyc": "^14.0.0",
     "prettier": "^1.16.4",

sdk/core/core-http/src/policies/throttlingRetryPolicy.ts

@@ -10,7 +10,8 @@ import {
 import { WebResourceLike } from "../webResource";
 import { HttpOperationResponse } from "../httpOperationResponse";
 import { Constants } from "../util/constants";
-import { delay } from "../util/utils";
+import { delay } from "@azure/core-util";
+import { AbortError } from "@azure/abort-controller";
 
 type ResponseHandler = (
   httpRequest: WebResourceLike,
@@ -26,6 +27,8 @@ export function throttlingRetryPolicy(): RequestPolicyFactory {
   };
 }
 
+const StandardAbortMessage = "The operation was aborted.";
+
 /**
  * To learn more, please refer to
  * https://docs.microsoft.com/en-us/azure/azure-resource-manager/resource-manager-request-limits,
@@ -67,7 +70,11 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy {
         retryAfterHeader
       );
       if (delayInMs) {
-        return delay(delayInMs).then((_: any) => this._nextPolicy.sendRequest(httpRequest));
+        await delay(delayInMs, httpRequest.abortSignal, StandardAbortMessage);
+        if (httpRequest.abortSignal?.aborted) {
+          throw new AbortError(StandardAbortMessage);
+        }
+        return await this._nextPolicy.sendRequest(httpRequest);
       }
     }
 

sdk/core/core-http/test/policies/throttlingRetryPolicy.node.ts

@@ -0,0 +1,49 @@
+import nock from "nock";
+import { Constants, ServiceClient } from "../../src/coreHttp";
+import { AbortController } from "@azure/abort-controller";
+import { assert } from "chai";
+
+describe("Throttling retry policy", () => {
+  let client: ServiceClient;
+
+  beforeEach(function() {
+    if (!nock.isActive()) {
+      nock.activate();
+    }
+    nock("https://fakeservice.io:443")
+      .persist()
+      .put(/.*/g)
+      .reply(
+        Constants.HttpConstants.StatusCodes.TooManyRequests,
+        {
+          type: "https://fakeservice.io/errors/too-many-requests",
+          title: "Resource utilization has surpassed the assigned quota",
+          policy: "Total Requests",
+          status: Constants.HttpConstants.StatusCodes.TooManyRequests
+        },
+        ["Retry-After", "10000"]
+      );
+    client = new ServiceClient();
+  });
+
+  afterEach(async function() {
+    nock.restore();
+    nock.cleanAll();
+    nock.enableNetConnect();
+  });
+
+  it("Should not retry forever (honors the abort signal passed)", async () => {
+    let errorWasThrown = false;
+    try {
+      await client.sendRequest({
+        url: "https://fakeservice.io/ABCD",
+        abortSignal: AbortController.timeout(100),
+        method: "PUT"
+      });
+    } catch (error) {
+      errorWasThrown = true;
+      assert.equal((error as any).name, "AbortError", "Unexpected error thrown");
+    }
+    assert.equal(errorWasThrown, true, "Error was not thrown");
+  });
+});

sdk/core/core-util/package.json

@@ -68,7 +68,8 @@
   "sideEffects": false,
   "prettier": "@azure/eslint-plugin-azure-sdk/prettier.json",
   "dependencies": {
-    "tslib": "^2.2.0"
+    "tslib": "^2.2.0",
+    "@azure/abort-controller": "^1.0.0"
   },
   "devDependencies": {
     "@azure/dev-tool": "^1.0.0",

sdk/core/core-util/review/core-util.api.md

@@ -4,8 +4,10 @@
 
 ```ts
 
+import { AbortSignalLike } from '@azure/abort-controller';
+
 // @public
-export function delay(timeInMs: number): Promise<void>;
+export function delay<T>(delayInMs: number, abortSignal?: AbortSignalLike, abortErrorMsg?: string, value?: T): Promise<T | void>;
 
 // @public
 export const isNode: boolean;

sdk/core/core-util/src/delay.ts

@@ -1,11 +1,57 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
+import { isDefined } from "./typeguards";
+import { AbortError, AbortSignalLike } from "@azure/abort-controller";
+const StandardAbortMessage = "The operation was aborted.";
+
 /**
- * A wrapper for setTimeout that resolves a promise after timeInMs milliseconds.
- * @param timeInMs - The number of milliseconds to be delayed.
- * @returns Promise that is resolved after timeInMs
+ * A wrapper for setTimeout that resolves a promise after delayInMs milliseconds.
+ * @param delayInMs - The number of milliseconds to be delayed.
+ * @param abortSignal - The abortSignal associated with containing operation.
+ * @param abortErrorMsg - The abort error message associated with containing operation.
+ * @param value - The value to be resolved with after a timeout of t milliseconds.
+ * @returns - Resolved promise
  */
-export function delay(timeInMs: number): Promise<void> {
-  return new Promise((resolve) => setTimeout(() => resolve(), timeInMs));
+export function delay<T>(
+  delayInMs: number,
+  abortSignal?: AbortSignalLike,
+  abortErrorMsg?: string,
+  value?: T
+): Promise<T | void> {
+  return new Promise((resolve, reject) => {
+    let timer: ReturnType<typeof setTimeout> | 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);
+    }
+  });
 }

sdk/core/core-util/src/typeguards.ts

@@ -0,0 +1,11 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/**
+ * Helper TypeGuard that checks if something is defined or not.
+ * @param thing - Anything
+ * @internal
+ */
+export function isDefined<T>(thing: T | undefined | null): thing is T {
+  return typeof thing !== "undefined" && thing !== null;
+}