[App Config] Handle throttling - do not hang - should honor abort signal

sdk/appconfiguration/app-configuration/CHANGELOG.md

@@ -10,6 +10,9 @@
 
 ### Fixed
 
+- High request rate would result in throttling. SDK would retry on the failed requests based on the service suggested time from the `retry-after-ms` header in the error response. If there are too many parallel requests, retries for all of them may also result in a high request rate entering into a state which might seem like the application is hanging forever.
+  - [#15721](https://github.com/Azure/azure-sdk-for-js/pull/15721) allows the user-provided abortSignal to be taken into account to abort the requests sooner.
+  - More resources - [App Configuration | Throttling](https://docs.microsoft.com/azure/azure-app-configuration/rest-api-throttling) and [App Configuration | Requests Quota](https://docs.microsoft.com/azure/azure-app-configuration/faq#which-app-configuration-tier-should-i-use)
 
 ## 1.2.0-beta.2 (2021-06-08)
 

sdk/appconfiguration/app-configuration/package.json

@@ -86,6 +86,7 @@
     ]
   },
   "dependencies": {
+    "@azure/abort-controller": "^1.0.0",
     "@azure/core-asynciterator-polyfill": "^1.0.0",
     "@azure/core-http": "^1.2.0",
     "@azure/core-paging": "^1.1.1",

sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts

@@ -204,7 +204,6 @@ export class AppConfigurationClient {
         entity: keyValue,
         ...newOptions
       });
-
       return transformKeyValueResponse(originalResponse);
     });
   }

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

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
+import { AbortError, AbortSignalLike } from "@azure/abort-controller";
 import {
   BaseRequestPolicy,
   RequestPolicy,
@@ -9,9 +10,9 @@ import {
   WebResource,
   HttpOperationResponse,
   Constants,
-  delay,
   RestError
 } from "@azure/core-http";
+import { isDefined } from "../internal/typeguards";
 
 /**
  * @internal
@@ -24,6 +25,57 @@ 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
@@ -37,15 +89,19 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy {
   }
 
   public async sendRequest(httpRequest: WebResource): Promise<HttpOperationResponse> {
-    return this._nextPolicy.sendRequest(httpRequest.clone()).catch((err) => {
+    return this._nextPolicy.sendRequest(httpRequest.clone()).catch(async (err) => {
       if (isRestErrorWithHeaders(err)) {
         const delayInMs = getDelayInMs(err.response.headers);
 
         if (delayInMs == null) {
           throw err;
         }
 
-        return delay(delayInMs).then((_: any) => this.sendRequest(httpRequest.clone()));
+        await delay(delayInMs, httpRequest.abortSignal, StandardAbortMessage);
+        if (httpRequest.abortSignal?.aborted) {
+          throw new AbortError(StandardAbortMessage);
+        }
+        return await this.sendRequest(httpRequest.clone());
       } else {
         throw err;
       }

sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts

@@ -13,6 +13,10 @@ import {
 } from "@azure/core-http";
 import { ThrottlingRetryPolicy, getDelayInMs } from "../../src/policies/throttlingRetryPolicy";
 import { assertThrowsRestError } from "../public/utils/testHelpers";
+import { AppConfigurationClient } from "../../src";
+import { AbortController } from "@azure/abort-controller";
+import nock from "nock";
+import { generateUuid } from "@azure/core-http";
 
 describe("ThrottlingRetryPolicy", () => {
   class PassThroughPolicy {
@@ -188,3 +192,62 @@ describe("ThrottlingRetryPolicy", () => {
     });
   });
 });
+
+describe("Should not retry forever - honors the abort signal passed", () => {
+  let client: AppConfigurationClient;
+  const connectionString = "Endpoint=https://myappconfig.azconfig.io;Id=key:ai/u/fake;Secret=abcd=";
+
+  beforeEach(function() {
+    if (!nock.isActive()) {
+      nock.activate();
+    }
+    nock("https://myappconfig.azconfig.io:443")
+      .persist()
+      .put(/.*/g)
+      .reply(
+        429,
+        {
+          type: "https://azconfig.io/errors/too-many-requests",
+          title: "Resource utilization has surpassed the assigned quota",
+          policy: "Total Requests",
+          status: 429
+        },
+        ["retry-after-ms", "123456"]
+      );
+
+    client = new AppConfigurationClient(connectionString);
+  });
+
+  afterEach(async function() {
+    nock.restore();
+    nock.cleanAll();
+    nock.enableNetConnect();
+  });
+
+  it("simulate the service throttling", async () => {
+    const key = generateUuid();
+    const numberOfSettings = 200;
+    const promises = [];
+    let errorWasThrown = false;
+    try {
+      for (let index = 0; index < numberOfSettings; index++) {
+        promises.push(
+          client.addConfigurationSetting(
+            {
+              key: key + "-" + index,
+              value: "added"
+            },
+            {
+              abortSignal: AbortController.timeout(1000)
+            }
+          )
+        );
+      }
+      await Promise.all(promises);
+    } catch (error) {
+      errorWasThrown = true;
+      chai.assert.equal((error as any).name, "AbortError", "Unexpected error thrown");
+    }
+    chai.assert.equal(errorWasThrown, true, "Error was not thrown");
+  });
+});