Skip to content
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

Merged
18 commits merged into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/appconfiguration/app-configuration/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ export class AppConfigurationClient {
entity: keyValue,
...newOptions
});

return transformKeyValueResponse(originalResponse);
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
delay,
RestError
} from "@azure/core-http";
import { AbortError } from "@azure/abort-controller";

/**
* @internal
Expand All @@ -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(
Copy link
Member Author

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?

Copy link
Member

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.

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

Copy link
Member Author

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?

Copy link
Member

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.

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
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's almost redundant to call abortSignal here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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;
}
Expand Down
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 });
});
});
});