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

HarshaNalluru · 2021-06-12T00:17:31Z

Description

Issue

Changelog

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 allows the user-provided abortSignal to be taken into account to abort the requests sooner.
- More resources - App Configuration | Throttling and App Configuration | Requests Quota

HarshaNalluru · 2021-06-12T00:18:14Z

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

+ * 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(


Maybe I should move this to core-utils?

The entire throttling policy really should end up in the core-http (or similar) library.

The only real difference between this one and the 'official' is that our AppConfig policy reacts based on a thrown exception. The other one assumes that it'll just be part of the response with no thrown error - perhaps that never actually worked.

azure-sdk-for-js/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts

Line 48 in c4f51be

return this._nextPolicy.sendRequest(httpRequest.clone()).then((response) => {

Might not be worth it if app-config is the only service that has retry-after-ms header with the error.

If we see at least one other service that does this, we can move this policy to the core?

I'm wondering both ways - do we think the existing policy is ever actually used?

I don't think it's our code that's doing the throw so I'm wondering if the core-http policy works at all. Perhaps this is a question we can answer by going through some swaggers at some point.

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

richardpark-msft

Looking good. Left some more comments.

sdk/appconfiguration/app-configuration/test/public/throttling.spec.ts

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

…to harshan/app-config/issue/15576

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

richardpark-msft · 2021-06-17T05:44:42Z

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

-          timeoutMessage: `Unable to fulfill the request in ${delayInMs}ms when retried.`
-        });
+        await delay(delayInMs, httpRequest.abortSignal, StandardAbortMessage);
+        if (httpRequest.abortSignal?.aborted) {


It's almost redundant to call abortSignal here.

Yeah, kept it just in case. Should I remove it?
My point is.. the next sendRequest call should never be invoked if the request has been aborted before, and this felt to be the way.

No, keep it. It's correct, it just looks redundant because technically 'delay' does the same check. But, being 'async', it's possible for abortSignal.aborted to finally take affect and have it only be available afterwards.

(kind of like double-checked locking)

richardpark-msft

Looks really great. Ping me when you've undrafted it.

HarshaNalluru · 2021-06-17T17:37:39Z

Looks really great. Ping me when you've undrafted it.

Can you reload? it isn't a draft. :)

richardpark-msft

The test needs to be tightened up a bit.

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

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

richardpark-msft

Looks great!

ghost · 2021-06-17T21:53:07Z

Hello @HarshaNalluru!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…nal (Azure#15721) ## Description ### Issue - Azure#15576 - Azure/AppConfiguration#503 ### Changelog - 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. - [Azure#15721](Azure#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) Fixes Azure#15576

Fixes #15796 ## Problem The throttlingRetryPolicy in core-http has the potential to retry for an extended period if the service continues returning "retry after" headers on subsequent calls. Here's the snippet of code that handles the "retry after" retries: ```typescript public async sendRequest(httpRequest: WebResource): Promise<HttpOperationResponse> { return this._nextPolicy.sendRequest(httpRequest.clone()).catch((err) => { // other code elided.... return delay(delayInMs).then((_: any) => this.sendRequest(httpRequest.clone())); ``` ## Solution Update delay such that it respects abort signal. Similar to what I had to do for app-config at #15721

[Hub Generated] Review request for Microsoft.ContainerInstance to add version stable/2021-07-01 (Azure#15721) * Adds base for updating Microsoft.ContainerInstance from version stable/2021-03-01 to version 2021-07-01 * Updates readme * Updates API version in new specs and examples * Adding subnet IDs * Adding fixes from pending 03-01 update * Changing definition name to match convention * Adding network dependencies API * Adding MSI+ACR properties * Removing network profile * Updating example * Fixing JSON error * Removing network profile reference * Adding Integer format where missing * Removing comma * Fixing example * Fixing example * Ran Prettier to resolve check failure * Running prettier again for format

HarshaNalluru added 4 commits June 11, 2021 17:14

Add abort-controller in dependencies

5955709

src

16a5238

Update ThrottlingRetryPolicy.sendRequest

f6270b7

test for throttling policy

ee6c764

ghost added the App Configuration Azure.ApplicationModel.Configuration label Jun 12, 2021

HarshaNalluru commented Jun 12, 2021

View reviewed changes

delayBetweenRechecksInMs

bea4617

HarshaNalluru requested review from richardpark-msft, chradek and ramya-rao-a and removed request for chradek June 12, 2021 00:24

richardpark-msft reviewed Jun 14, 2021

View reviewed changes

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts Outdated Show resolved Hide resolved

HarshaNalluru added 2 commits June 15, 2021 14:37

throttling retry policy updates

613407c

test

6579a13

richardpark-msft reviewed Jun 15, 2021

View reviewed changes

HarshaNalluru added 7 commits June 16, 2021 09:54

address Richard's feedback

93a2913

delete the live test

fe63f7b

Should not retry forever - honors the abort signal passed test

1ca2bb6

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

b294f36

…to harshan/app-config/issue/15576

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-js in…

d5cfcfe

…to harshan/app-config/issue/15576

changelog

596f4cc

changelog update

9becefe

HarshaNalluru marked this pull request as ready for review June 16, 2021 21:28

HarshaNalluru marked this pull request as draft June 16, 2021 21:34

HarshaNalluru added 2 commits June 16, 2021 14:59

delay method with abort signal

dc204db

no need of generics

4547c23

richardpark-msft reviewed Jun 16, 2021

View reviewed changes

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts Outdated Show resolved Hide resolved

HarshaNalluru marked this pull request as ready for review June 16, 2021 22:20

local en-us remove from links

5aac533

HarshaNalluru requested a review from richardpark-msft June 17, 2021 00:24

richardpark-msft reviewed Jun 17, 2021

View reviewed changes

richardpark-msft requested changes Jun 17, 2021

View reviewed changes

sdk/appconfiguration/app-configuration/test/internal/throttlingRetryPolicyTests.spec.ts Outdated Show resolved Hide resolved

sdk/appconfiguration/app-configuration/src/policies/throttlingRetryPolicy.ts Show resolved Hide resolved

HarshaNalluru mentioned this pull request Jun 17, 2021

[core-http] Throttling retry policy should account for abortSignal #15796

Closed

check for AbortError

77b0d3d

HarshaNalluru requested a review from richardpark-msft June 17, 2021 18:11

richardpark-msft approved these changes Jun 17, 2021

View reviewed changes

HarshaNalluru added the auto-merge label Jun 17, 2021

ghost merged commit a955ff2 into Azure:master Jun 17, 2021

HarshaNalluru deleted the harshan/app-config/issue/15576 branch June 17, 2021 21:54

HarshaNalluru mentioned this pull request Jun 17, 2021

[core-http] Throttling retry policy fix in core-http #15832

Merged

HarshaNalluru mentioned this pull request Jun 30, 2021

[core-rest-pipeline] Throttling retry policy should account for abortSignal #16118

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HarshaNalluru commented Jun 12, 2021 •

edited

Loading

HarshaNalluru Jun 12, 2021

richardpark-msft Jun 14, 2021

HarshaNalluru Jun 14, 2021

richardpark-msft Jun 17, 2021

richardpark-msft left a comment

richardpark-msft Jun 17, 2021

HarshaNalluru Jun 17, 2021

richardpark-msft Jun 17, 2021

richardpark-msft left a comment

HarshaNalluru commented Jun 17, 2021

richardpark-msft left a comment

richardpark-msft left a comment

ghost commented Jun 17, 2021

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

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

Conversation

HarshaNalluru commented Jun 12, 2021 • edited Loading

Description

Issue

Changelog

HarshaNalluru Jun 12, 2021

Choose a reason for hiding this comment

richardpark-msft Jun 14, 2021

Choose a reason for hiding this comment

HarshaNalluru Jun 14, 2021

Choose a reason for hiding this comment

richardpark-msft Jun 17, 2021

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

richardpark-msft Jun 17, 2021

Choose a reason for hiding this comment

HarshaNalluru Jun 17, 2021

Choose a reason for hiding this comment

richardpark-msft Jun 17, 2021

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

HarshaNalluru commented Jun 17, 2021

richardpark-msft left a comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

ghost commented Jun 17, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

HarshaNalluru commented Jun 12, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.