[App Config] Throttling retry policy update - no retrying forever (#1…

…6376)

* app-config throttlign retry policy fix - no retrying forever

* typo

* update versions

* Add changelog

* Update sdk/appconfiguration/app-configuration/CHANGELOG.md

* Update sdk/appconfiguration/app-configuration/CHANGELOG.md

* Redeclare RetryOptions

* API Report

* delay(maxRetryDelayInMs) for the case of (this.retryOptions.maxRetryDelayInMs && delayInMs > this.retryOptions.maxRetryDelayInMs)

* address feedback

* remove 90 seconds default

sdk/appconfiguration/app-configuration/CHANGELOG.md

@@ -8,6 +8,10 @@
 
 ### Bugs Fixed
 
+- Throttling may have resulted in retrying the request indefinitely if the service responded with `retry-after-ms` header in the error for each retried request. The behaviour has been changed to retry for a maximum of 3 times by default from [#16376](https://github.com/Azure/azure-sdk-for-js/pull/16376).
+  - Additionally, [#16376](https://github.com/Azure/azure-sdk-for-js/pull/16376) also exposes retryOptions on the `AppConfigurationClient`'s client options, which lets you configure the `maxRetries` and the `maxRetryDelayInMs`.
+  - 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)
+
 ### Other Changes
 
 ## 1.2.0 (2021-07-07)

sdk/appconfiguration/app-configuration/review/app-configuration.api.md

@@ -37,6 +37,7 @@ export class AppConfigurationClient {
 
 // @public
 export interface AppConfigurationClientOptions {
+    retryOptions?: RetryOptions;
     userAgentOptions?: UserAgentOptions;
 }
 
@@ -174,6 +175,12 @@ export function parseFeatureFlag(setting: ConfigurationSetting): ConfigurationSe
 // @public
 export function parseSecretReference(setting: ConfigurationSetting): ConfigurationSetting<SecretReferenceValue>;
 
+// @public
+export interface RetryOptions {
+    maxRetries?: number;
+    maxRetryDelayInMs?: number;
+}
+
 // @public
 export const secretReferenceContentType = "application/vnd.microsoft.appconfig.keyvaultref+json;charset=utf-8";
 

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

@@ -35,6 +35,7 @@ import {
   ListConfigurationSettingsOptions,
   ListRevisionsOptions,
   ListRevisionsPage,
+  RetryOptions,
   SetConfigurationSettingOptions,
   SetConfigurationSettingParam,
   SetConfigurationSettingResponse,
@@ -99,6 +100,11 @@ export interface AppConfigurationClientOptions {
    * Options for adding user agent details to outgoing requests.
    */
   userAgentOptions?: UserAgentOptions;
+
+  /**
+   * Options that control how to retry failed requests.
+   */
+  retryOptions?: RetryOptions;
 }
 
 /**
@@ -529,7 +535,7 @@ export function getGeneratedClientOptions(
   const retryPolicies = [
     exponentialRetryPolicy(),
     systemErrorRetryPolicy(),
-    throttlingRetryPolicy()
+    throttlingRetryPolicy(internalAppConfigOptions.retryOptions)
   ];
 
   const userAgent = getUserAgentPrefix(

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

@@ -331,3 +331,18 @@ export interface SetReadOnlyResponse
   extends ConfigurationSetting,
     SyncTokenHeaderField,
     HttpResponseField<SyncTokenHeaderField> {}
+
+/**
+ * Options that control how to retry failed requests.
+ */
+export interface RetryOptions {
+  /**
+   * The maximum number of retry attempts.  Defaults to 3.
+   */
+  maxRetries?: number;
+
+  /**
+   * The maximum delay in milliseconds allowed before retrying an operation.
+   */
+  maxRetryDelayInMs?: number;
+}

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

@@ -13,20 +13,24 @@ import {
   RestError
 } from "@azure/core-http";
 import { delay } from "@azure/core-http";
+import { RetryOptions } from "../models";
 
 /**
  * @internal
  */
-export function throttlingRetryPolicy(): RequestPolicyFactory {
+export function throttlingRetryPolicy(retryOptions?: RetryOptions): RequestPolicyFactory {
   return {
     create: (nextPolicy: RequestPolicy, options: RequestPolicyOptions) => {
-      return new ThrottlingRetryPolicy(nextPolicy, options);
+      return new ThrottlingRetryPolicy(nextPolicy, options, retryOptions);
     }
   };
 }
 
 const StandardAbortMessage = "The operation was aborted.";
 
+// Merge this constant with the one in core-http when we unify throttling retry policy in core-http and app-config
+const DEFAULT_CLIENT_RETRY_COUNT = 3;
+
 /**
  * This policy is a close copy of the ThrottlingRetryPolicy class from
  * core-http with modifications to work with how AppConfig is currently
@@ -35,27 +39,51 @@ const StandardAbortMessage = "The operation was aborted.";
  * @internal
  */
 export class ThrottlingRetryPolicy extends BaseRequestPolicy {
-  constructor(nextPolicy: RequestPolicy, options: RequestPolicyOptions) {
+  private numberOfRetries = 0;
+  constructor(
+    nextPolicy: RequestPolicy,
+    options: RequestPolicyOptions,
+    private retryOptions: RetryOptions = { maxRetries: DEFAULT_CLIENT_RETRY_COUNT }
+  ) {
     super(nextPolicy, options);
   }
 
   public async sendRequest(httpRequest: WebResource): Promise<HttpOperationResponse> {
     return this._nextPolicy.sendRequest(httpRequest.clone()).catch(async (err) => {
       if (isRestErrorWithHeaders(err)) {
-        const delayInMs = getDelayInMs(err.response.headers);
+        let delayInMs = getDelayInMs(err.response.headers);
 
         if (delayInMs == null) {
           throw err;
         }
 
+        if (
+          this.retryOptions.maxRetryDelayInMs &&
+          delayInMs > this.retryOptions.maxRetryDelayInMs
+        ) {
+          delayInMs = this.retryOptions.maxRetryDelayInMs;
+        }
+
+        this.numberOfRetries += 1;
         await delay(delayInMs, undefined, {
           abortSignal: httpRequest.abortSignal,
           abortErrorMsg: StandardAbortMessage
         });
         if (httpRequest.abortSignal?.aborted) {
           throw new AbortError(StandardAbortMessage);
         }
-        return await this.sendRequest(httpRequest.clone());
+
+        if (this.retryOptions.maxRetries == undefined) {
+          this.retryOptions.maxRetries = DEFAULT_CLIENT_RETRY_COUNT;
+        }
+
+        if (this.numberOfRetries < this.retryOptions.maxRetries) {
+          // retries
+          return this.sendRequest(httpRequest.clone());
+        } else {
+          // passes on to the next policy
+          return this._nextPolicy.sendRequest(httpRequest.clone());
+        }
       } else {
         throw err;
       }

sdk/appconfiguration/app-configuration/test/internal/node/throttlingRetryPolicy.spec.ts

@@ -7,16 +7,16 @@ import { AbortController } from "@azure/abort-controller";
 import nock from "nock";
 import { generateUuid } from "@azure/core-http";
 
-describe("Should not retry forever - honors the abort signal passed", () => {
+describe("Should not retry forever", () => {
   let client: AppConfigurationClient;
   const connectionString = "Endpoint=https://myappconfig.azconfig.io;Id=key:ai/u/fake;Secret=abcd=";
 
-  beforeEach(function() {
+  function mockErrorResponse(retryAfterMs: string, persistence: boolean = true) {
     if (!nock.isActive()) {
       nock.activate();
     }
     nock("https://myappconfig.azconfig.io:443")
-      .persist()
+      .persist(persistence)
       .put(/.*/g)
       .reply(
         429,
@@ -26,9 +26,11 @@ describe("Should not retry forever - honors the abort signal passed", () => {
           policy: "Total Requests",
           status: 429
         },
-        ["retry-after-ms", "123456"]
+        ["retry-after-ms", retryAfterMs]
       );
+  }
 
+  beforeEach(() => {
     client = new AppConfigurationClient(connectionString);
   });
 
@@ -38,7 +40,8 @@ describe("Should not retry forever - honors the abort signal passed", () => {
     nock.enableNetConnect();
   });
 
-  it("simulate the service throttling", async () => {
+  it("simulate the service throttling - honors the abort signal passed", async () => {
+    mockErrorResponse("123456");
     const key = generateUuid();
     const numberOfSettings = 200;
     const promises = [];
@@ -64,4 +67,40 @@ describe("Should not retry forever - honors the abort signal passed", () => {
     }
     chai.assert.equal(errorWasThrown, true, "Error was not thrown");
   });
+
+  it("should not retry forever without abortSignal", async () => {
+    const responseCount = 10;
+    for (let index = 0; index < responseCount; index++) {
+      mockErrorResponse("100", false);
+    }
+    const key = generateUuid();
+    let errorWasThrown = false;
+
+    chai.assert.equal(
+      nock.pendingMocks().length,
+      responseCount,
+      "unexpected pending mocks before making the request"
+    );
+    try {
+      await client.addConfigurationSetting({
+        key: key,
+        value: "added"
+      });
+    } catch (error) {
+      errorWasThrown = true;
+      chai.assert.equal(error.name, "RestError", "Unexpected error thrown");
+      chai.assert.equal(JSON.parse(error.message).status, 429, "Unexpected error thrown");
+      chai.assert.equal(
+        JSON.parse(error.message).title,
+        "Resource utilization has surpassed the assigned quota",
+        "Unexpected error thrown"
+      );
+    }
+    chai.assert.equal(errorWasThrown, true, "Error was not thrown");
+    chai.assert.equal(
+      nock.pendingMocks().length,
+      responseCount - 1 - 3, // one attempt + three retries
+      "unexpected pending mocks after the test was run"
+    );
+  });
 });