From 7885d615acf1d4be7159fed5ccd1fe0beb2f240b Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Tue, 22 Jun 2021 23:52:19 +0000 Subject: [PATCH 01/13] [core] Retry on 503 using the Retry-After header --- sdk/core/core-http/CHANGELOG.md | 2 + .../src/policies/throttlingRetryPolicy.ts | 5 +- sdk/core/core-http/src/util/constants.ts | 3 +- .../policies/throttlingRetryPolicyTests.ts | 34 ++++++-- sdk/core/core-rest-pipeline/CHANGELOG.md | 4 + .../src/policies/throttlingRetryPolicy.ts | 2 +- .../test/throttlingRetryPolicy.spec.ts | 84 ++++++++++++++++++- 7 files changed, 123 insertions(+), 11 deletions(-) diff --git a/sdk/core/core-http/CHANGELOG.md b/sdk/core/core-http/CHANGELOG.md index b0602c0a0a9a..05f62976c884 100644 --- a/sdk/core/core-http/CHANGELOG.md +++ b/sdk/core/core-http/CHANGELOG.md @@ -4,6 +4,8 @@ ### Features Added +- Added support for the `Retry-After` header on responses with status code 503, Service Unavailable. + ### Breaking Changes ### Key Bugs Fixed diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index 6c02e59220f1..ab59b1ce66e6 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -46,7 +46,10 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { public async sendRequest(httpRequest: WebResourceLike): Promise { return this._nextPolicy.sendRequest(httpRequest.clone()).then((response) => { - if (response.status !== StatusCodes.TooManyRequests) { + if ( + response.status !== StatusCodes.TooManyRequests && + response.status !== StatusCodes.ServiceUnavailable + ) { return response; } else { return this._handleResponse(httpRequest, response); diff --git a/sdk/core/core-http/src/util/constants.ts b/sdk/core/core-http/src/util/constants.ts index a870e36a2324..2431536f9db6 100644 --- a/sdk/core/core-http/src/util/constants.ts +++ b/sdk/core/core-http/src/util/constants.ts @@ -52,7 +52,8 @@ export const Constants = { }, StatusCodes: { - TooManyRequests: 429 + TooManyRequests: 429, + ServiceUnavailable: 503 } }, diff --git a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts index 5d663dfdd1d9..9c6d6055aeab 100644 --- a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts +++ b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts @@ -10,7 +10,7 @@ import { HttpHeaders, RequestPolicyOptions } from "../../src/coreHttp"; describe("ThrottlingRetryPolicy", () => { class PassThroughPolicy { - constructor(private _response: HttpOperationResponse) {} + constructor(private _response: HttpOperationResponse) { } public sendRequest(request: WebResource): Promise { const response = { ...this._response, @@ -71,7 +71,7 @@ describe("ThrottlingRetryPolicy", () => { assert.deepEqual(response.request, request); }); - it("should do nothing when status code is not 429", async () => { + it("should do nothing when status code is not 429 nor 503", async () => { const request = new WebResource(); const mockResponse = { status: 400, @@ -112,21 +112,43 @@ describe("ThrottlingRetryPolicy", () => { delete (response.request as any).requestId; assert.deepEqual(response, mockResponse); }); + + it("should pass the response to the handler if the status code equals 503", async () => { + const request = new WebResource(); + const mockResponse = { + status: 503, + headers: new HttpHeaders({ + "Retry-After": "100" + }), + request: request + }; + const policy = createDefaultThrottlingRetryPolicy(mockResponse, (_, response) => { + delete (response.request as any).requestId; + delete (mockResponse.request as any).requestId; + assert.deepEqual(response, mockResponse); + return Promise.resolve(response); + }); + + const response = await policy.sendRequest(request); + delete (request as any).requestId; + delete (response.request as any).requestId; + assert.deepEqual(response, mockResponse); + }); }); describe("parseRetryAfterHeader", () => { - it("should return undefined for ill-formed header", function() { + it("should return undefined for ill-formed header", function () { const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader("foobar"); assert.equal(retryAfter, undefined); }); - it("should return sleep interval value in milliseconds if parameter is a number", function(done) { + it("should return sleep interval value in milliseconds if parameter is a number", function (done) { const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader("1"); assert.equal(retryAfter, 1000); done(); }); - it("should return sleep interval value in milliseconds for full date format", function(done) { + it("should return sleep interval value in milliseconds for full date format", function (done) { const clock = sinon.useFakeTimers(new Date("Fri, 31 Dec 1999 23:00:00 GMT").getTime()); const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader( "Fri, 31 Dec 1999 23:02:00 GMT" @@ -138,7 +160,7 @@ describe("ThrottlingRetryPolicy", () => { done(); }); - it("should return sleep interval value in milliseconds for shorter date format", function(done) { + it("should return sleep interval value in milliseconds for shorter date format", function (done) { const clock = sinon.useFakeTimers(new Date("Fri, 31 Dec 1999 23:00:00 GMT").getTime()); const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader("31 Dec 1999 23:03:00 GMT"); diff --git a/sdk/core/core-rest-pipeline/CHANGELOG.md b/sdk/core/core-rest-pipeline/CHANGELOG.md index b9011a20ad39..d8a01a2abef3 100644 --- a/sdk/core/core-rest-pipeline/CHANGELOG.md +++ b/sdk/core/core-rest-pipeline/CHANGELOG.md @@ -6,6 +6,10 @@ - Fixed an issue where `proxySettings` does not work when there is username but no password [Issue 15720](https://github.com/Azure/azure-sdk-for-js/issues/15720) +### Features Added + +- Added support for the `Retry-After` header on responses with status code 503, Service Unavailable. + ## 1.1.0-beta.3 (2021-06-03) - Merged `bearerTokenChallengeAuthenticationPolicy` into `bearerTokenAuthenticationPolicy`. This will keep the functionality of `bearerTokenAuthenticationPolicy`, but also adds the `challengeCallbacks` feature. diff --git a/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts index 71e06e48978c..413ef12660c4 100644 --- a/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts @@ -23,7 +23,7 @@ export function throttlingRetryPolicy(): PipelinePolicy { name: throttlingRetryPolicyName, async sendRequest(request: PipelineRequest, next: SendRequest): Promise { const response = await next(request); - if (response.status !== 429) { + if (response.status !== 429 && response.status !== 503) { return response; } diff --git a/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts b/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts index d0586a3cfcf5..0ac2d493ea8a 100644 --- a/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts +++ b/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts @@ -16,7 +16,7 @@ describe("throttlingRetryPolicy", function() { sinon.restore(); }); - it("It should retry after a given number of seconds", async () => { + it("It should retry after a given number of seconds on a response with status code 429", async () => { const request = createPipelineRequest({ url: "https://bing.com" }); @@ -54,7 +54,7 @@ describe("throttlingRetryPolicy", function() { clock.restore(); }); - it("It should retry after a given date occurs", async () => { + it("It should retry after a given date occurs on a response with status code 429", async () => { const request = createPipelineRequest({ url: "https://bing.com" }); @@ -95,4 +95,84 @@ describe("throttlingRetryPolicy", function() { assert.strictEqual(result, successResponse); clock.restore(); }); + + it("It should retry after a given number of seconds on a response with status code 503", async () => { + const request = createPipelineRequest({ + url: "https://bing.com" + }); + const retryResponse: PipelineResponse = { + headers: createHttpHeaders({ + "Retry-After": "10" + }), + request, + status: 503 + }; + const successResponse: PipelineResponse = { + headers: createHttpHeaders(), + request, + status: 200 + }; + + const policy = throttlingRetryPolicy(); + const next = sinon.stub, ReturnType>(); + next.onFirstCall().resolves(retryResponse); + next.onSecondCall().resolves(successResponse); + + const clock = sinon.useFakeTimers(); + + const promise = policy.sendRequest(request, next); + assert.isTrue(next.calledOnce); + + // allow the delay to occur + const time = await clock.nextAsync(); + assert.strictEqual(time, 10 * 1000); + assert.isTrue(next.calledTwice); + + const result = await promise; + + assert.strictEqual(result, successResponse); + clock.restore(); + }); + + it("It should retry after a given date occurs on a response with status code 503", async () => { + const request = createPipelineRequest({ + url: "https://bing.com" + }); + const retryResponse: PipelineResponse = { + headers: createHttpHeaders({ + "Retry-After": "Wed, 21 Oct 2015 07:28:00 GMT" + }), + request, + status: 503 + }; + const successResponse: PipelineResponse = { + headers: createHttpHeaders(), + request, + status: 200 + }; + + const policy = throttlingRetryPolicy(); + const next = sinon.stub, ReturnType>(); + next.onFirstCall().resolves(retryResponse); + next.onSecondCall().resolves(successResponse); + + const clock = sinon.useFakeTimers(new Date("Wed, 21 Oct 2015 07:20:00 GMT")); + + const promise = policy.sendRequest(request, next); + assert.isTrue(next.calledOnce); + + // allow the delay to occur + const time = await clock.nextAsync(); + assert.strictEqual( + time, + new Date("Wed, 21 Oct 2015 07:28:00 GMT").getTime(), + "It should now be the time from the header." + ); + assert.isTrue(next.calledTwice); + + const result = await promise; + + assert.strictEqual(result, successResponse); + clock.restore(); + }); }); From 8ab4e314c6d83ff5bbae5ae564ea985ed0670335 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 23 Jun 2021 00:18:31 +0000 Subject: [PATCH 02/13] formatting --- .../test/policies/throttlingRetryPolicyTests.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts index 9c6d6055aeab..1db017487aea 100644 --- a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts +++ b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts @@ -10,7 +10,7 @@ import { HttpHeaders, RequestPolicyOptions } from "../../src/coreHttp"; describe("ThrottlingRetryPolicy", () => { class PassThroughPolicy { - constructor(private _response: HttpOperationResponse) { } + constructor(private _response: HttpOperationResponse) {} public sendRequest(request: WebResource): Promise { const response = { ...this._response, @@ -137,18 +137,18 @@ describe("ThrottlingRetryPolicy", () => { }); describe("parseRetryAfterHeader", () => { - it("should return undefined for ill-formed header", function () { + it("should return undefined for ill-formed header", function() { const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader("foobar"); assert.equal(retryAfter, undefined); }); - it("should return sleep interval value in milliseconds if parameter is a number", function (done) { + it("should return sleep interval value in milliseconds if parameter is a number", function(done) { const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader("1"); assert.equal(retryAfter, 1000); done(); }); - it("should return sleep interval value in milliseconds for full date format", function (done) { + it("should return sleep interval value in milliseconds for full date format", function(done) { const clock = sinon.useFakeTimers(new Date("Fri, 31 Dec 1999 23:00:00 GMT").getTime()); const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader( "Fri, 31 Dec 1999 23:02:00 GMT" @@ -160,7 +160,7 @@ describe("ThrottlingRetryPolicy", () => { done(); }); - it("should return sleep interval value in milliseconds for shorter date format", function (done) { + it("should return sleep interval value in milliseconds for shorter date format", function(done) { const clock = sinon.useFakeTimers(new Date("Fri, 31 Dec 1999 23:00:00 GMT").getTime()); const retryAfter = ThrottlingRetryPolicy.parseRetryAfterHeader("31 Dec 1999 23:03:00 GMT"); From 13a7ff9334fd62bcc3ad88660e075d99e820449c Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 23 Jun 2021 03:17:36 +0000 Subject: [PATCH 03/13] core-http API review file --- sdk/core/core-http/review/core-http.api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/core/core-http/review/core-http.api.md b/sdk/core/core-http/review/core-http.api.md index 830b40000f54..71d2d1bfba00 100644 --- a/sdk/core/core-http/review/core-http.api.md +++ b/sdk/core/core-http/review/core-http.api.md @@ -145,6 +145,7 @@ export const Constants: { }; StatusCodes: { TooManyRequests: number; + ServiceUnavailable: number; }; }; HeaderConstants: { From 0cc46d255595818da243bd1971716476856bd8f2 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 23 Jun 2021 19:46:10 +0000 Subject: [PATCH 04/13] WIP --- sdk/core/core-http/CHANGELOG.md | 1 + .../src/policies/exponentialRetryPolicy.ts | 10 +++++ .../src/policies/throttlingRetryPolicy.ts | 10 ++++- .../src/util/throttlingRetryStrategy.ts | 4 ++ sdk/core/core-rest-pipeline/CHANGELOG.md | 1 + .../src/policies/exponentialRetryPolicy.ts | 9 ++++- .../managedIdentityCredential/imdsMsi.ts | 1 - .../node/managedIdentityCredential.spec.ts | 40 +++++++++++++++++++ 8 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 sdk/core/core-http/src/util/throttlingRetryStrategy.ts diff --git a/sdk/core/core-http/CHANGELOG.md b/sdk/core/core-http/CHANGELOG.md index 05f62976c884..6617667d7e40 100644 --- a/sdk/core/core-http/CHANGELOG.md +++ b/sdk/core/core-http/CHANGELOG.md @@ -5,6 +5,7 @@ ### Features Added - Added support for the `Retry-After` header on responses with status code 503, Service Unavailable. +- Added support for multiple retries on the `ThrottlingRetryPolicy` (up to 3 by default). ### Breaking Changes diff --git a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts index 89fbfb86c836..b0238116280c 100644 --- a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts @@ -22,6 +22,7 @@ import { } from "../util/exponentialBackoffStrategy"; import { RestError } from "../restError"; import { logger } from "../log"; +import { Constants } from "../util/constants"; export function exponentialRetryPolicy( retryCount?: number, @@ -139,6 +140,15 @@ async function retry( ): Promise { function shouldPolicyRetry(responseParam?: HttpOperationResponse): boolean { const statusCode = responseParam?.status; + if ( + statusCode && + statusCode === 503 && + response && + response.headers.get(Constants.HeaderConstants.RETRY_AFTER) + ) { + return false; + } + if ( statusCode === undefined || (statusCode < 500 && statusCode !== 408) || diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index ab59b1ce66e6..4a79f5564cfb 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -11,6 +11,7 @@ import { WebResourceLike } from "../webResource"; import { HttpOperationResponse } from "../httpOperationResponse"; import { Constants } from "../util/constants"; import { delay } from "../util/utils"; +import { DEFAULT_CLIENT_MAX_RETRY_COUNT } from "../util/throttlingRetryStrategy"; type ResponseHandler = ( httpRequest: WebResourceLike, @@ -34,6 +35,7 @@ export function throttlingRetryPolicy(): RequestPolicyFactory { */ export class ThrottlingRetryPolicy extends BaseRequestPolicy { private _handleResponse: ResponseHandler; + private numberOfRetries = 0; constructor( nextPolicy: RequestPolicy, @@ -70,7 +72,13 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { retryAfterHeader ); if (delayInMs) { - return delay(delayInMs).then((_: any) => this._nextPolicy.sendRequest(httpRequest)); + this.numberOfRetries += 1; + await delay(delayInMs); + if (this.numberOfRetries < DEFAULT_CLIENT_MAX_RETRY_COUNT) { + return this.sendRequest(httpRequest); + } else { + return this._nextPolicy.sendRequest(httpRequest); + } } } diff --git a/sdk/core/core-http/src/util/throttlingRetryStrategy.ts b/sdk/core/core-http/src/util/throttlingRetryStrategy.ts new file mode 100644 index 000000000000..6baae238f608 --- /dev/null +++ b/sdk/core/core-http/src/util/throttlingRetryStrategy.ts @@ -0,0 +1,4 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3; diff --git a/sdk/core/core-rest-pipeline/CHANGELOG.md b/sdk/core/core-rest-pipeline/CHANGELOG.md index d8a01a2abef3..c390807f2a90 100644 --- a/sdk/core/core-rest-pipeline/CHANGELOG.md +++ b/sdk/core/core-rest-pipeline/CHANGELOG.md @@ -9,6 +9,7 @@ ### Features Added - Added support for the `Retry-After` header on responses with status code 503, Service Unavailable. +- The `ExponentialRetryPolicy` will now ignore `503` responses if they have the `Retry-After` header. ## 1.1.0-beta.3 (2021-06-03) diff --git a/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts index 23784c619fd8..c138ee2cfb57 100644 --- a/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts @@ -70,7 +70,12 @@ export function exponentialRetryPolicy( * @param retryData - The retry data. * @returns True if the operation qualifies for a retry; false otherwise. */ - function shouldRetry(statusCode: number | undefined, retryData: RetryData): boolean { + function shouldRetry(response: PipelineResponse | undefined, retryData: RetryData): boolean { + const statusCode = response?.status; + if (statusCode && statusCode === 503 && response && response.headers.get("Retry-After")) { + return false; + } + if ( statusCode === undefined || (statusCode < 500 && statusCode !== 408) || @@ -126,7 +131,7 @@ export function exponentialRetryPolicy( ): Promise { retryData = updateRetryData(retryData, requestError); const isAborted = request.abortSignal?.aborted; - if (!isAborted && shouldRetry(response?.status, retryData)) { + if (!isAborted && shouldRetry(response, retryData)) { logger.info(`Retrying request in ${retryData.retryInterval}`); try { await delay(retryData.retryInterval); diff --git a/sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts b/sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts index f27c43d12f73..af0a3abeaceb 100644 --- a/sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts +++ b/sdk/identity/identity/src/credentials/managedIdentityCredential/imdsMsi.ts @@ -93,7 +93,6 @@ export const imdsMsi: MSI = { webResource.timeout = updatedOptions?.requestOptions?.timeout || 500; try { - logger.info(`Pinging IMDS endpoint`); await identityClient.sendRequest(webResource); } catch (err) { if ( diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index 4d9fdf5e5cb6..ff03d8d19031 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -248,6 +248,46 @@ describe("ManagedIdentityCredential", function() { ); }); + it.only("IMDS MSI retries also retries on 503s", async function() { + process.env.AZURE_CLIENT_ID = "errclient"; + + const mockHttpClient = new MockAuthHttpClient({ + // First response says the IMDS endpoint is available. + authResponse: [ + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 200 }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { + status: 200, + parsedBody: { + access_token: "token" + } + } + ] + }); + + const credential = new ManagedIdentityCredential(process.env.AZURE_CLIENT_ID, { + ...mockHttpClient.tokenCredentialOptions, + retryOptions: { + maxRetries: 3 + } + }); + + const promise = credential.getToken("scopes"); + + imdsMsiRetryConfig.startDelayInMs = 80; // 800ms / 10 + + // 800ms -> 1600ms -> 3200ms, results in 6400ms, / 10 = 640ms + // Plus four 503s: 20s * 6 + clock.tick(640 + 2000 * 6); + + assert.equal((await promise).token, "token"); + }); + // Unavailable exception throws while IMDS endpoint is unavailable. This test not valid. // it("can extend timeout for IMDS endpoint", async function() { // // Mock a timeout so that the endpoint ping fails From efef18fe2be629bc0f08c706ced18922282d38e6 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Wed, 23 Jun 2021 23:29:37 +0000 Subject: [PATCH 05/13] the rest of the changes --- .../src/policies/throttlingRetryPolicy.ts | 3 +- .../src/util/throttlingRetryStrategy.ts | 3 ++ sdk/core/core-rest-pipeline/CHANGELOG.md | 1 + .../src/policies/throttlingRetryPolicy.ts | 29 ++++++----- .../test/throttlingRetryPolicy.spec.ts | 48 +++++++++++++++++++ .../node/managedIdentityCredential.spec.ts | 40 +++++++--------- .../node/clientSecretCredential.spec.ts | 3 +- 7 files changed, 92 insertions(+), 35 deletions(-) diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index 4a79f5564cfb..99b6b28e1130 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -74,7 +74,8 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { if (delayInMs) { this.numberOfRetries += 1; await delay(delayInMs); - if (this.numberOfRetries < DEFAULT_CLIENT_MAX_RETRY_COUNT) { + // The original request and up to DEFAULT_CLIENT_MAX_RETRY_COUNT retries + if (this.numberOfRetries <= DEFAULT_CLIENT_MAX_RETRY_COUNT) { return this.sendRequest(httpRequest); } else { return this._nextPolicy.sendRequest(httpRequest); diff --git a/sdk/core/core-http/src/util/throttlingRetryStrategy.ts b/sdk/core/core-http/src/util/throttlingRetryStrategy.ts index 6baae238f608..88b6208d889e 100644 --- a/sdk/core/core-http/src/util/throttlingRetryStrategy.ts +++ b/sdk/core/core-http/src/util/throttlingRetryStrategy.ts @@ -1,4 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +/** + * Maximum number of retries for the throttling retry policy + */ export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3; diff --git a/sdk/core/core-rest-pipeline/CHANGELOG.md b/sdk/core/core-rest-pipeline/CHANGELOG.md index c390807f2a90..a7bf943e1627 100644 --- a/sdk/core/core-rest-pipeline/CHANGELOG.md +++ b/sdk/core/core-rest-pipeline/CHANGELOG.md @@ -10,6 +10,7 @@ - Added support for the `Retry-After` header on responses with status code 503, Service Unavailable. - The `ExponentialRetryPolicy` will now ignore `503` responses if they have the `Retry-After` header. +- Added support for multiple retries on the `ThrottlingRetryPolicy` (up to 3 by default). ## 1.1.0-beta.3 (2021-06-03) diff --git a/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts index 413ef12660c4..abe143fb31a2 100644 --- a/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts @@ -10,6 +10,11 @@ import { delay } from "../util/helpers"; */ export const throttlingRetryPolicyName = "throttlingRetryPolicy"; +/** + * Maximum number of retries for the throttling retry policy + */ +export const DEFAULT_CLIENT_MAX_RETRY_COUNT = 3; + /** * A policy that retries when the server sends a 429 response with a Retry-After header. * @@ -22,21 +27,23 @@ export function throttlingRetryPolicy(): PipelinePolicy { return { name: throttlingRetryPolicyName, async sendRequest(request: PipelineRequest, next: SendRequest): Promise { - const response = await next(request); - if (response.status !== 429 && response.status !== 503) { - return response; - } + let response = await next(request); - const retryAfterHeader = response.headers.get("Retry-After"); - - if (retryAfterHeader) { + for (let count = 0; count <= DEFAULT_CLIENT_MAX_RETRY_COUNT; count++) { + if (response.status !== 429 && response.status !== 503) { + return response; + } + const retryAfterHeader = response.headers.get("Retry-After"); + if (!retryAfterHeader) { + break; + } const delayInMs = parseRetryAfterHeader(retryAfterHeader); - if (delayInMs) { - await delay(delayInMs); - return next(request); + if (!delayInMs) { + break; } + await delay(delayInMs); + response = await next(request); } - return response; } }; diff --git a/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts b/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts index 0ac2d493ea8a..84225435a967 100644 --- a/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts +++ b/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts @@ -2,6 +2,7 @@ // Licensed under the MIT license. import { assert } from "chai"; +import { Context } from "mocha"; import * as sinon from "sinon"; import { createPipelineRequest, @@ -175,4 +176,51 @@ describe("throttlingRetryPolicy", function() { assert.strictEqual(result, successResponse); clock.restore(); }); + + it("It should retry up to three times", async function(this: Context) { + const clock = sinon.useFakeTimers(); + + const request = createPipelineRequest({ + url: "https://bing.com" + }); + const retryResponse: PipelineResponse = { + headers: createHttpHeaders({ + "Retry-After": "1" + }), + request, + status: 503 + }; + const successResponse: PipelineResponse = { + headers: createHttpHeaders(), + request, + status: 200 + }; + + let policy = throttlingRetryPolicy(); + let next = sinon.stub, ReturnType>(); + next.onCall(0).resolves(retryResponse); + next.onCall(1).resolves(retryResponse); + next.onCall(2).resolves(retryResponse); + next.onCall(3).resolves(successResponse); + + let promise = policy.sendRequest(request, next); + await clock.tickAsync(4000); + let response = await promise; + assert.equal(response.status, 200); + + policy = throttlingRetryPolicy(); + next = sinon.stub, ReturnType>(); + next.onCall(0).resolves(retryResponse); + next.onCall(1).resolves(retryResponse); + next.onCall(2).resolves(retryResponse); + next.onCall(3).resolves(retryResponse); + next.onCall(4).resolves(retryResponse); + + promise = policy.sendRequest(request, next); + await clock.tickAsync(5000); + response = await promise; + assert.equal(response.status, 503); + + clock.restore(); + }); }); diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index ff03d8d19031..4b8b18bdf1ce 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -28,7 +28,6 @@ interface AuthRequestDetails { describe("ManagedIdentityCredential", function() { let envCopy: string = ""; let sandbox: Sinon.SinonSandbox; - let clock: Sinon.SinonFakeTimers; beforeEach(() => { envCopy = JSON.stringify(process.env); @@ -39,10 +38,6 @@ describe("ManagedIdentityCredential", function() { delete process.env.IDENTITY_SERVER_THUMBPRINT; delete process.env.IMDS_ENDPOINT; sandbox = Sinon.createSandbox(); - clock = sandbox.useFakeTimers({ - now: Date.now(), - shouldAdvanceTime: true - }); }); afterEach(() => { const env = JSON.parse(envCopy); @@ -53,7 +48,6 @@ describe("ManagedIdentityCredential", function() { process.env.IDENTITY_SERVER_THUMBPRINT = env.IDENTITY_SERVER_THUMBPRINT; process.env.IMDS_ENDPOINT = env.IMDS_ENDPOINT; sandbox.restore(); - clock.restore(); }); it("sends an authorization request with a modified resource name", async function() { @@ -232,12 +226,11 @@ describe("ManagedIdentityCredential", function() { ...mockHttpClient.tokenCredentialOptions }); + const clock = sandbox.useFakeTimers(); const promise = credential.getToken("scopes"); - imdsMsiRetryConfig.startDelayInMs = 80; // 800ms / 10 - - // 800ms -> 1600ms -> 3200ms, results in 6400ms, / 10 = 640ms - clock.tick(640); + // 800ms -> 1600ms -> 3200ms, results in 6400ms + await clock.tickAsync(6400); await assertRejects( promise, @@ -246,9 +239,10 @@ describe("ManagedIdentityCredential", function() { `Failed to retrieve IMDS token after ${imdsMsiRetryConfig.maxRetries} retries.` ) > -1 ); + clock.restore(); }); - it.only("IMDS MSI retries also retries on 503s", async function() { + it("IMDS MSI retries also retries on 503s", async function() { process.env.AZURE_CLIENT_ID = "errclient"; const mockHttpClient = new MockAuthHttpClient({ @@ -257,10 +251,14 @@ describe("ManagedIdentityCredential", function() { { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + // The ThrottlingRetryPolicy of core-http will retry up to 3 times, an extra retry would make this fail (meaning a 503 response would be considered the result) + // { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 200 }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, + { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 200, parsedBody: { @@ -270,22 +268,20 @@ describe("ManagedIdentityCredential", function() { ] }); - const credential = new ManagedIdentityCredential(process.env.AZURE_CLIENT_ID, { - ...mockHttpClient.tokenCredentialOptions, - retryOptions: { - maxRetries: 3 - } - }); + const credential = new ManagedIdentityCredential( + process.env.AZURE_CLIENT_ID, + mockHttpClient.tokenCredentialOptions + ); + const clock = sandbox.useFakeTimers(); const promise = credential.getToken("scopes"); - imdsMsiRetryConfig.startDelayInMs = 80; // 800ms / 10 - - // 800ms -> 1600ms -> 3200ms, results in 6400ms, / 10 = 640ms - // Plus four 503s: 20s * 6 - clock.tick(640 + 2000 * 6); + // 800ms -> 1600ms -> 3200ms, results in 6400ms + // Plus four 503s: 20s * 8 + await clock.tickAsync(6400 + 2000 * 8); assert.equal((await promise).token, "token"); + clock.restore(); }); // Unavailable exception throws while IMDS endpoint is unavailable. This test not valid. diff --git a/sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts b/sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts index 46d088e86898..2742ea24790a 100644 --- a/sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts +++ b/sdk/identity/identity/test/public/node/clientSecretCredential.spec.ts @@ -82,7 +82,8 @@ describe("ClientSecretCredential", function() { }) ); - it("supports specifying the regional authority", async function(this: Context) { + // This test is extremely slow. Let's skip it for now. + it.skip("supports specifying the regional authority", async function(this: Context) { if (isLiveMode()) { this.skip(); } From fdd9ecf045b41be3e347c37e8b016c6731771ec3 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Fri, 25 Jun 2021 16:36:11 +0000 Subject: [PATCH 06/13] temporary fix for Node 16 --- .../internal/node/managedIdentityCredential.spec.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index 4b8b18bdf1ce..35df984a7a10 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -226,11 +226,16 @@ describe("ManagedIdentityCredential", function() { ...mockHttpClient.tokenCredentialOptions }); - const clock = sandbox.useFakeTimers(); + // Sinon's clock has some issues with Node 16. + // If we remove this conditional on Node 16, we get the following error: + // PromiseRejection HandledWarning: Promise rejection was handled asynchronously + // It will point to the `await promise` line below. + const clock = process.version.startsWith("v16") ? undefined : sandbox.useFakeTimers(); + const promise = credential.getToken("scopes"); // 800ms -> 1600ms -> 3200ms, results in 6400ms - await clock.tickAsync(6400); + await clock?.tickAsync(6400); await assertRejects( promise, @@ -239,7 +244,7 @@ describe("ManagedIdentityCredential", function() { `Failed to retrieve IMDS token after ${imdsMsiRetryConfig.maxRetries} retries.` ) > -1 ); - clock.restore(); + clock?.restore(); }); it("IMDS MSI retries also retries on 503s", async function() { From b561d1992050baeefe0671d4267d3a07fa7154b0 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Fri, 25 Jun 2021 18:17:13 +0000 Subject: [PATCH 07/13] feedback by Jeremy --- .../src/policies/throttlingRetryPolicy.ts | 19 +++++++++---------- .../node/managedIdentityCredential.spec.ts | 4 +--- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index 23d913324ad4..759fb3dfeee1 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -50,16 +50,15 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { } public async sendRequest(httpRequest: WebResourceLike): Promise { - return this._nextPolicy.sendRequest(httpRequest.clone()).then((response) => { - if ( - response.status !== StatusCodes.TooManyRequests && - response.status !== StatusCodes.ServiceUnavailable - ) { - return response; - } else { - return this._handleResponse(httpRequest, response); - } - }); + const response = await this._nextPolicy.sendRequest(httpRequest.clone()); + if ( + response.status !== StatusCodes.TooManyRequests && + response.status !== StatusCodes.ServiceUnavailable + ) { + return response; + } else { + return this._handleResponse(httpRequest, response); + } } private async _defaultResponseHandler( diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index 35df984a7a10..ea68e038ad1b 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -248,8 +248,6 @@ describe("ManagedIdentityCredential", function() { }); it("IMDS MSI retries also retries on 503s", async function() { - process.env.AZURE_CLIENT_ID = "errclient"; - const mockHttpClient = new MockAuthHttpClient({ // First response says the IMDS endpoint is available. authResponse: [ @@ -274,7 +272,7 @@ describe("ManagedIdentityCredential", function() { }); const credential = new ManagedIdentityCredential( - process.env.AZURE_CLIENT_ID, + "errclient", mockHttpClient.tokenCredentialOptions ); From 33260ec57203fa2c075a8a8ab78eee9c22382671 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Fri, 25 Jun 2021 18:59:09 +0000 Subject: [PATCH 08/13] Since unhandled promise rejections are not supported in Node 16, tickAsync can't be awaited --- .../internal/node/managedIdentityCredential.spec.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index ea68e038ad1b..92b0f3845a3c 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -226,16 +226,15 @@ describe("ManagedIdentityCredential", function() { ...mockHttpClient.tokenCredentialOptions }); - // Sinon's clock has some issues with Node 16. - // If we remove this conditional on Node 16, we get the following error: - // PromiseRejection HandledWarning: Promise rejection was handled asynchronously - // It will point to the `await promise` line below. - const clock = process.version.startsWith("v16") ? undefined : sandbox.useFakeTimers(); + const clock = sandbox.useFakeTimers(); const promise = credential.getToken("scopes"); + // Important: + // We can't await tickAsync on Node 16, since it makes the promise above reject without the proper error handling. + // This is true even when we're properly handling the promise rejection later with assertRejects, // 800ms -> 1600ms -> 3200ms, results in 6400ms - await clock?.tickAsync(6400); + clock?.tickAsync(6400); await assertRejects( promise, From 502f6828e8ce8052b30bda209cd7ff7b52cbbe84 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Fri, 25 Jun 2021 19:06:30 +0000 Subject: [PATCH 09/13] some comment improvements --- .../node/managedIdentityCredential.spec.ts | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index 92b0f3845a3c..cc6cb90d2f66 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -25,7 +25,7 @@ interface AuthRequestDetails { token: AccessToken | null; } -describe("ManagedIdentityCredential", function() { +describe("ManagedIdentityCredential", function () { let envCopy: string = ""; let sandbox: Sinon.SinonSandbox; @@ -50,7 +50,7 @@ describe("ManagedIdentityCredential", function() { sandbox.restore(); }); - it("sends an authorization request with a modified resource name", async function() { + it("sends an authorization request with a modified resource name", async function () { const authDetails = await getMsiTokenAuthRequest(["https://service/.default"], "client", { authResponse: [ { status: 200 }, // Respond to IMDS isAvailable @@ -107,7 +107,7 @@ describe("ManagedIdentityCredential", function() { } }); - it("returns error when no MSI is available", async function() { + it("returns error when no MSI is available", async function () { process.env.AZURE_CLIENT_ID = "errclient"; const imdsError: RestError = new RestError("Request Timeout", "REQUEST_SEND_ERROR", 408); @@ -124,7 +124,7 @@ describe("ManagedIdentityCredential", function() { ); }); - it("an unexpected error bubbles all the way up", async function() { + it("an unexpected error bubbles all the way up", async function () { process.env.AZURE_CLIENT_ID = "errclient"; const errResponse: OAuthErrorResponse = { @@ -145,7 +145,7 @@ describe("ManagedIdentityCredential", function() { ); }); - it("returns expected error when the network was unreachable", async function() { + it("returns expected error when the network was unreachable", async function () { process.env.AZURE_CLIENT_ID = "errclient"; const netError: RestError = new RestError("Request Timeout", "ENETUNREACH", 408); @@ -162,7 +162,7 @@ describe("ManagedIdentityCredential", function() { ); }); - it("returns expected error when the host was unreachable", async function() { + it("returns expected error when the host was unreachable", async function () { process.env.AZURE_CLIENT_ID = "errclient"; const hostError: RestError = new RestError("Request Timeout", "EHOSTUNREACH", 408); @@ -180,7 +180,7 @@ describe("ManagedIdentityCredential", function() { ); }); - it("IMDS MSI retries and succeeds on 404", async function() { + it("IMDS MSI retries and succeeds on 404", async function () { process.env.AZURE_CLIENT_ID = "errclient"; const mockHttpClient = new MockAuthHttpClient({ @@ -208,7 +208,7 @@ describe("ManagedIdentityCredential", function() { assert.equal(response.token, "token"); }); - it("IMDS MSI retries up to a limit on 404", async function() { + it("IMDS MSI retries up to a limit on 404", async function () { process.env.AZURE_CLIENT_ID = "errclient"; const mockHttpClient = new MockAuthHttpClient({ @@ -227,13 +227,14 @@ describe("ManagedIdentityCredential", function() { }); const clock = sandbox.useFakeTimers(); - const promise = credential.getToken("scopes"); // Important: // We can't await tickAsync on Node 16, since it makes the promise above reject without the proper error handling. - // This is true even when we're properly handling the promise rejection later with assertRejects, - // 800ms -> 1600ms -> 3200ms, results in 6400ms + // This is true even when we're properly handling the promise rejection later with assertRejects. + // + // As per the source code of the IMDS MSI, + // the timeouts increase exponentially until we reach the limit: 800ms -> 1600ms -> 3200ms, results in 6400ms clock?.tickAsync(6400); await assertRejects( @@ -246,7 +247,7 @@ describe("ManagedIdentityCredential", function() { clock?.restore(); }); - it("IMDS MSI retries also retries on 503s", async function() { + it("IMDS MSI retries also retries on 503s", async function () { const mockHttpClient = new MockAuthHttpClient({ // First response says the IMDS endpoint is available. authResponse: [ @@ -378,7 +379,7 @@ describe("ManagedIdentityCredential", function() { } }); - it("sends an authorization request correctly in an Azure Arc environment", async function() { + it("sends an authorization request correctly in an Azure Arc environment", async function () { // Trigger Azure Arc behavior by setting environment variables process.env.IMDS_ENDPOINT = "https://endpoint"; From 023ce553b128fa703eb3fb5a865ef61539eaeb69 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Fri, 25 Jun 2021 21:22:10 +0000 Subject: [PATCH 10/13] small improvement --- .../test/internal/node/managedIdentityCredential.spec.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index 233628f83a5b..0d7c1e31c0b5 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -279,9 +279,11 @@ describe("ManagedIdentityCredential", function() { const clock = sandbox.useFakeTimers(); const promise = credential.getToken("scopes"); + // From the retry code of the IMDS MSI, + // the timeouts increase exponentially until we reach the limit: // 800ms -> 1600ms -> 3200ms, results in 6400ms - // Plus four 503s: 20s * 8 - await clock.tickAsync(6400 + 2000 * 8); + // Plus four 503s: 20s * 8 from the 503 responses. + clock.tickAsync(6400 + 2000 * 8); assert.equal((await promise).token, "token"); clock.restore(); From 8b7fa25f9d23f89ba4af477f6957c9697c944081 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Rodr=C3=ADguez?= Date: Mon, 28 Jun 2021 18:52:35 -0400 Subject: [PATCH 11/13] Update sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts Co-authored-by: Jeff Fisher --- .../core-rest-pipeline/src/policies/exponentialRetryPolicy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts index c138ee2cfb57..e55af5aa694e 100644 --- a/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts @@ -72,7 +72,7 @@ export function exponentialRetryPolicy( */ function shouldRetry(response: PipelineResponse | undefined, retryData: RetryData): boolean { const statusCode = response?.status; - if (statusCode && statusCode === 503 && response && response.headers.get("Retry-After")) { + if (statusCode === 503 && response?.headers.get("Retry-After")) { return false; } From 3539c30a4f252bf5a25bd36df89253313d8c92d3 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Tue, 29 Jun 2021 01:42:52 +0000 Subject: [PATCH 12/13] core-http feedback from Jeff, and tests --- .../src/policies/exponentialRetryPolicy.ts | 7 +- .../src/policies/throttlingRetryPolicy.ts | 18 ++-- .../policies/throttlingRetryPolicyTests.ts | 94 +++++++++++++++++++ 3 files changed, 104 insertions(+), 15 deletions(-) diff --git a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts index afc06b35794d..046f419bad05 100644 --- a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts @@ -140,12 +140,7 @@ async function retry( ): Promise { function shouldPolicyRetry(responseParam?: HttpOperationResponse): boolean { const statusCode = responseParam?.status; - if ( - statusCode && - statusCode === 503 && - response && - response.headers.get(Constants.HeaderConstants.RETRY_AFTER) - ) { + if (statusCode === 503 && response?.headers.get(Constants.HeaderConstants.RETRY_AFTER)) { return false; } diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index 759fb3dfeee1..66a8b2da8dcc 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -76,17 +76,17 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { if (delayInMs) { this.numberOfRetries += 1; - await delay(delayInMs, undefined, { - abortSignal: httpRequest.abortSignal, - abortErrorMsg: StandardAbortMessage - }); - - if (httpRequest.abortSignal?.aborted) { - throw new AbortError(StandardAbortMessage); - } - // The original request and up to DEFAULT_CLIENT_MAX_RETRY_COUNT retries if (this.numberOfRetries <= DEFAULT_CLIENT_MAX_RETRY_COUNT) { + await delay(delayInMs, undefined, { + abortSignal: httpRequest.abortSignal, + abortErrorMsg: StandardAbortMessage + }); + + if (httpRequest.abortSignal?.aborted) { + throw new AbortError(StandardAbortMessage); + } + return this.sendRequest(httpRequest); } else { return this._nextPolicy.sendRequest(httpRequest); diff --git a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts index 8b53d815cd10..e27b275bb388 100644 --- a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts +++ b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts @@ -136,6 +136,100 @@ describe("ThrottlingRetryPolicy", () => { assert.deepEqual(response, mockResponse); }); + it("if the status code equals 429, it should retry up to 3 times", async () => { + const request = new WebResource(); + const status = 429; + const retryResponse = { + status, + headers: new HttpHeaders({ + "Retry-After": "1" + }), + request + }; + const responses: HttpOperationResponse[] = [ + retryResponse, + retryResponse, + retryResponse, + retryResponse, + // This one should be returned + { + status, + headers: new HttpHeaders({ + "Retry-After": "1", + "final-response": "final-response" + }), + request + } + ]; + + const clock = sinon.useFakeTimers(); + + const policy = new ThrottlingRetryPolicy( + { + async sendRequest(): Promise { + return responses.shift()!; + } + }, + new RequestPolicyOptions() + ); + + const promise = policy.sendRequest(request); + clock.tickAsync(3000); + + const response = await promise; + assert.deepEqual(response.status, status); + assert.deepEqual(response.headers.get("final-response"), "final-response"); + + clock.restore(); + }); + + it("if the status code equals 503, it should retry up to 3 times", async () => { + const request = new WebResource(); + const status = 503; + const retryResponse = { + status, + headers: new HttpHeaders({ + "Retry-After": "1" + }), + request + }; + const responses: HttpOperationResponse[] = [ + retryResponse, + retryResponse, + retryResponse, + retryResponse, + // This one should be returned + { + status, + headers: new HttpHeaders({ + "Retry-After": "1", + "final-response": "final-response" + }), + request + } + ]; + + const clock = sinon.useFakeTimers(); + + const policy = new ThrottlingRetryPolicy( + { + async sendRequest(): Promise { + return responses.shift()!; + } + }, + new RequestPolicyOptions() + ); + + const promise = policy.sendRequest(request); + clock.tickAsync(3000); + + const response = await promise; + assert.deepEqual(response.status, status); + assert.deepEqual(response.headers.get("final-response"), "final-response"); + + clock.restore(); + }); + it("should honor the abort signal passed", async () => { const request = new WebResource( "https://fakeservice.io", From a6636a138817dc95fad87120f71a70df5859bb9f Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Tue, 29 Jun 2021 03:01:39 +0000 Subject: [PATCH 13/13] the rest of the feedback, and test improvements --- .../src/policies/throttlingRetryPolicy.ts | 17 ++++---- .../policies/throttlingRetryPolicyTests.ts | 6 +-- .../src/policies/throttlingRetryPolicy.ts | 2 +- .../test/throttlingRetryPolicy.spec.ts | 39 +++++++------------ .../node/managedIdentityCredential.spec.ts | 6 +-- 5 files changed, 28 insertions(+), 42 deletions(-) diff --git a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts index 66a8b2da8dcc..00f86ebceb59 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -76,17 +76,16 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { if (delayInMs) { this.numberOfRetries += 1; - // The original request and up to DEFAULT_CLIENT_MAX_RETRY_COUNT retries - if (this.numberOfRetries <= DEFAULT_CLIENT_MAX_RETRY_COUNT) { - await delay(delayInMs, undefined, { - abortSignal: httpRequest.abortSignal, - abortErrorMsg: StandardAbortMessage - }); + await delay(delayInMs, undefined, { + abortSignal: httpRequest.abortSignal, + abortErrorMsg: StandardAbortMessage + }); - if (httpRequest.abortSignal?.aborted) { - throw new AbortError(StandardAbortMessage); - } + if (httpRequest.abortSignal?.aborted) { + throw new AbortError(StandardAbortMessage); + } + if (this.numberOfRetries < DEFAULT_CLIENT_MAX_RETRY_COUNT) { return this.sendRequest(httpRequest); } else { return this._nextPolicy.sendRequest(httpRequest); diff --git a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts index e27b275bb388..ff60b9980ab5 100644 --- a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts +++ b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts @@ -150,7 +150,6 @@ describe("ThrottlingRetryPolicy", () => { retryResponse, retryResponse, retryResponse, - retryResponse, // This one should be returned { status, @@ -178,7 +177,7 @@ describe("ThrottlingRetryPolicy", () => { const response = await promise; assert.deepEqual(response.status, status); - assert.deepEqual(response.headers.get("final-response"), "final-response"); + assert.equal(response.headers.get("final-response"), "final-response"); clock.restore(); }); @@ -197,7 +196,6 @@ describe("ThrottlingRetryPolicy", () => { retryResponse, retryResponse, retryResponse, - retryResponse, // This one should be returned { status, @@ -225,7 +223,7 @@ describe("ThrottlingRetryPolicy", () => { const response = await promise; assert.deepEqual(response.status, status); - assert.deepEqual(response.headers.get("final-response"), "final-response"); + assert.equal(response.headers.get("final-response"), "final-response"); clock.restore(); }); diff --git a/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts index abe143fb31a2..54bf26c92126 100644 --- a/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts @@ -29,7 +29,7 @@ export function throttlingRetryPolicy(): PipelinePolicy { async sendRequest(request: PipelineRequest, next: SendRequest): Promise { let response = await next(request); - for (let count = 0; count <= DEFAULT_CLIENT_MAX_RETRY_COUNT; count++) { + for (let count = 0; count < DEFAULT_CLIENT_MAX_RETRY_COUNT; count++) { if (response.status !== 429 && response.status !== 503) { return response; } diff --git a/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts b/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts index 84225435a967..c176d6fb6d7b 100644 --- a/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts +++ b/sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts @@ -190,36 +190,27 @@ describe("throttlingRetryPolicy", function() { request, status: 503 }; - const successResponse: PipelineResponse = { - headers: createHttpHeaders(), - request, - status: 200 - }; - - let policy = throttlingRetryPolicy(); - let next = sinon.stub, ReturnType>(); - next.onCall(0).resolves(retryResponse); - next.onCall(1).resolves(retryResponse); - next.onCall(2).resolves(retryResponse); - next.onCall(3).resolves(successResponse); - let promise = policy.sendRequest(request, next); - await clock.tickAsync(4000); - let response = await promise; - assert.equal(response.status, 200); - - policy = throttlingRetryPolicy(); - next = sinon.stub, ReturnType>(); + const policy = throttlingRetryPolicy(); + const next = sinon.stub, ReturnType>(); next.onCall(0).resolves(retryResponse); next.onCall(1).resolves(retryResponse); next.onCall(2).resolves(retryResponse); - next.onCall(3).resolves(retryResponse); - next.onCall(4).resolves(retryResponse); + // This one should be returned + next.onCall(3).resolves({ + headers: createHttpHeaders({ + "Retry-After": "1", + "final-response": "final-response" + }), + request, + status: 503 + }); - promise = policy.sendRequest(request, next); - await clock.tickAsync(5000); - response = await promise; + const promise = policy.sendRequest(request, next); + await clock.tickAsync(3000); + const response = await promise; assert.equal(response.status, 503); + assert.equal(response.headers.get("final-response"), "final-response"); clock.restore(); }); diff --git a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts index 0d7c1e31c0b5..2d67507b542e 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -254,14 +254,12 @@ describe("ManagedIdentityCredential", function() { { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, - { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, // The ThrottlingRetryPolicy of core-http will retry up to 3 times, an extra retry would make this fail (meaning a 503 response would be considered the result) // { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 200 }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, - { status: 503, headers: new HttpHeaders({ "Retry-After": "2" }) }, { status: 200, parsedBody: { @@ -282,8 +280,8 @@ describe("ManagedIdentityCredential", function() { // From the retry code of the IMDS MSI, // the timeouts increase exponentially until we reach the limit: // 800ms -> 1600ms -> 3200ms, results in 6400ms - // Plus four 503s: 20s * 8 from the 503 responses. - clock.tickAsync(6400 + 2000 * 8); + // Plus four 503s: 20s * 6 from the 503 responses. + clock.tickAsync(6400 + 2000 * 6); assert.equal((await promise).token, "token"); clock.restore();