-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[App Config] Handle throttling - do not hang - should honor abort signal #15721
Changes from 4 commits
5955709
16a5238
f6270b7
ee6c764
bea4617
613407c
6579a13
93a2913
fe63f7b
1ca2bb6
b294f36
d5cfcfe
596f4cc
9becefe
dc204db
4547c23
5aac533
77b0d3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<boolean>, | ||
delayBetweenRetriesInMilliseconds: number = 1000, | ||
maxWaitTimeInMilliseconds: number = 10000 | ||
): Promise<boolean> { | ||
const maxTime = Date.now() + maxWaitTimeInMilliseconds; | ||
HarshaNalluru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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,15 +55,19 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { | |
} | ||
|
||
public async sendRequest(httpRequest: WebResource): Promise<HttpOperationResponse> { | ||
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); | ||
|
||
if (delayInMs == null) { | ||
throw err; | ||
} | ||
|
||
return delay(delayInMs).then((_: any) => this.sendRequest(httpRequest.clone())); | ||
await checkWithTimeout(() => httpRequest.abortSignal?.aborted === true, 1, delayInMs); | ||
if (httpRequest.abortSignal?.aborted) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's almost redundant to call abortSignal here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, kept it just in case. Should I remove it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
throw new AbortError("The operation was aborted."); | ||
} | ||
return await this.sendRequest(httpRequest.clone()); | ||
} else { | ||
throw err; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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", () => { | ||
HarshaNalluru marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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 }); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should move this to core-utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.