From 595570913c9c761617831eeeda80b19aeaea7faa Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Fri, 11 Jun 2021 17:14:28 -0700 Subject: [PATCH 01/16] Add abort-controller in dependencies --- sdk/appconfiguration/app-configuration/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/appconfiguration/app-configuration/package.json b/sdk/appconfiguration/app-configuration/package.json index 89f2fd328676..1645e0802ab3 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", From 16a5238f770cf4b7865c820a693a172a8d6c1306 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Fri, 11 Jun 2021 17:14:40 -0700 Subject: [PATCH 02/16] src --- .../app-configuration/src/appConfigurationClient.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts b/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts index d5b3d2b7ec7a..7f37dd515fd0 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); }); } From f6270b7acc6eee1d3aaafec01e9af6621293e8f3 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Fri, 11 Jun 2021 17:15:04 -0700 Subject: [PATCH 03/16] Update ThrottlingRetryPolicy.sendRequest --- .../src/policies/throttlingRetryPolicy.ts | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index a5b209d1a235..8638dfba1c98 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts @@ -12,6 +12,7 @@ import { delay, RestError } from "@azure/core-http"; +import { AbortError } from "@azure/abort-controller"; /** * @internal @@ -24,6 +25,23 @@ export function throttlingRetryPolicy(): RequestPolicyFactory { }; } +/** + * Maximum wait duration for the expected event to happen = `10000 ms`(default value is 10 seconds)(= maxWaitTimeInMilliseconds) + * Keep checking whether the predicate is true after every `1000 ms`(default value is 1 second) (= delayBetweenRetriesInMilliseconds) + */ +export async function checkWithTimeout( + predicate: () => boolean | Promise, + delayBetweenRetriesInMilliseconds: number = 1000, + maxWaitTimeInMilliseconds: number = 10000 +): Promise { + const maxTime = Date.now() + maxWaitTimeInMilliseconds; + while (Date.now() < maxTime) { + if (await predicate()) return true; + await delay(delayBetweenRetriesInMilliseconds); + } + return false; +} + /** * This policy is a close copy of the ThrottlingRetryPolicy class from * core-http with modifications to work with how AppConfig is currently @@ -37,7 +55,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 +63,11 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { throw err; } - return delay(delayInMs).then((_: any) => this.sendRequest(httpRequest.clone())); + await checkWithTimeout(() => httpRequest.abortSignal?.aborted === true, 1, delayInMs); + if (httpRequest.abortSignal?.aborted) { + throw new AbortError("The operation was aborted."); + } + return await this.sendRequest(httpRequest.clone()); } else { throw err; } From ee6c764c87eeeed826ab464fde0dbffc5f3dda44 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Fri, 11 Jun 2021 17:15:18 -0700 Subject: [PATCH 04/16] test for throttling policy --- .../test/public/throttling.spec.ts | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts diff --git a/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts b/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts new file mode 100644 index 000000000000..082eccd0ece1 --- /dev/null +++ b/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts @@ -0,0 +1,50 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { createAppConfigurationClientForTests, startRecorder } from "./utils/testHelpers"; +import { AppConfigurationClient } from "../../src"; +import { Recorder } from "@azure/test-utils-recorder"; +import { Context } from "mocha"; +import { AbortController } from "@azure/abort-controller"; + +describe("AppConfigurationClient", () => { + let client: AppConfigurationClient; + let recorder: Recorder; + + beforeEach(function(this: Context) { + recorder = startRecorder(this); + client = createAppConfigurationClientForTests() || this.skip(); + }); + + afterEach(async function(this: Context) { + await recorder.stop(); + }); + + describe.only("simple usages", () => { + it("Add and query a setting without a label", async () => { + const key = recorder.getUniqueName("noLabelTests"); + const numberOfSettings = 2000; + const promises = []; + try { + for (let index = 0; index < numberOfSettings; index++) { + promises.push( + client.addConfigurationSetting( + { + key: key + " " + +index, + value: "added" + }, + { + abortSignal: AbortController.timeout(10000) + } + ) + ); + } + await Promise.all(promises); + } catch (error) { + console.log(error); + } + + await client.deleteConfigurationSetting({ key }); + }); + }); +}); From bea461766fe3ccf16fec17cb8a4f50a583a5e0c1 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Fri, 11 Jun 2021 17:19:17 -0700 Subject: [PATCH 05/16] delayBetweenRechecksInMs --- .../src/policies/throttlingRetryPolicy.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index 8638dfba1c98..fab49920150f 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts @@ -63,7 +63,12 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { throw err; } - await checkWithTimeout(() => httpRequest.abortSignal?.aborted === true, 1, delayInMs); + const delayBetweenRechecksInMs = 1; + await checkWithTimeout( + () => httpRequest.abortSignal?.aborted === true, + delayBetweenRechecksInMs, + delayInMs + ); if (httpRequest.abortSignal?.aborted) { throw new AbortError("The operation was aborted."); } From 613407c2c0fa5ebbfe96e9e7886e74fce7968ced Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 15 Jun 2021 14:37:40 -0700 Subject: [PATCH 06/16] throttling retry policy updates --- .../src/policies/throttlingRetryPolicy.ts | 111 ++++++++++++++---- 1 file changed, 87 insertions(+), 24 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index fab49920150f..d7cfba0d711b 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,10 +10,8 @@ import { WebResource, HttpOperationResponse, Constants, - delay, RestError } from "@azure/core-http"; -import { AbortError } from "@azure/abort-controller"; /** * @internal @@ -25,21 +24,87 @@ export function throttlingRetryPolicy(): RequestPolicyFactory { }; } +const StandardAbortMessage = "The operation was aborted."; + +/** + * An executor for a function that returns a Promise that obeys both a timeout and an + * optional AbortSignal. + * @param actionFn - The callback that we want to resolve. + * @param timeoutMs - The number of milliseconds to allow before throwing an OperationTimeoutError. + * @param timeoutMessage - The message to place in the .description field for the thrown exception for Timeout. + * @param abortSignal - The abortSignal associated with containing operation. + * + * @internal + */ +export async function waitForTimeoutOrAbortOrResolve(args: { + actionFn: () => Promise; + timeoutMs: number; + timeoutMessage: string; + abortSignal: AbortSignalLike | undefined; +}): Promise { + if (args.abortSignal && args.abortSignal.aborted) { + throw new AbortError(StandardAbortMessage); + } + + let timer: any | undefined = undefined; + let clearAbortSignal: (() => void) | undefined = undefined; + + const clearAbortSignalAndTimer = (): void => { + clearTimeout(timer); + + if (clearAbortSignal) { + clearAbortSignal(); + } + }; + + const abortOrTimeoutPromise = new Promise((_resolve, reject) => { + clearAbortSignal = checkAndRegisterWithAbortSignal(reject, args.abortSignal); + + timer = setTimeout(() => { + reject(new Error(args.timeoutMessage)); + }, args.timeoutMs); + }); + + try { + return await Promise.race([abortOrTimeoutPromise, args.actionFn()]); + } finally { + clearAbortSignalAndTimer(); + } +} + /** - * Maximum wait duration for the expected event to happen = `10000 ms`(default value is 10 seconds)(= maxWaitTimeInMilliseconds) - * Keep checking whether the predicate is true after every `1000 ms`(default value is 1 second) (= delayBetweenRetriesInMilliseconds) + * Registers listener to the abort event on the abortSignal to call your abortFn and + * returns a function that will clear the same listener. + * + * If abort signal is already aborted, then throws an AbortError and returns a function that does nothing + * + * @returns A function that removes any of our attached event listeners on the abort signal or an empty function if + * the abortSignal was not defined. + * + * @internal */ -export async function checkWithTimeout( - predicate: () => boolean | Promise, - delayBetweenRetriesInMilliseconds: number = 1000, - maxWaitTimeInMilliseconds: number = 10000 -): Promise { - const maxTime = Date.now() + maxWaitTimeInMilliseconds; - while (Date.now() < maxTime) { - if (await predicate()) return true; - await delay(delayBetweenRetriesInMilliseconds); +export function checkAndRegisterWithAbortSignal( + onAbortFn: (abortError: AbortError) => void, + abortSignal?: AbortSignalLike +): () => void { + if (abortSignal == null) { + return () => { + /** Nothing to do here, no abort signal */ + }; } - return false; + + if (abortSignal.aborted) { + throw new AbortError(StandardAbortMessage); + } + + const onAbort = (): void => { + abortSignal.removeEventListener("abort", onAbort); + onAbortFn(new AbortError(StandardAbortMessage)); + }; + + abortSignal.addEventListener("abort", onAbort); + + return () => abortSignal.removeEventListener("abort", onAbort); } /** @@ -63,16 +128,14 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { throw err; } - const delayBetweenRechecksInMs = 1; - await checkWithTimeout( - () => httpRequest.abortSignal?.aborted === true, - delayBetweenRechecksInMs, - delayInMs - ); - if (httpRequest.abortSignal?.aborted) { - throw new AbortError("The operation was aborted."); - } - return await this.sendRequest(httpRequest.clone()); + return await waitForTimeoutOrAbortOrResolve({ + timeoutMs: delayInMs, + abortSignal: httpRequest.abortSignal, + actionFn: async () => { + return await this.sendRequest(httpRequest.clone()); + }, + timeoutMessage: `ServiceBusy: Unable to fulfill the request in ${delayInMs}ms when retried.` + }); } else { throw err; } From 6579a13de895966cf40bf98aa4a45683791e15e5 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Tue, 15 Jun 2021 14:37:48 -0700 Subject: [PATCH 07/16] test --- .../test/public/throttling.spec.ts | 51 ++++++++++++------- 1 file changed, 32 insertions(+), 19 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts b/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts index 082eccd0ece1..f62ac3914304 100644 --- a/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts +++ b/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts @@ -23,28 +23,41 @@ describe("AppConfigurationClient", () => { describe.only("simple usages", () => { it("Add and query a setting without a label", async () => { const key = recorder.getUniqueName("noLabelTests"); - const numberOfSettings = 2000; - const promises = []; - try { - for (let index = 0; index < numberOfSettings; index++) { - promises.push( - client.addConfigurationSetting( - { - key: key + " " + +index, - value: "added" - }, - { - abortSignal: AbortController.timeout(10000) - } - ) - ); + const numberOfSettings = 200; + const times = 1000; + for (let time = 0; time < times; time++) { + const promises = []; + try { + for (let index = 0; index < numberOfSettings; index++) { + promises.push( + client.addConfigurationSetting( + { + key: key + " " + +index, + value: "added" + }, + { + abortSignal: AbortController.timeout(10000) + } + ) + ); + } + await Promise.all(promises); + } catch (error) { + console.log(error); } - await Promise.all(promises); - } catch (error) { - console.log(error); } - await client.deleteConfigurationSetting({ key }); + await cleanupSampleValues([key], client); }); }); }); + +async function cleanupSampleValues(keys: string[], client: AppConfigurationClient) { + const settingsIterator = client.listConfigurationSettings({ + keyFilter: keys.join(",") + }); + + for await (const setting of settingsIterator) { + await client.deleteConfigurationSetting({ key: setting.key, label: setting.label }); + } +} From 93a2913f4b2797d4d3a000cab9433026e4de088c Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 09:54:33 -0700 Subject: [PATCH 08/16] address Richard's feedback --- .../src/policies/throttlingRetryPolicy.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index d7cfba0d711b..3db86f5b4fb5 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts @@ -83,7 +83,7 @@ export async function waitForTimeoutOrAbortOrResolve(args: { * * @internal */ -export function checkAndRegisterWithAbortSignal( +function checkAndRegisterWithAbortSignal( onAbortFn: (abortError: AbortError) => void, abortSignal?: AbortSignalLike ): () => void { @@ -131,10 +131,8 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { return await waitForTimeoutOrAbortOrResolve({ timeoutMs: delayInMs, abortSignal: httpRequest.abortSignal, - actionFn: async () => { - return await this.sendRequest(httpRequest.clone()); - }, - timeoutMessage: `ServiceBusy: Unable to fulfill the request in ${delayInMs}ms when retried.` + actionFn: () => this.sendRequest(httpRequest.clone()), + timeoutMessage: `Unable to fulfill the request in ${delayInMs}ms when retried.` }); } else { throw err; From fe63f7b802642c2d9a808d0c1743c5eb2637bc5e Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 13:52:57 -0700 Subject: [PATCH 09/16] delete the live test --- .../test/public/throttling.spec.ts | 63 ------------------- 1 file changed, 63 deletions(-) delete mode 100644 sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts diff --git a/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts b/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts deleted file mode 100644 index f62ac3914304..000000000000 --- a/sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -import { createAppConfigurationClientForTests, startRecorder } from "./utils/testHelpers"; -import { AppConfigurationClient } from "../../src"; -import { Recorder } from "@azure/test-utils-recorder"; -import { Context } from "mocha"; -import { AbortController } from "@azure/abort-controller"; - -describe("AppConfigurationClient", () => { - let client: AppConfigurationClient; - let recorder: Recorder; - - beforeEach(function(this: Context) { - recorder = startRecorder(this); - client = createAppConfigurationClientForTests() || this.skip(); - }); - - afterEach(async function(this: Context) { - await recorder.stop(); - }); - - describe.only("simple usages", () => { - it("Add and query a setting without a label", async () => { - const key = recorder.getUniqueName("noLabelTests"); - const numberOfSettings = 200; - const times = 1000; - for (let time = 0; time < times; time++) { - const promises = []; - try { - for (let index = 0; index < numberOfSettings; index++) { - promises.push( - client.addConfigurationSetting( - { - key: key + " " + +index, - value: "added" - }, - { - abortSignal: AbortController.timeout(10000) - } - ) - ); - } - await Promise.all(promises); - } catch (error) { - console.log(error); - } - } - - await cleanupSampleValues([key], client); - }); - }); -}); - -async function cleanupSampleValues(keys: string[], client: AppConfigurationClient) { - const settingsIterator = client.listConfigurationSettings({ - keyFilter: keys.join(",") - }); - - for await (const setting of settingsIterator) { - await client.deleteConfigurationSetting({ key: setting.key, label: setting.label }); - } -} From 1ca2bb6c35c4890bb6ccca062c56d33df0b03ae5 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 13:58:08 -0700 Subject: [PATCH 10/16] Should not retry forever - honors the abort signal passed test --- .../throttlingRetryPolicyTests.spec.ts | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts b/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts index 8f7982f92b59..8c271a4a79b9 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,57 @@ 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 = []; + 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 (_) {} + }); +}); From 596f4cc957b27761a13cae3540f8991f7395405c Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 14:21:15 -0700 Subject: [PATCH 11/16] changelog --- sdk/appconfiguration/app-configuration/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sdk/appconfiguration/app-configuration/CHANGELOG.md b/sdk/appconfiguration/app-configuration/CHANGELOG.md index 01e17c719d9c..221d1c067bf9 100644 --- a/sdk/appconfiguration/app-configuration/CHANGELOG.md +++ b/sdk/appconfiguration/app-configuration/CHANGELOG.md @@ -8,8 +8,11 @@ ### Key Bugs Fixed -### 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/en-us/azure/azure-app-configuration/rest-api-throttling) and [App Configuration | Requests Quota](https://docs.microsoft.com/en-us/azure/azure-app-configuration/faq#which-app-configuration-tier-should-i-use) +### Fixed ## 1.2.0-beta.2 (2021-06-08) From 9becefea9676568923f63db72f4af8bf3e855bd7 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 14:22:09 -0700 Subject: [PATCH 12/16] changelog update --- sdk/appconfiguration/app-configuration/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/CHANGELOG.md b/sdk/appconfiguration/app-configuration/CHANGELOG.md index 221d1c067bf9..9c9fe24b274c 100644 --- a/sdk/appconfiguration/app-configuration/CHANGELOG.md +++ b/sdk/appconfiguration/app-configuration/CHANGELOG.md @@ -8,12 +8,12 @@ ### Key Bugs Fixed +### 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/en-us/azure/azure-app-configuration/rest-api-throttling) and [App Configuration | Requests Quota](https://docs.microsoft.com/en-us/azure/azure-app-configuration/faq#which-app-configuration-tier-should-i-use) -### Fixed - ## 1.2.0-beta.2 (2021-06-08) - With [#15136](https://github.com/Azure/azure-sdk-for-js/pull/15136), if the key of a feature flag(setting with `contentType="application/vnd.microsoft.appconfig.ff+json;charset=utf-8"`) doesn't start with `".appconfig.featureflag/"` (featureFlagPrefix), SDK adds the prefix before sending the request. From dc204db55826cde91a8851116d858897e1928476 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 14:59:44 -0700 Subject: [PATCH 13/16] delay method with abort signal --- .../src/policies/throttlingRetryPolicy.ts | 118 +++++++----------- 1 file changed, 43 insertions(+), 75 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index 3db86f5b4fb5..bdeed6aecbcc 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts @@ -12,6 +12,7 @@ import { Constants, RestError } from "@azure/core-http"; +import { isDefined } from "../internal/typeguards"; /** * @internal @@ -27,84 +28,52 @@ export function throttlingRetryPolicy(): RequestPolicyFactory { const StandardAbortMessage = "The operation was aborted."; /** - * An executor for a function that returns a Promise that obeys both a timeout and an - * optional AbortSignal. - * @param actionFn - The callback that we want to resolve. - * @param timeoutMs - The number of milliseconds to allow before throwing an OperationTimeoutError. - * @param timeoutMessage - The message to place in the .description field for the thrown exception for 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. - * - * @internal + * @param abortErrorMsg - The abort error message associated with containing operation. + * @returns - Resolved promise */ -export async function waitForTimeoutOrAbortOrResolve(args: { - actionFn: () => Promise; - timeoutMs: number; - timeoutMessage: string; - abortSignal: AbortSignalLike | undefined; -}): Promise { - if (args.abortSignal && args.abortSignal.aborted) { - throw new AbortError(StandardAbortMessage); - } +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)); + }; - let timer: any | undefined = undefined; - let clearAbortSignal: (() => void) | undefined = undefined; + const removeListeners = (): void => { + if (abortSignal && onAborted) { + abortSignal.removeEventListener("abort", onAborted); + } + }; - const clearAbortSignalAndTimer = (): void => { - clearTimeout(timer); + onAborted = (): void => { + if (isDefined(timer)) { + clearTimeout(timer); + } + removeListeners(); + return rejectOnAbort(); + }; - if (clearAbortSignal) { - clearAbortSignal(); + if (abortSignal && abortSignal.aborted) { + return rejectOnAbort(); } - }; - - const abortOrTimeoutPromise = new Promise((_resolve, reject) => { - clearAbortSignal = checkAndRegisterWithAbortSignal(reject, args.abortSignal); timer = setTimeout(() => { - reject(new Error(args.timeoutMessage)); - }, args.timeoutMs); - }); - - try { - return await Promise.race([abortOrTimeoutPromise, args.actionFn()]); - } finally { - clearAbortSignalAndTimer(); - } -} + removeListeners(); + resolve(); + }, delayInMs); -/** - * Registers listener to the abort event on the abortSignal to call your abortFn and - * returns a function that will clear the same listener. - * - * If abort signal is already aborted, then throws an AbortError and returns a function that does nothing - * - * @returns A function that removes any of our attached event listeners on the abort signal or an empty function if - * the abortSignal was not defined. - * - * @internal - */ -function checkAndRegisterWithAbortSignal( - onAbortFn: (abortError: AbortError) => void, - abortSignal?: AbortSignalLike -): () => void { - if (abortSignal == null) { - return () => { - /** Nothing to do here, no abort signal */ - }; - } - - if (abortSignal.aborted) { - throw new AbortError(StandardAbortMessage); - } - - const onAbort = (): void => { - abortSignal.removeEventListener("abort", onAbort); - onAbortFn(new AbortError(StandardAbortMessage)); - }; - - abortSignal.addEventListener("abort", onAbort); - - return () => abortSignal.removeEventListener("abort", onAbort); + if (abortSignal) { + abortSignal.addEventListener("abort", onAborted); + } + }); } /** @@ -128,12 +97,11 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { throw err; } - return await waitForTimeoutOrAbortOrResolve({ - timeoutMs: delayInMs, - abortSignal: httpRequest.abortSignal, - actionFn: () => this.sendRequest(httpRequest.clone()), - timeoutMessage: `Unable to fulfill the request in ${delayInMs}ms when retried.` - }); + await delay(delayInMs, httpRequest.abortSignal, StandardAbortMessage); + if (httpRequest.abortSignal?.aborted) { + throw new AbortError(StandardAbortMessage); + } + return await this.sendRequest(httpRequest.clone()); } else { throw err; } From 4547c23ff76f583adda6f4f9c946a22bacb5d8cc Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 14:59:55 -0700 Subject: [PATCH 14/16] no need of generics --- .../app-configuration/src/policies/throttlingRetryPolicy.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts index bdeed6aecbcc..c53f9b599c4f 100644 --- a/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts +++ b/sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts @@ -34,11 +34,11 @@ const StandardAbortMessage = "The operation was aborted."; * @param abortErrorMsg - The abort error message associated with containing operation. * @returns - Resolved promise */ -export function delay( +export function delay( delayInMs: number, abortSignal?: AbortSignalLike, abortErrorMsg?: string -): Promise { +): Promise { return new Promise((resolve, reject) => { let timer: ReturnType | undefined = undefined; let onAborted: (() => void) | undefined = undefined; From 5aac533c8e7abee960f536db7d4fec7e20cd9e53 Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Wed, 16 Jun 2021 17:24:11 -0700 Subject: [PATCH 15/16] local en-us remove from links --- sdk/appconfiguration/app-configuration/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/appconfiguration/app-configuration/CHANGELOG.md b/sdk/appconfiguration/app-configuration/CHANGELOG.md index 9c9fe24b274c..b71383a58e81 100644 --- a/sdk/appconfiguration/app-configuration/CHANGELOG.md +++ b/sdk/appconfiguration/app-configuration/CHANGELOG.md @@ -12,7 +12,7 @@ - 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/en-us/azure/azure-app-configuration/rest-api-throttling) and [App Configuration | Requests Quota](https://docs.microsoft.com/en-us/azure/azure-app-configuration/faq#which-app-configuration-tier-should-i-use) + - 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) From 77b0d3d5d9f90498b2313ec65a2301d349b3f45f Mon Sep 17 00:00:00 2001 From: HarshaNalluru Date: Thu, 17 Jun 2021 11:10:46 -0700 Subject: [PATCH 16/16] check for AbortError --- .../test/internal/throttlingRetryPolicyTests.spec.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts b/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts index 8c271a4a79b9..d33ed58b9c6f 100644 --- a/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts +++ b/sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts @@ -228,6 +228,7 @@ describe("Should not retry forever - honors the abort signal passed", () => { const key = generateUuid(); const numberOfSettings = 200; const promises = []; + let errorWasThrown = false; try { for (let index = 0; index < numberOfSettings; index++) { promises.push( @@ -243,6 +244,10 @@ describe("Should not retry forever - honors the abort signal passed", () => { ); } await Promise.all(promises); - } catch (_) {} + } catch (error) { + errorWasThrown = true; + chai.assert.equal((error as any).name, "AbortError", "Unexpected error thrown"); + } + chai.assert.equal(errorWasThrown, true, "Error was not thrown"); }); });