diff --git a/sdk/core/core-http/CHANGELOG.md b/sdk/core/core-http/CHANGELOG.md index 75dee5caa518..7163b4275fc6 100644 --- a/sdk/core/core-http/CHANGELOG.md +++ b/sdk/core/core-http/CHANGELOG.md @@ -4,7 +4,9 @@ ### Features Added -- Changed TS compilation target to ES2017 in order to produce smaller bundles and use more native platform features +- Changed TS compilation target to ES2017 in order to produce smaller bundles and use more native platform features. +- 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/review/core-http.api.md b/sdk/core/core-http/review/core-http.api.md index 37110cddac5c..0d938fb86051 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: { diff --git a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts index 0623e5a00586..046f419bad05 100644 --- a/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/exponentialRetryPolicy.ts @@ -21,6 +21,7 @@ import { } from "../util/exponentialBackoffStrategy"; import { RestError } from "../restError"; import { logger } from "../log"; +import { Constants } from "../util/constants"; import { delay } from "../util/delay"; export function exponentialRetryPolicy( @@ -139,6 +140,10 @@ async function retry( ): Promise { function shouldPolicyRetry(responseParam?: HttpOperationResponse): boolean { const statusCode = responseParam?.status; + if (statusCode === 503 && 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 5d928a49a9b7..00f86ebceb59 100644 --- a/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts +++ b/sdk/core/core-http/src/policies/throttlingRetryPolicy.ts @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT license. +import { AbortError } from "@azure/abort-controller"; import { BaseRequestPolicy, RequestPolicy, @@ -10,8 +11,8 @@ import { import { WebResourceLike } from "../webResource"; import { HttpOperationResponse } from "../httpOperationResponse"; import { Constants } from "../util/constants"; +import { DEFAULT_CLIENT_MAX_RETRY_COUNT } from "../util/throttlingRetryStrategy"; import { delay } from "../util/delay"; -import { AbortError } from "@azure/abort-controller"; type ResponseHandler = ( httpRequest: WebResourceLike, @@ -37,6 +38,7 @@ const StandardAbortMessage = "The operation was aborted."; */ export class ThrottlingRetryPolicy extends BaseRequestPolicy { private _handleResponse: ResponseHandler; + private numberOfRetries = 0; constructor( nextPolicy: RequestPolicy, @@ -48,13 +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) { - 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( @@ -70,14 +74,22 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy { retryAfterHeader ); if (delayInMs) { + this.numberOfRetries += 1; + await delay(delayInMs, undefined, { abortSignal: httpRequest.abortSignal, abortErrorMsg: StandardAbortMessage }); + if (httpRequest.abortSignal?.aborted) { throw new AbortError(StandardAbortMessage); } - return this._nextPolicy.sendRequest(httpRequest); + + 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/constants.ts b/sdk/core/core-http/src/util/constants.ts index 9e51d987813f..54d629cd1412 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/src/util/throttlingRetryStrategy.ts b/sdk/core/core-http/src/util/throttlingRetryStrategy.ts new file mode 100644 index 000000000000..88b6208d889e --- /dev/null +++ b/sdk/core/core-http/src/util/throttlingRetryStrategy.ts @@ -0,0 +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-http/test/policies/throttlingRetryPolicyTests.ts b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts index c14a873c6196..ff60b9980ab5 100644 --- a/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts +++ b/sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts @@ -72,7 +72,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, @@ -114,6 +114,120 @@ describe("ThrottlingRetryPolicy", () => { 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); + }); + + 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, + // 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.equal(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, + // 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.equal(response.headers.get("final-response"), "final-response"); + + clock.restore(); + }); + it("should honor the abort signal passed", async () => { const request = new WebResource( "https://fakeservice.io", diff --git a/sdk/core/core-rest-pipeline/CHANGELOG.md b/sdk/core/core-rest-pipeline/CHANGELOG.md index 204c6f00e236..2fc48a81256d 100644 --- a/sdk/core/core-rest-pipeline/CHANGELOG.md +++ b/sdk/core/core-rest-pipeline/CHANGELOG.md @@ -6,6 +6,12 @@ - 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. +- 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). + ### Breaking Changes - Updated @azure/core-tracing to version `1.0.0-preview.12`. See [@azure/core-tracing CHANGELOG](https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-tracing/CHANGELOG.md) for details about breaking changes with tracing. diff --git a/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/exponentialRetryPolicy.ts index 23784c619fd8..e55af5aa694e 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 === 503 && 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/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts b/sdk/core/core-rest-pipeline/src/policies/throttlingRetryPolicy.ts index 71e06e48978c..54bf26c92126 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) { - 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 d0586a3cfcf5..c176d6fb6d7b 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, @@ -16,7 +17,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 +55,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 +96,122 @@ 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(); + }); + + 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 policy = throttlingRetryPolicy(); + const next = sinon.stub, ReturnType>(); + next.onCall(0).resolves(retryResponse); + next.onCall(1).resolves(retryResponse); + next.onCall(2).resolves(retryResponse); + // This one should be returned + next.onCall(3).resolves({ + headers: createHttpHeaders({ + "Retry-After": "1", + "final-response": "final-response" + }), + request, + status: 503 + }); + + 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/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 aa6ac5f726bd..2d67507b542e 100644 --- a/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts +++ b/sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts @@ -232,15 +232,58 @@ describe("ManagedIdentityCredential", function() { credential.getToken("scopes").catch((error) => { errorMessage = error.message; }); - // 800ms -> 1600ms -> 3200ms, results in 6400ms + // From the retry code of the IMDS MSI, + // the timeouts increase exponentially until we reach the limit: + // 800ms -> 1600ms -> 3200ms, results in 6400ms await clock.tickAsync(6400); + assert.ok( errorMessage.indexOf( `Failed to retrieve IMDS token after ${imdsMsiRetryConfig.maxRetries} retries.` ) > -1 ); + clock?.restore(); + }); + + it("IMDS MSI retries also retries on 503s", async function() { + 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" }) }, + // 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: 200, + parsedBody: { + access_token: "token" + } + } + ] + }); + + const credential = new ManagedIdentityCredential( + "errclient", + mockHttpClient.tokenCredentialOptions + ); + + 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 * 6 from the 503 responses. + clock.tickAsync(6400 + 2000 * 6); + + assert.equal((await promise).token, "token"); clock.restore(); });