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.

Sign up for GitHub

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

Jump to bottom

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

Closed
HarshaNalluru opened this issue Jun 17, 2021 · 1 comment · Fixed by #15832
Closed

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

HarshaNalluru opened this issue Jun 17, 2021 · 1 comment · Fixed by #15832
Assignees
@HarshaNalluru
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Milestone
[2021] July

Comments

@HarshaNalluru
Copy link
Member

HarshaNalluru commented Jun 17, 2021 •
edited
Loading

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:

  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()));

TODO
@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 17, 2021
@HarshaNalluru HarshaNalluru mentioned this issue Jun 17, 2021
Merged
@HarshaNalluru HarshaNalluru self-assigned this Jun 17, 2021
@HarshaNalluru
Copy link
Member Author

HarshaNalluru commented Jun 17, 2021 •
edited
Loading

#15832

@HarshaNalluru HarshaNalluru added this to the [2021] July milestone Jun 17, 2021
@HarshaNalluru HarshaNalluru mentioned this issue Jun 17, 2021
Merged
@ramya-rao-a ramya-rao-a added Azure.Core Client This issue points to a problem in the data-plane of the library. labels Jun 17, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Jun 17, 2021
@ghost ghost closed this as completed in #15832 Jun 25, 2021
ghost pushed a commit that referenced this issue Jun 25, 2021
@HarshaNalluru
[core-http] Throttling retry policy fix in core-http (#15832)
bb9896d 
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
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@HarshaNalluru HarshaNalluru

Labels
Azure.Core Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Milestone
[2021] July
Development

Successfully merging a pull request may close this issue.

[core-http] Throttling retry policy fix in core-http Azure/azure-sdk-for-js
2 participants
@HarshaNalluru @ramya-rao-a