Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Retry on 503 using the Retry-After header #15906

Merged
17 commits merged into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sdk/core/core-http/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion sdk/core/core-http/src/policies/throttlingRetryPolicy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ export class ThrottlingRetryPolicy extends BaseRequestPolicy {

public async sendRequest(httpRequest: WebResourceLike): Promise<HttpOperationResponse> {
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);
Expand Down
3 changes: 2 additions & 1 deletion sdk/core/core-http/src/util/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ export const Constants = {
},

StatusCodes: {
TooManyRequests: 429
TooManyRequests: 429,
ServiceUnavailable: 503
}
},

Expand Down
34 changes: 28 additions & 6 deletions sdk/core/core-http/test/policies/throttlingRetryPolicyTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpOperationResponse> {
const response = {
...this._response,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"
Expand All @@ -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");

Expand Down
4 changes: 4 additions & 0 deletions sdk/core/core-rest-pipeline/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export function throttlingRetryPolicy(): PipelinePolicy {
name: throttlingRetryPolicyName,
async sendRequest(request: PipelineRequest, next: SendRequest): Promise<PipelineResponse> {
const response = await next(request);
if (response.status !== 429) {
if (response.status !== 429 && response.status !== 503) {
return response;
}

Expand Down
84 changes: 82 additions & 2 deletions sdk/core/core-rest-pipeline/test/throttlingRetryPolicy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
});
Expand Down Expand Up @@ -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"
});
Expand Down Expand Up @@ -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<Parameters<SendRequest>, ReturnType<SendRequest>>();
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<Parameters<SendRequest>, ReturnType<SendRequest>>();
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();
});
});