diff --git a/sdk/appconfiguration/app-configuration/CHANGELOG.md b/sdk/appconfiguration/app-configuration/CHANGELOG.md index 01e17c719d9c..b71383a58e81 100644 --- a/sdk/appconfiguration/app-configuration/CHANGELOG.md +++ b/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) diff --git a/sdk/appconfiguration/app-configuration/package.json b/sdk/appconfiguration/app-configuration/package.json index 74fcc2d6f4f7..a8896b927af1 100644 --- a/sdk/appconfiguration/app-configuration/package.json +++ b/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", diff --git a/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts b/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts index 189021bb8e18..a703f9f04bc4 100644 --- a/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts +++ b/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts @@ -204,7 +204,6 @@ export class AppConfigurationClient { entity: keyValue, ...newOptions }); - return transformKeyValueResponse(originalResponse); }); } diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index a5b209d1a235..c53f9b599c4f 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/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 { + 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(); + }, 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,7 +89,7 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { } public async sendRequest(httpRequest: WebResource): Promise { - 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); @@ -45,7 +97,11 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { 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; } diff --git a/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts b/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts index 8f7982f92b59..d33ed58b9c6f 100644 --- a/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts +++ b/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"); + }); +});