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

HarshaNalluru · 2021-06-17T23:04:10Z

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

Solution

Update delay such that it respects abort signal.
Similar to what I had to do for app-config at #15721

…to harshan/core-http/throttling-policy

deyaaeldeen · 2021-06-17T23:49:56Z

sdk/appconfiguration/app-configuration/package.json

@@ -91,6 +91,7 @@
    "@azure/core-http": "^1.2.0",
    "@azure/core-paging": "^1.1.1",
    "@azure/core-tracing": "1.0.0-preview.11",
+    "@azure/core-util": "^1.0.0-beta.1",


is appconfiguration in beta? if not, it should not take a dep on a beta.

App-config is beta, but we may soon GA in the next couple of months.
Why is core-util in beta anyways? Can't we make that GA instead?

Because we did not settle on the public surface for it yet.

appconfig is in beta but core-http and core-amqp are not IIRC and you added core-util as a dep there. My recommendation is to keep the local implementation of delay for now and have it moved to core-util in another PR.

I really don't want to keep 3 copies of delay (core-http, core-amqp and app-config).

@jeremymeng @chradek @richardpark-msft

I really don't want to keep 3 copies of delay (core-http, core-amqp and app-config).

Why cannot App Config use the one in core-http?

Since this PR's main objective is around fixing an issue with the retry policy, can we stick to just that in this PR?

@ramya-rao-a So, you're suggesting core-http and core-amqp would have delay? Can do that if weare fine with 2 copies.

(What's the point of core-util then?)

What's the point of core-util then

It is pending the on the conclusion on the discussion we had today on the subject of code sharing

We now have two copies of (advanced) delay.. core-amqp and core-http.

Logged #15930 for core-util discussion.

… harshan/core-http/throttling-policy

xirzec · 2021-06-18T21:15:21Z

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

@@ -12,7 +12,7 @@ import {
  Constants,
  RestError
 } from "@azure/core-http";


Now we just need to move to corev2 so we can fix #6484

sdk/core/core-http/test/policies/throttlingRetryPolicy.node.ts

chradek · 2021-06-24T15:45:52Z

sdk/core/core-http/review/core-http.api.md

@@ -181,7 +181,7 @@ export class DefaultHttpClient extends FetchHttpClient {
    }

 // @public
-export function delay<T>(t: number, value?: T): Promise<T | void>;
+export function delay<T>(delayInMs: number, abortSignal?: AbortSignalLike, abortErrorMsg?: string, value?: T): Promise<T | void>;


value?: T should be the 2nd argument, otherwise this would be a breaking change.

Good point.

I'll switch to that.
What about the core-amqp counterpart? How to get the copy of this method to stay consistent? Does that mean we can only bring consistency if we upgrade to a major version?

@HarshaNalluru I think there is no need to care about their consistency, we can just ignore them once core-util goes GA. Also, core-http is going to be phased out fairly soon anyway.

deyaaeldeen

Thanks for the fix!

xirzec

Nice work! A few small comments/suggestions

sdk/core/core-http/review/core-http.api.md

sdk/core/core-http/src/policies/rpRegistrationPolicy.ts

xirzec · 2021-06-24T20:58:40Z

sdk/core/core-http/src/util/delay.ts

+      }
+    };
+
+    onAborted = (): void => {


why is this assigned? I don't see it getting assigned back to undefined can we just declare the method in scope?

I just copied this from core-amqp.

Can be done, will do.

Linter complains about this. Reverting.

sdk/core/core-http/src/util/delay.ts

sdk/core/core-http/src/util/typeguards.ts

ghost · 2021-06-24T23:11:41Z

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.

Microsoft.ApiManagement : Release api-version 2021-04-01-preview (Azure#15832) * Adds base for updating Microsoft.ApiManagement from version preview/2021-01-01-preview to version 2021-04-01-preview * Updates readme * Updates API version in new specs and examples * Added support for GraphQL API type (Azure#14851) * Added support for GraphQL API type * Added missing enum value for GraphQL APIs. * add schema definition fix from 2021-01-01-preview (Azure#14883) Co-authored-by: Alan Feng <alanfeng@microsoft.com> * Vanguard: Get Outbound Network Dependency (Azure#14900) * outbound dependency * fix resource * Update backup/restore API for managed identities (Azure#14973) * Update backup/restore API for system-assigned and user-assigned identities * Fix formatting issue * Rename user-assigned-msi-client-id with client-id * SystemData implemenation on API Management Control Plane (Azure#14899) * system-data * fix spec * remove unreferenced file * APIM - Network Watcher Connectivity Check integration specs (Azure#15056) * Connectivity Check API Specs * Fix * Fix errors * style fixs * Fix 202 code * HTTPConnect request parameters * Fix errors Co-authored-by: Nicolás Barrera <t-nbarrera@microsoft.com> * Parameter examples added (Azure#14836) * platformversion (Azure#15114) * fix update service (Azure#15478) * HttpConnect example (Azure#15493) * HttpConnect example * Prettifier fix Co-authored-by: Nicolás Barrera <t-nbarrera@microsoft.com> * Added properties of difference API spec format for "SchemaDocumentProperties" (Azure#15703) * add schema definition fix from 2021-01-01-preview * added properties for SchemaDocumentProperties * updated the fix * updated fix * [APIM]Add private endpoint connection APIs (Azure#15115) * Add private endpoint connection apis * add readme * quick fixes * fix to PE contract * Small fixes * small fixes * small fixes * Small fixes * small fixes * small fix * small fixes * small fixes * small fixes * Update readme.md * fix error * small fix * fix format * address comments * small fixes * Change to lower case * small fixes Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com> * Adds base for updating Microsoft.ApiManagement from version preview/2021-01-01-preview to version 2021-04-01-preview * Updates readme * Updates API version in new specs and examples * Added support for GraphQL API type (Azure#14851) * Added support for GraphQL API type * Added missing enum value for GraphQL APIs. * add schema definition fix from 2021-01-01-preview (Azure#14883) Co-authored-by: Alan Feng <alanfeng@microsoft.com> * Vanguard: Get Outbound Network Dependency (Azure#14900) * outbound dependency * fix resource * Update backup/restore API for managed identities (Azure#14973) * Update backup/restore API for system-assigned and user-assigned identities * Fix formatting issue * Rename user-assigned-msi-client-id with client-id * SystemData implemenation on API Management Control Plane (Azure#14899) * system-data * fix spec * remove unreferenced file * APIM - Network Watcher Connectivity Check integration specs (Azure#15056) * Connectivity Check API Specs * Fix * Fix errors * style fixs * Fix 202 code * HTTPConnect request parameters * Fix errors Co-authored-by: Nicolás Barrera <t-nbarrera@microsoft.com> * Parameter examples added (Azure#14836) * platformversion (Azure#15114) * fix update service (Azure#15478) * HttpConnect example (Azure#15493) * HttpConnect example * Prettifier fix Co-authored-by: Nicolás Barrera <t-nbarrera@microsoft.com> * Added properties of difference API spec format for "SchemaDocumentProperties" (Azure#15703) * add schema definition fix from 2021-01-01-preview * added properties for SchemaDocumentProperties * updated the fix * updated fix * [APIM]Add private endpoint connection APIs (Azure#15115) * Add private endpoint connection apis * add readme * quick fixes * fix to PE contract * Small fixes * small fixes * small fixes * Small fixes * small fixes * small fix * small fixes * small fixes * small fixes * Update readme.md * fix error * small fix * fix format * address comments * small fixes * Change to lower case * small fixes Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com> * fix tenant sync contract * fix linter issues * examples for managed identity backup * prettier * Fix Apis-Get and updated comments for api-version 2021-04-01-preview (#3) * add schema definition fix from 2021-01-01-preview * added fix * small fix (#6) * 'examples' added to RepresentationContract (#7) Co-authored-by: Alexander Zaslonov <zaslonov.alexander@gmail.com> Co-authored-by: DreamlessA <DreamlessA@users.noreply.github.com> Co-authored-by: Alan Feng <alanfeng@microsoft.com> Co-authored-by: Jatin Sanghvi <20547963+JatinSanghvi@users.noreply.github.com> Co-authored-by: Nicolás Barrera <nicolasbarrera@outlook.com> Co-authored-by: Nicolás Barrera <t-nbarrera@microsoft.com> Co-authored-by: VitaliyKurokhtin <vitaliik@microsoft.com> Co-authored-by: RupengLiu <rliu1211@terpmail.umd.edu> Co-authored-by: msyyc <70930885+msyyc@users.noreply.github.com> Co-authored-by: RupengLiu <rupliu@microsoft.com> Co-authored-by: VitaliyKurokhtin <Vitaliy.Kurokhtin@gmail.com>

…3019) ### Packages impacted by this PR core-util ### Issues associated with this PR #15930 ### Describe the problem that is addressed by this PR - Update "delay" in core-util to support abort message - Unduplicate "delay" function in core-http and core-amqp ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? - Choose the implementation with "options" parameters to keep the flexibility in case we want to add extra functionality in the future ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ #15832 ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [x ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary)

HarshaNalluru added 7 commits June 16, 2021 18:54

throttling policy fix in core-http

0616843

remove comment

10a3816

typo

8bf61fe

import core-amqp delay from core-util

d876b54

import core-util

db74514

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

01bd7c4

…to harshan/core-http/throttling-policy

import { delay } from "@azure/core-util";

a0307af

ghost added Azure.Core App Configuration Azure.ApplicationModel.Configuration labels Jun 17, 2021

HarshaNalluru changed the title ~~Harshan/core http/throttling policy~~ [core-http] Throttling retry policy fix in core-http Jun 17, 2021

This was referenced Jun 17, 2021

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

Closed

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

Closed

HarshaNalluru added 3 commits June 17, 2021 23:15

changelog

78814aa

throw abort error check

ff9cb2a

revert core-rest-pipeline

d003ea8

HarshaNalluru marked this pull request as ready for review June 17, 2021 23:47

HarshaNalluru requested review from bterlson, chradek, deyaaeldeen, jeremymeng, richardpark-msft and xirzec as code owners June 17, 2021 23:47

deyaaeldeen reviewed Jun 17, 2021

View reviewed changes

HarshaNalluru added 2 commits June 18, 2021 20:41

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

15f9428

… harshan/core-http/throttling-policy

import { delay } from "@azure/core-util";

c774368

xirzec reviewed Jun 18, 2021

View reviewed changes

HarshaNalluru added 4 commits June 18, 2021 21:15

fix linter errors

daedfbb

fix build failures

0dd2974

export { delay } from "@azure/core-util";

6c4ed84

API Report

0ad0a26

HarshaNalluru requested review from xirzec, ramya-rao-a and deyaaeldeen June 23, 2021 21:05

jeremymeng reviewed Jun 23, 2021

View reviewed changes

sdk/core/core-http/test/policies/throttlingRetryPolicy.node.ts Outdated Show resolved Hide resolved

get rid of circular dependencies

8977a42

chradek reviewed Jun 24, 2021

View reviewed changes

value should be the second argument

3029418

deyaaeldeen approved these changes Jun 24, 2021

View reviewed changes

xirzec approved these changes Jun 24, 2021

View reviewed changes

HarshaNalluru added 6 commits June 24, 2021 22:07

update delay to have options bag and additional feedback

75ad58e

make async functions and typeguard feedback

6ede956

update app-config and API Report

37018a1

HttpMockFacade

1a6555b

should honor the abort signal passed

58b5855

remove nock

a3a0a04

HarshaNalluru requested review from jeremymeng, xirzec, chradek and deyaaeldeen June 24, 2021 22:38

jeremymeng approved these changes Jun 24, 2021

View reviewed changes

satisfy linter

9b7f233

HarshaNalluru added the auto-merge label Jun 24, 2021

HarshaNalluru mentioned this pull request Jun 25, 2021

[Identity] Identity ManagedIdentityCredential unit test flaky #15978

Closed

ghost merged commit bb9896d into main Jun 25, 2021

ghost deleted the harshan/core-http/throttling-policy branch June 25, 2021 01:49

HarshaNalluru mentioned this pull request Jun 30, 2021

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

Closed

minhanh-phan mentioned this pull request Aug 26, 2022

[core-util] Update "delay" method in core-util with abort options #23019

Merged

2 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HarshaNalluru commented Jun 17, 2021 •

edited

Loading

deyaaeldeen Jun 17, 2021

HarshaNalluru Jun 17, 2021

deyaaeldeen Jun 18, 2021

HarshaNalluru Jun 18, 2021

HarshaNalluru Jun 18, 2021

ramya-rao-a Jun 23, 2021

HarshaNalluru Jun 23, 2021

HarshaNalluru Jun 23, 2021

ramya-rao-a Jun 23, 2021

HarshaNalluru Jun 23, 2021

xirzec Jun 18, 2021

chradek Jun 24, 2021

HarshaNalluru Jun 24, 2021 •

edited

Loading

deyaaeldeen Jun 24, 2021

deyaaeldeen left a comment

xirzec left a comment

xirzec Jun 24, 2021

HarshaNalluru Jun 24, 2021

HarshaNalluru Jun 24, 2021

ghost commented Jun 24, 2021

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

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

Conversation

HarshaNalluru commented Jun 17, 2021 • edited Loading

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deyaaeldeen left a comment

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jun 24, 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 17, 2021 •

edited

Loading

HarshaNalluru Jun 24, 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.