diff --git a/sdk/appconfiguration/app-configuration/CHANGELOG.md b/sdk/appconfiguration/app-configuration/CHANGELOG.md index 27e9dd955ea4..4379512cee3b 100644 --- a/sdk/appconfiguration/app-configuration/CHANGELOG.md +++ b/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) diff --git a/sdk/appconfiguration/app-configuration/review/app-configuration.api.md b/sdk/appconfiguration/app-configuration/review/app-configuration.api.md index 93a9b10be30a..919ba38fb408 100644 --- a/sdk/appconfiguration/app-configuration/review/app-configuration.api.md +++ b/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; +// @public +export interface RetryOptions { + maxRetries?: number; + maxRetryDelayInMs?: number; +} + // @public export const secretReferenceContentType = "application/vnd.microsoft.appconfig.keyvaultref+json;charset=utf-8"; diff --git a/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts b/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts index 090b3ad2ba91..3cb6c491ee97 100644 --- a/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts +++ b/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( diff --git a/sdk/appconfiguration/app-configuration/src/models.ts b/sdk/appconfiguration/app-configuration/src/models.ts index 6463baae8e32..ecd8e0bab2e1 100644 --- a/sdk/appconfiguration/app-configuration/src/models.ts +++ b/sdk/appconfiguration/app-configuration/src/models.ts @@ -331,3 +331,18 @@ export interface SetReadOnlyResponse extends ConfigurationSetting, SyncTokenHeaderField, HttpResponseField {} + +/** + * 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; +} diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index 020daedd7cfb..2ad387652c8a 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/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,19 +39,32 @@ 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 { 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 @@ -55,7 +72,18 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { 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; } diff --git a/sdk/appconfiguration/app-configuration/test/internal/node/throttlingRetryPolicy.spec.ts b/sdk/appconfiguration/app-configuration/test/internal/node/throttlingRetryPolicy.spec.ts index aee3b8bd2fb1..364d64dacdf8 100644 --- a/sdk/appconfiguration/app-configuration/test/internal/node/throttlingRetryPolicy.spec.ts +++ b/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" + ); + }); });