Merge remote-tracking branch 'upstream/master' into dom-shim

Changelog.md

@@ -1,9 +1,13 @@
 # Changelog
 
-## 2.2.3 - UNRELEASED
+## 2.2.4 - UNRELEASED
 
 - Rework the use of `lib: ["dom"]` so consumers of this package don't need it in their tsconfig. Fixes (Issue [#367](https://github.com/Azure/ms-rest-js/issues/367))
 
+## 2.2.3 (Unreleased)
+
+- `ThrottlingRetryPolicy` now keep retrying on 429 responses up to a limit. Fixes (Issue [#394](https://github.com/Azure/ms-rest-js/issues/394))
+
 ## 2.2.2 - 2021-02-02
 
 - The global `fetch()` is no longer overridden by node-fetch. Fixes (Issue [#383](https://github.com/Azure/ms-rest-js/issues/383))

lib/policies/throttlingRetryPolicy.ts

@@ -12,16 +12,25 @@ import { HttpOperationResponse } from "../httpOperationResponse";
 import { Constants } from "../util/constants";
 import { delay } from "../util/utils";
 
-type ResponseHandler = (
-  httpRequest: WebResourceLike,
-  response: HttpOperationResponse
-) => Promise<HttpOperationResponse>;
 const StatusCodes = Constants.HttpConstants.StatusCodes;
+const DEFAULT_RETRY_COUNT = 3;
 
-export function throttlingRetryPolicy(): RequestPolicyFactory {
+/**
+ * Options that control how to retry on response status code 429.
+ */
+export interface ThrottlingRetryOptions {
+  /**
+   * The maximum number of retry attempts.  Defaults to 3.
+   */
+  maxRetries?: number;
+}
+
+export function throttlingRetryPolicy(
+  maxRetries: number = DEFAULT_RETRY_COUNT
+): RequestPolicyFactory {
   return {
     create: (nextPolicy: RequestPolicy, options: RequestPolicyOptionsLike) => {
-      return new ThrottlingRetryPolicy(nextPolicy, options);
+      return new ThrottlingRetryPolicy(nextPolicy, options, maxRetries);
     },
   };
 }
@@ -33,41 +42,40 @@ export function throttlingRetryPolicy(): RequestPolicyFactory {
  * https://docs.microsoft.com/en-us/azure/virtual-machines/troubleshooting/troubleshooting-throttling-errors
  */
 export class ThrottlingRetryPolicy extends BaseRequestPolicy {
-  private _handleResponse: ResponseHandler;
+  private retryLimit: number;
 
-  constructor(
-    nextPolicy: RequestPolicy,
-    options: RequestPolicyOptionsLike,
-    _handleResponse?: ResponseHandler
-  ) {
+  constructor(nextPolicy: RequestPolicy, options: RequestPolicyOptionsLike, retryLimit: number) {
     super(nextPolicy, options);
-    this._handleResponse = _handleResponse || this._defaultResponseHandler;
+    this.retryLimit = retryLimit;
   }
 
   public async sendRequest(httpRequest: WebResourceLike): Promise<HttpOperationResponse> {
     return this._nextPolicy.sendRequest(httpRequest.clone()).then((response) => {
-      if (response.status !== StatusCodes.TooManyRequests) {
-        return response;
-      } else {
-        return this._handleResponse(httpRequest, response);
-      }
+      return this.retry(httpRequest, response, 0);
     });
   }
 
-  private async _defaultResponseHandler(
+  private async retry(
     httpRequest: WebResourceLike,
-    httpResponse: HttpOperationResponse
+    httpResponse: HttpOperationResponse,
+    retryCount: number
   ): Promise<HttpOperationResponse> {
+    if (httpResponse.status !== StatusCodes.TooManyRequests) {
+      return httpResponse;
+    }
+
     const retryAfterHeader: string | undefined = httpResponse.headers.get(
       Constants.HeaderConstants.RETRY_AFTER
     );
 
-    if (retryAfterHeader) {
+    if (retryAfterHeader && retryCount < this.retryLimit) {
       const delayInMs: number | undefined = ThrottlingRetryPolicy.parseRetryAfterHeader(
         retryAfterHeader
       );
       if (delayInMs) {
-        return delay(delayInMs).then((_: any) => this._nextPolicy.sendRequest(httpRequest));
+        await delay(delayInMs);
+        const res = await this._nextPolicy.sendRequest(httpRequest);
+        return this.retry(httpRequest, res, retryCount + 1);
       }
     }
 

lib/util/constants.ts

@@ -7,7 +7,7 @@ export const Constants = {
    * @const
    * @type {string}
    */
-  msRestVersion: "2.2.3",
+  msRestVersion: "2.2.4",
 
   /**
    * Specifies HTTP.

package.json

@@ -5,7 +5,7 @@
     "email": "azsdkteam@microsoft.com",
     "url": "https://github.com/Azure/ms-rest-js"
   },
-  "version": "2.2.3",
+  "version": "2.2.4",
   "description": "Isomorphic client Runtime for Typescript/node.js/browser javascript client libraries generated using AutoRest",
   "tags": [
     "isomorphic",

test/policies/throttlingRetryPolicyTests.ts

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License. See License.txt in the project root for license information.
 
-import { assert, AssertionError } from "chai";
+import { assert } from "chai";
 import sinon from "sinon";
 import { ThrottlingRetryPolicy } from "../../lib/policies/throttlingRetryPolicy";
 import { WebResource, WebResourceLike } from "../../lib/webResource";
@@ -27,19 +27,37 @@ describe("ThrottlingRetryPolicy", () => {
     headers: new HttpHeaders(),
   };
 
-  function createDefaultThrottlingRetryPolicy(
-    response?: HttpOperationResponse,
-    actionHandler?: (
-      httpRequest: WebResourceLike,
-      response: HttpOperationResponse
-    ) => Promise<HttpOperationResponse>
-  ) {
+  // Inject 429 responses on first numberRetryAfter sendRequest() calls
+  class RetryFirstNRequestsPolicy {
+    public count = 0;
+    constructor(private _response: HttpOperationResponse, private numberRetryAfter: number) {}
+    public sendRequest(request: WebResource): Promise<HttpOperationResponse> {
+      if (this.count < this.numberRetryAfter) {
+        this.count++;
+
+        return Promise.resolve({
+          status: 429,
+          headers: new HttpHeaders({
+            "Retry-After": "1",
+          }),
+          request,
+        });
+      }
+
+      return Promise.resolve({
+        ...this._response,
+        request,
+      });
+    }
+  }
+
+  function createDefaultThrottlingRetryPolicy(response?: HttpOperationResponse) {
     if (!response) {
       response = defaultResponse;
     }
 
     const passThroughPolicy = new PassThroughPolicy(response);
-    return new ThrottlingRetryPolicy(passThroughPolicy, new RequestPolicyOptions(), actionHandler);
+    return new ThrottlingRetryPolicy(passThroughPolicy, new RequestPolicyOptions(), 3);
   }
 
   describe("sendRequest", () => {
@@ -51,7 +69,7 @@ describe("ThrottlingRetryPolicy", () => {
           return Promise.resolve(defaultResponse);
         },
       };
-      const policy = new ThrottlingRetryPolicy(nextPolicy, new RequestPolicyOptions());
+      const policy = new ThrottlingRetryPolicy(nextPolicy, new RequestPolicyOptions(), 3);
       await policy.sendRequest(request);
     });
 
@@ -78,32 +96,36 @@ describe("ThrottlingRetryPolicy", () => {
         }),
         request: request,
       };
-      const policy = createDefaultThrottlingRetryPolicy(mockResponse, (_) => {
-        throw new AssertionError("fail");
-      });
+      const faultyPolicy = new RetryFirstNRequestsPolicy(mockResponse, 0);
+      const policy = new ThrottlingRetryPolicy(faultyPolicy, new RequestPolicyOptions(), 3);
+      const spy = sinon.spy(policy as any, "retry");
 
       const response = await policy.sendRequest(request);
-
       assert.deepEqual(response, mockResponse);
+      assert.strictEqual(spy.callCount, 1);
     });
 
-    it("should pass the response to the handler if the status code equals 429", async () => {
+    it("should retry if the status code equals 429", async () => {
       const request = new WebResource();
-      const mockResponse = {
-        status: 429,
-        headers: new HttpHeaders({
-          "Retry-After": "100",
-        }),
-        request: request,
-      };
-      const policy = createDefaultThrottlingRetryPolicy(mockResponse, (_, response) => {
-        assert.deepEqual(response, mockResponse);
-        return Promise.resolve(response);
-      });
+      const faultyPolicy = new RetryFirstNRequestsPolicy(defaultResponse, 1);
+      const policy = new ThrottlingRetryPolicy(faultyPolicy, new RequestPolicyOptions(), 3);
+      const spy = sinon.spy(policy as any, "retry");
 
       const response = await policy.sendRequest(request);
-      assert.deepEqual(response, mockResponse);
+      assert.deepEqual(response, defaultResponse);
+      assert.strictEqual(spy.callCount, 2); // last retry returns directly for 200 response
     });
+
+    it("should give up on 429 after retry limit", async () => {
+      const request = new WebResource();
+      const faultyPolicy = new RetryFirstNRequestsPolicy(defaultResponse, 4);
+      const policy = new ThrottlingRetryPolicy(faultyPolicy, new RequestPolicyOptions(), 3);
+      const spy = sinon.spy(policy as any, "retry");
+
+      const response = await policy.sendRequest(request);
+      assert.deepEqual(response.status, 429);
+      assert.strictEqual(spy.callCount, 4); // last retry returns directly after reaching retry limit
+    }).timeout(5000);
   });
 
   describe("parseRetryAfterHeader", () => {