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

AppConfigurationClient does not retry when a request is throttled #6408

Closed
2 of 6 tasks
et13 opened this issue Dec 4, 2019 · 3 comments · Fixed by #6431
Closed
2 of 6 tasks

AppConfigurationClient does not retry when a request is throttled #6408

et13 opened this issue Dec 4, 2019 · 3 comments · Fixed by #6431
Assignees
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization.

Comments

@et13
Copy link
Member

et13 commented Dec 4, 2019

  • Package Name: @azure/app-configuration
  • Package Version: 1.0.0-preview.9
  • Operating system: Win10 Enterprise 1909
  • nodejs
    • version: 12.13.0
  • browser
    • name/version:
  • typescript
    • version: 3.7.2
  • Is the bug related to documentation in

Describe the bug
AppConfigurationClient does not retry when a request is throttled. AppConfigurationClient throws a RestError with the 429 response.

To Reproduce

  1. Add a valid connection string where indicated in the typescript below and run. The code will attempt to add 200 keys to the App Configuration instance in parallel, outputting any errors to the console.
import { AppConfigurationClient, SetConfigurationSettingParam } from '@azure/app-configuration';

async function main() {

    // Add valid connection string
    const connectionString = "<connectionstring>";
    const client = new AppConfigurationClient(connectionString);

    const promises = [];
    const count = 200;

    for (let i = 0; i < count; i++) {       
        promises.push(setSetting(client, {
            key: `Key${i}`
        }));
    }

    await Promise.all(promises);
}

async function setSetting(client: AppConfigurationClient, setting: SetConfigurationSettingParam) {
    try {
        await client.setConfigurationSetting(setting);
    } catch (error) {
        console.log(`${setting.key}: ${error.statusCode} ${error.stack}`);
    }
}

main();

Expected behavior
AppConfigurationClient performs a retry when throttled (after the retry-after-ms in the 429 response). No errors should be reported and all 200 keys are created.

Additional context
When throttled, the AppConfigurationClient throws a RestError that looks like:

Key176: 429 Error: {"type":"https://azconfig.io/errors/too-many-requests","title":"The amount of requests sent to the server have surpassed the assigned quota","policy":"TotalRequests","status":429}
    at new RestError (<path>\node_modules\@azure\core-http\dist\coreHttp.node.js:1716:28)
    at <path>\node_modules\@azure\core-http\dist\coreHttp.node.js:3416:37
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

Also, I checked the .NET SDK, and it does the retry, resulting in the exepcted behavior.

@ramya-rao-a ramya-rao-a added App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Dec 5, 2019
@triage-new-issues triage-new-issues bot removed the triage label Dec 5, 2019
richardpark-msft added a commit to richardpark-msft/azure-sdk-for-js that referenced this issue Dec 5, 2019
This PR adds in `retry-after-ms` and `x-ms-retry-after-ms` to the list
of headers we check alongside `Retry-After`, bringing it to parity with
what the C# SDK does.

Fixes Azure#6408
richardpark-msft added a commit that referenced this issue Dec 6, 2019
Adds in `retry-after-ms` and `x-ms-retry-after-ms` to the list
of headers we check alongside `Retry-After`, bringing it to parity with
what the C# SDK does.

Fixes #6408
@ghost
Copy link

ghost commented Dec 6, 2019

Thanks for working with Microsoft on GitHub! Tell us how you feel about your experience using the reactions on this comment.

@richardpark-msft
Copy link
Member

Thank you for this very detailed bug report. I was able to merge in a fix and we'll look to publish this next week.

@ramya-rao-a ramya-rao-a added this to the [2020] January milestone Dec 9, 2019
@richardpark-msft
Copy link
Member

richardpark-msft commented Dec 10, 2019

@et13 - a fix for this has been published.

https://www.npmjs.com/package/@azure/app-configuration/v/1.0.0-preview.10
(edited to fix link to the actual current version)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
App Configuration Azure.ApplicationModel.Configuration Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants