From 9ba0091d311feca955e3ff7c10788064fe5aafc2 Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Mon, 29 Apr 2019 17:06:17 -0700 Subject: [PATCH 1/5] Framework for automatic HTTP retries --- src/utils/api-request.ts | 144 +++++++++- src/utils/error.ts | 1 + test/unit/utils/api-request.spec.ts | 403 +++++++++++++++++++++++++++- 3 files changed, 543 insertions(+), 5 deletions(-) diff --git a/src/utils/api-request.ts b/src/utils/api-request.ts index c53aefcab6..cbf88aeab6 100644 --- a/src/utils/api-request.ts +++ b/src/utils/api-request.ts @@ -79,6 +79,14 @@ interface LowLevelError extends Error { response?: LowLevelResponse; } +export interface RetryConfig { + maxRetries: number; + statusCodes?: number[]; + ioErrorCodes?: string[]; + backOffFactor?: number; + maxDelayInMillis: number; +} + class DefaultHttpResponse implements HttpResponse { public readonly status: number; @@ -166,8 +174,62 @@ export class HttpError extends Error { } } +/** + * Default retry configuration for HTTP requests. + */ +const DEFAULT_RETRY_CONFIG: RetryConfig = { + maxRetries: 1, + ioErrorCodes: ['ECONNRESET', 'ETIMEDOUT'], + maxDelayInMillis: 60 * 1000, +}; + +/** + * Ensures that the given RetryConfig object is valid. + * + * @param retry The configuration to be validated. + */ +function validateRetryConfig(retry: RetryConfig) { + if (!validator.isNumber(retry.maxRetries) || retry.maxRetries < 0) { + throw new FirebaseAppError( + AppErrorCodes.INVALID_ARGUMENT, + 'maxRetries must be a non-negative integer'); + } + + if (typeof retry.backOffFactor !== 'undefined') { + if (!validator.isNumber(retry.backOffFactor) || retry.backOffFactor < 0) { + throw new FirebaseAppError( + AppErrorCodes.INVALID_ARGUMENT, + 'backOffFactor must be a non-negative number'); + } + } + + if (!validator.isNumber(retry.maxDelayInMillis) || retry.maxDelayInMillis < 0) { + throw new FirebaseAppError( + AppErrorCodes.INVALID_ARGUMENT, + 'maxDelayInMillis must be a non-negative number'); + } + + if (typeof retry.statusCodes !== 'undefined' && !validator.isArray(retry.statusCodes)) { + throw new FirebaseAppError( + AppErrorCodes.INVALID_ARGUMENT, + 'statusCodes must be an array'); + } + + if (typeof retry.ioErrorCodes !== 'undefined' && !validator.isArray(retry.ioErrorCodes)) { + throw new FirebaseAppError( + AppErrorCodes.INVALID_ARGUMENT, + 'ioErrorCodes must be an array'); + } +} + export class HttpClient { + constructor(private readonly retry: RetryConfig = DEFAULT_RETRY_CONFIG) { + if (this.retry) { + validateRetryConfig(this.retry); + } + } + /** * Sends an HTTP request to a remote server. If the server responds with a successful response (2xx), the returned * promise resolves with an HttpResponse. If the server responds with an error (3xx, 4xx, 5xx), the promise rejects @@ -193,14 +255,19 @@ export class HttpClient { return AsyncHttpCall.invoke(config) .then((resp) => { return this.createHttpResponse(resp); - }).catch((err: LowLevelError) => { - const retryCodes = ['ECONNRESET', 'ETIMEDOUT']; - if (retryCodes.indexOf(err.code) !== -1 && attempts === 0) { - return this.sendWithRetry(config, attempts + 1); + }) + .catch((err: LowLevelError) => { + const [delayMillis, canRetry] = this.getRetryDelayMillis(attempts, err); + if (canRetry && delayMillis <= this.retry.maxDelayInMillis) { + return this.waitForRetry(delayMillis).then(() => { + return this.sendWithRetry(config, attempts + 1); + }); } + if (err.response) { throw new HttpError(this.createHttpResponse(err.response)); } + if (err.code === 'ETIMEDOUT') { throw new FirebaseAppError( AppErrorCodes.NETWORK_TIMEOUT, @@ -218,6 +285,75 @@ export class HttpClient { } return new DefaultHttpResponse(resp); } + + private getRetryDelayMillis(retryAttempts: number, err: LowLevelError): [number, boolean] { + if (!this.isRetryEligible(retryAttempts, err)) { + return [0, false]; + } + const response = err.response; + if (response && response.headers['retry-after']) { + const delayMillis = this.parseRetryAfterIntoMillis(response.headers['retry-after']); + if (delayMillis > 0) { + return [delayMillis, true]; + } + } + + return [this.backOffDelayMillis(retryAttempts), true]; + } + + private waitForRetry(delayMillis: number): Promise { + if (delayMillis > 0) { + return new Promise((resolve) => { + setTimeout(resolve, delayMillis); + }); + } + return Promise.resolve(); + } + + private isRetryEligible(retryAttempts: number, err: LowLevelError): boolean { + if (!this.retry) { + return false; + } + + if (retryAttempts >= this.retry.maxRetries) { + return false; + } + + if (err.response) { + const statusCodes = this.retry.statusCodes || []; + return statusCodes.indexOf(err.response.status) !== -1; + } + + const retryCodes = this.retry.ioErrorCodes || []; + return retryCodes.indexOf(err.code) !== -1; + } + + /** + * Parses the Retry-After HTTP header as a milliseconds value. Return value is negative if the Retry-After header + * contains an expired timestamp or otherwise malformed. + */ + private parseRetryAfterIntoMillis(retryAfter: string): number { + const delaySeconds: number = parseInt(retryAfter, 10); + if (!isNaN(delaySeconds)) { + return delaySeconds * 1000; + } + + const date = new Date(retryAfter); + if (!isNaN(date.getTime())) { + return date.getTime() - Date.now(); + } + return -1; + } + + private backOffDelayMillis(retryAttempts: number): number { + if (retryAttempts === 0) { + return 0; + } + + const backOffFactor = this.retry.backOffFactor || 0; + const delayInSeconds = (2 ** retryAttempts) * backOffFactor; + return Math.min(delayInSeconds * 1000, this.retry.maxDelayInMillis); + } } /** diff --git a/src/utils/error.ts b/src/utils/error.ts index 2f3fc2d3fe..88e5b72335 100755 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -321,6 +321,7 @@ export class FirebaseProjectManagementError extends PrefixedFirebaseError { export class AppErrorCodes { public static APP_DELETED = 'app-deleted'; public static DUPLICATE_APP = 'duplicate-app'; + public static INVALID_ARGUMENT = 'invalid-argument'; public static INTERNAL_ERROR = 'internal-error'; public static INVALID_APP_NAME = 'invalid-app-name'; public static INVALID_APP_OPTIONS = 'invalid-app-options'; diff --git a/test/unit/utils/api-request.spec.ts b/test/unit/utils/api-request.spec.ts index 6ab5209345..6053dc5cae 100644 --- a/test/unit/utils/api-request.spec.ts +++ b/test/unit/utils/api-request.spec.ts @@ -27,7 +27,8 @@ import * as mocks from '../../resources/mocks'; import {FirebaseApp} from '../../../src/firebase-app'; import { - ApiSettings, HttpClient, HttpError, AuthorizedHttpClient, ApiCallbackFunction, HttpRequestConfig, parseHttpResponse, + ApiSettings, HttpClient, HttpError, AuthorizedHttpClient, ApiCallbackFunction, HttpRequestConfig, + HttpResponse, parseHttpResponse, } from '../../../src/utils/api-request'; import { deepCopy } from '../../../src/utils/deep-copy'; import {Agent} from 'http'; @@ -94,6 +95,7 @@ function mockRequestWithError(err: any) { describe('HttpClient', () => { let mockedRequests: nock.Scope[] = []; let transportSpy: sinon.SinonSpy = null; + let delayStub: sinon.SinonStub = null; const sampleMultipartData = '--boundary\r\n' + 'Content-type: application/json\r\n\r\n' @@ -110,6 +112,18 @@ describe('HttpClient', () => { transportSpy.restore(); transportSpy = null; } + if (delayStub) { + delayStub.restore(); + delayStub = null; + } + }); + + ['string', null, undefined, {}, true, false, NaN, -1].forEach((maxRetries: any) => { + it(`should throw when maxRetries is: ${maxRetries}`, () => { + expect(() => { + new HttpClient({maxRetries} as any); + }).to.throw('maxRetries must be a non-negative integer'); + }); }); it('should be fulfilled for a 2xx response with a json payload', () => { @@ -636,6 +650,393 @@ describe('HttpClient', () => { }); }); + it('should not retry when RetryConfig is not set', () => { + mockedRequests.push(mockRequestWithError({message: 'connection reset 1', code: 'ECONNRESET'})); + const client = new HttpClient(null); + const err = 'Error while making request: connection reset 1'; + return client.send({ + method: 'GET', + url: mockUrl, + }).should.eventually.be.rejectedWith(err).and.have.property('code', 'app/network-error'); + }); + + it('should not retry when maxRetries is set to 0', () => { + mockedRequests.push(mockRequestWithError({message: 'connection reset 1', code: 'ECONNRESET'})); + const client = new HttpClient({ + maxRetries: 0, + ioErrorCodes: ['ECONNRESET'], + maxDelayInMillis: 10000, + }); + const err = 'Error while making request: connection reset 1'; + return client.send({ + method: 'GET', + url: mockUrl, + }).should.eventually.be.rejectedWith(err).and.have.property('code', 'app/network-error'); + }); + + it('should not retry when for error codes that are not configured', () => { + mockedRequests.push(mockRequestWithError({message: 'connection reset 1', code: 'ECONNRESET'})); + const client = new HttpClient({ + maxRetries: 1, + maxDelayInMillis: 10000, + }); + const err = 'Error while making request: connection reset 1'; + return client.send({ + method: 'GET', + url: mockUrl, + }).should.eventually.be.rejectedWith(err).and.have.property('code', 'app/network-error'); + }); + + it('should succeed after a retry on a configured I/O error', () => { + mockedRequests.push(mockRequestWithError({message: 'connection reset 1', code: 'ETESTCODE'})); + const respData = {foo: 'bar'}; + const scope = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope); + const client = new HttpClient({ + maxRetries: 1, + maxDelayInMillis: 1000, + ioErrorCodes: ['ETESTCODE'], + }); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp) => { + expect(resp.status).to.equal(200); + expect(resp.data).to.deep.equal(respData); + }); + }); + + it('should succeed after a retry on a configured HTTP error', () => { + const scope1 = nock('https://' + mockHost) + .get(mockPath) + .reply(503, {}, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope1); + const respData = {foo: 'bar'}; + const scope2 = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope2); + const client = new HttpClient({ + maxRetries: 1, + maxDelayInMillis: 1000, + statusCodes: [503], + }); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp) => { + expect(resp.status).to.equal(200); + expect(resp.data).to.deep.equal(respData); + }); + }); + + it('should not retry more than maxRetries', () => { + // simulate 2 low-level errors + mockedRequests.push(mockRequestWithError({message: 'connection reset 1', code: 'ECONNRESET'})); + mockedRequests.push(mockRequestWithError({message: 'connection reset 2', code: 'ECONNRESET'})); + + // followed by 3 HTTP errors + const scope = nock('https://' + mockHost) + .get(mockPath) + .times(3) + .reply(503, {}, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope); + + const client = new HttpClient({ + maxRetries: 4, + maxDelayInMillis: 10 * 1000, + ioErrorCodes: ['ECONNRESET'], + statusCodes: [503], + }); + return client.send({ + method: 'GET', + url: mockUrl, + }).catch((err: HttpError) => { + expect(err.message).to.equal('Server responded with status 503.'); + const resp = err.response; + expect(resp.status).to.equal(503); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal({}); + expect(resp.isJson()).to.be.true; + }); + }); + + it('should not retry when retry-after exceeds maxDelayInMillis', () => { + const scope = nock('https://' + mockHost) + .get(mockPath) + .reply(503, {}, { + 'content-type': 'application/json', + 'retry-after': '61', + }); + mockedRequests.push(scope); + const client = new HttpClient({ + maxRetries: 1, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + return client.send({ + method: 'GET', + url: mockUrl, + }).catch((err: HttpError) => { + expect(err.message).to.equal('Server responded with status 503.'); + const resp = err.response; + expect(resp.status).to.equal(503); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal({}); + expect(resp.isJson()).to.be.true; + }); + }); + + it('should retry with exponential back off', () => { + const scope = nock('https://' + mockHost) + .get(mockPath) + .times(5) + .reply(503, {}, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope); + const client = new HttpClient({ + maxRetries: 4, + backOffFactor: 0.5, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).catch((err: HttpError) => { + expect(err.message).to.equal('Server responded with status 503.'); + const resp = err.response; + expect(resp.status).to.equal(503); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal({}); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(4); + const delays = delayStub.args.map((args) => args[0]); + expect(delays).to.deep.equal([0, 1000, 2000, 4000]); + }); + }); + + it('delay should not exceed maxDelayInMillis', () => { + const scope = nock('https://' + mockHost) + .get(mockPath) + .times(5) + .reply(503, {}, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope); + const client = new HttpClient({ + maxRetries: 4, + backOffFactor: 1, + maxDelayInMillis: 4 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).catch((err: HttpError) => { + expect(err.message).to.equal('Server responded with status 503.'); + const resp = err.response; + expect(resp.status).to.equal(503); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal({}); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(4); + const delays = delayStub.args.map((args) => args[0]); + expect(delays).to.deep.equal([0, 2000, 4000, 4000]); + }); + }); + + it('should retry without delays when backOffFactor is not set', () => { + const scope = nock('https://' + mockHost) + .get(mockPath) + .times(5) + .reply(503, {}, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope); + const client = new HttpClient({ + maxRetries: 4, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).catch((err: HttpError) => { + expect(err.message).to.equal('Server responded with status 503.'); + const resp = err.response; + expect(resp.status).to.equal(503); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal({}); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(4); + const delays = delayStub.args.map((args) => args[0]); + expect(delays).to.deep.equal([0, 0, 0, 0]); + }); + }); + + it('should wait when retry-after expressed as seconds', () => { + const scope1 = nock('https://' + mockHost) + .get(mockPath) + .reply(503, {}, { + 'content-type': 'application/json', + 'retry-after': '30', + }); + mockedRequests.push(scope1); + const respData = {foo: 'bar'}; + const scope2 = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope2); + + const client = new HttpClient({ + maxRetries: 4, + backOffFactor: 0.5, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp: HttpResponse) => { + expect(resp.status).to.equal(200); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal(respData); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(1); + expect(delayStub.args[0][0]).to.equal(30 * 1000); + }); + }); + + it('should wait when retry-after expressed as a timestamp', () => { + const timestamp = new Date(Date.now() + 30 * 1000); + + const scope1 = nock('https://' + mockHost) + .get(mockPath) + .reply(503, {}, { + 'content-type': 'application/json', + 'retry-after': timestamp.toUTCString(), + }); + mockedRequests.push(scope1); + const respData = {foo: 'bar'}; + const scope2 = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope2); + + const client = new HttpClient({ + maxRetries: 4, + backOffFactor: 0.5, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp: HttpResponse) => { + expect(resp.status).to.equal(200); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal(respData); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(1); + expect(delayStub.args[0][0]).to.be.gt(27 * 1000).and.to.be.lte(30 * 1000); + }); + }); + + it('should not wait when retry-after timestamp is expired', () => { + const timestamp = new Date(Date.now() - 30 * 1000); + + const scope1 = nock('https://' + mockHost) + .get(mockPath) + .reply(503, {}, { + 'content-type': 'application/json', + 'retry-after': timestamp.toUTCString(), + }); + mockedRequests.push(scope1); + const respData = {foo: 'bar'}; + const scope2 = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope2); + + const client = new HttpClient({ + maxRetries: 4, + backOffFactor: 0.5, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp: HttpResponse) => { + expect(resp.status).to.equal(200); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal(respData); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(1); + expect(delayStub.args[0][0]).to.equal(0); + }); + }); + + it('should not wait when retry-after is malformed', () => { + const scope1 = nock('https://' + mockHost) + .get(mockPath) + .reply(503, {}, { + 'content-type': 'application/json', + 'retry-after': 'invalid', + }); + mockedRequests.push(scope1); + const respData = {foo: 'bar'}; + const scope2 = nock('https://' + mockHost) + .get(mockPath) + .reply(200, respData, { + 'content-type': 'application/json', + }); + mockedRequests.push(scope2); + + const client = new HttpClient({ + maxRetries: 4, + backOffFactor: 0.5, + maxDelayInMillis: 60 * 1000, + statusCodes: [503], + }); + delayStub = sinon.stub(client as any, 'waitForRetry').resolves(); + return client.send({ + method: 'GET', + url: mockUrl, + }).then((resp: HttpResponse) => { + expect(resp.status).to.equal(200); + expect(resp.headers['content-type']).to.equal('application/json'); + expect(resp.data).to.deep.equal(respData); + expect(resp.isJson()).to.be.true; + expect(delayStub.callCount).to.equal(1); + expect(delayStub.args[0][0]).to.equal(0); + }); + }); + it('should reject if the request payload is invalid', () => { const client = new HttpClient(); const err = 'Error while making request: Request data must be a string, a Buffer ' From 6dc34c71bc8f3b9e8430426cd9ee0cb20c71f79d Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Tue, 30 Apr 2019 11:10:22 -0700 Subject: [PATCH 2/5] Added docs and more tests --- src/utils/api-request.ts | 71 +++++++++++++++-------------- test/unit/utils/api-request.spec.ts | 39 +++++++++++++++- 2 files changed, 74 insertions(+), 36 deletions(-) diff --git a/src/utils/api-request.ts b/src/utils/api-request.ts index cbf88aeab6..f3be0b1408 100644 --- a/src/utils/api-request.ts +++ b/src/utils/api-request.ts @@ -79,14 +79,6 @@ interface LowLevelError extends Error { response?: LowLevelResponse; } -export interface RetryConfig { - maxRetries: number; - statusCodes?: number[]; - ioErrorCodes?: string[]; - backOffFactor?: number; - maxDelayInMillis: number; -} - class DefaultHttpResponse implements HttpResponse { public readonly status: number; @@ -175,7 +167,18 @@ export class HttpError extends Error { } /** - * Default retry configuration for HTTP requests. + * Specifies how failing HTTP requests should be retried. + */ +export interface RetryConfig { + maxRetries: number; + statusCodes?: number[]; + ioErrorCodes?: string[]; + backOffFactor?: number; + maxDelayInMillis: number; +} + +/** + * Default retry configuration for HTTP requests. Retries once on connection reset and timeout errors. */ const DEFAULT_RETRY_CONFIG: RetryConfig = { maxRetries: 1, @@ -191,34 +194,27 @@ const DEFAULT_RETRY_CONFIG: RetryConfig = { function validateRetryConfig(retry: RetryConfig) { if (!validator.isNumber(retry.maxRetries) || retry.maxRetries < 0) { throw new FirebaseAppError( - AppErrorCodes.INVALID_ARGUMENT, - 'maxRetries must be a non-negative integer'); + AppErrorCodes.INVALID_ARGUMENT, 'maxRetries must be a non-negative integer'); } if (typeof retry.backOffFactor !== 'undefined') { if (!validator.isNumber(retry.backOffFactor) || retry.backOffFactor < 0) { throw new FirebaseAppError( - AppErrorCodes.INVALID_ARGUMENT, - 'backOffFactor must be a non-negative number'); + AppErrorCodes.INVALID_ARGUMENT, 'backOffFactor must be a non-negative number'); } } if (!validator.isNumber(retry.maxDelayInMillis) || retry.maxDelayInMillis < 0) { throw new FirebaseAppError( - AppErrorCodes.INVALID_ARGUMENT, - 'maxDelayInMillis must be a non-negative number'); + AppErrorCodes.INVALID_ARGUMENT, 'maxDelayInMillis must be a non-negative integer'); } if (typeof retry.statusCodes !== 'undefined' && !validator.isArray(retry.statusCodes)) { - throw new FirebaseAppError( - AppErrorCodes.INVALID_ARGUMENT, - 'statusCodes must be an array'); + throw new FirebaseAppError(AppErrorCodes.INVALID_ARGUMENT, 'statusCodes must be an array'); } if (typeof retry.ioErrorCodes !== 'undefined' && !validator.isArray(retry.ioErrorCodes)) { - throw new FirebaseAppError( - AppErrorCodes.INVALID_ARGUMENT, - 'ioErrorCodes must be an array'); + throw new FirebaseAppError(AppErrorCodes.INVALID_ARGUMENT, 'ioErrorCodes must be an array'); } } @@ -241,7 +237,7 @@ export class HttpClient { * header should be explicitly set by the caller. To send a JSON leaf value (e.g. "foo", 5), parse it into JSON, * and pass as a string or a Buffer along with the appropriate content-type header. * - * @param {HttpRequest} request HTTP request to be sent. + * @param {HttpRequest} config HTTP request to be sent. * @return {Promise} A promise that resolves with the response details. */ public send(config: HttpRequestConfig): Promise { @@ -249,18 +245,23 @@ export class HttpClient { } /** - * Sends an HTTP request, and retries it once in case of low-level network errors. + * Sends an HTTP request. In the event of an error, retries the HTTP request according to the + * RetryConfig set on the HttpClient. + * + * @param {HttpRequestConfig} config HTTP request to be sent. + * @param {number} retryAttempts Number of retries performed up to now. + * @return {Promise} A promise that resolves with the response details. */ - private sendWithRetry(config: HttpRequestConfig, attempts: number = 0): Promise { + private sendWithRetry(config: HttpRequestConfig, retryAttempts: number = 0): Promise { return AsyncHttpCall.invoke(config) .then((resp) => { return this.createHttpResponse(resp); }) .catch((err: LowLevelError) => { - const [delayMillis, canRetry] = this.getRetryDelayMillis(attempts, err); + const [delayMillis, canRetry] = this.getRetryDelayMillis(retryAttempts, err); if (canRetry && delayMillis <= this.retry.maxDelayInMillis) { return this.waitForRetry(delayMillis).then(() => { - return this.sendWithRetry(config, attempts + 1); + return this.sendWithRetry(config, retryAttempts + 1); }); } @@ -286,6 +287,15 @@ export class HttpClient { return new DefaultHttpResponse(resp); } + private waitForRetry(delayMillis: number): Promise { + if (delayMillis > 0) { + return new Promise((resolve) => { + setTimeout(resolve, delayMillis); + }); + } + return Promise.resolve(); + } + private getRetryDelayMillis(retryAttempts: number, err: LowLevelError): [number, boolean] { if (!this.isRetryEligible(retryAttempts, err)) { return [0, false]; @@ -301,15 +311,6 @@ export class HttpClient { return [this.backOffDelayMillis(retryAttempts), true]; } - private waitForRetry(delayMillis: number): Promise { - if (delayMillis > 0) { - return new Promise((resolve) => { - setTimeout(resolve, delayMillis); - }); - } - return Promise.resolve(); - } - private isRetryEligible(retryAttempts: number, err: LowLevelError): boolean { if (!this.retry) { return false; diff --git a/test/unit/utils/api-request.spec.ts b/test/unit/utils/api-request.spec.ts index 6053dc5cae..28599749a3 100644 --- a/test/unit/utils/api-request.spec.ts +++ b/test/unit/utils/api-request.spec.ts @@ -118,7 +118,10 @@ describe('HttpClient', () => { } }); - ['string', null, undefined, {}, true, false, NaN, -1].forEach((maxRetries: any) => { + const invalidNumbers: any[] = ['string', null, undefined, {}, [], true, false, NaN, -1]; + const invalidArrays: any[] = ['string', null, {}, true, false, NaN, 0, 1]; + + invalidNumbers.forEach((maxRetries: any) => { it(`should throw when maxRetries is: ${maxRetries}`, () => { expect(() => { new HttpClient({maxRetries} as any); @@ -126,6 +129,40 @@ describe('HttpClient', () => { }); }); + invalidNumbers.forEach((backOffFactor: any) => { + if (typeof backOffFactor !== 'undefined') { + it(`should throw when backOffFactor is: ${backOffFactor}`, () => { + expect(() => { + new HttpClient({maxRetries: 1, backOffFactor} as any); + }).to.throw('backOffFactor must be a non-negative number'); + }); + } + }); + + invalidNumbers.forEach((maxDelayInMillis: any) => { + it(`should throw when maxDelayInMillis is: ${maxDelayInMillis}`, () => { + expect(() => { + new HttpClient({maxRetries: 1, maxDelayInMillis} as any); + }).to.throw('maxDelayInMillis must be a non-negative integer'); + }); + }); + + invalidArrays.forEach((ioErrorCodes: any) => { + it(`should throw when ioErrorCodes is: ${ioErrorCodes}`, () => { + expect(() => { + new HttpClient({maxRetries: 1, maxDelayInMillis: 10000, ioErrorCodes} as any); + }).to.throw('ioErrorCodes must be an array'); + }); + }); + + invalidArrays.forEach((statusCodes: any) => { + it(`should throw when statusCodes is: ${statusCodes}`, () => { + expect(() => { + new HttpClient({maxRetries: 1, maxDelayInMillis: 10000, statusCodes} as any); + }).to.throw('statusCodes must be an array'); + }); + }); + it('should be fulfilled for a 2xx response with a json payload', () => { const respData = {foo: 'bar'}; const scope = nock('https://' + mockHost) From 5494181c611cd5c5cb426e9c9b0630cdc88dfcbc Mon Sep 17 00:00:00 2001 From: Hiranya Jayathilaka Date: Tue, 21 May 2019 13:40:09 -0700 Subject: [PATCH 3/5] Updated documentation; Improved a clock-based test using fake timers --- src/utils/api-request.ts | 23 +++++++++++++++++++++++ test/unit/utils/api-request.spec.ts | 13 ++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/src/utils/api-request.ts b/src/utils/api-request.ts index f3be0b1408..27dacef9d3 100644 --- a/src/utils/api-request.ts +++ b/src/utils/api-request.ts @@ -170,10 +170,23 @@ export class HttpError extends Error { * Specifies how failing HTTP requests should be retried. */ export interface RetryConfig { + /** Maximum number of times to retry a given request. */ maxRetries: number; + + /** HTTP status codes taht should be retried. */ statusCodes?: number[]; + + /** Low-level I/O error codes that should be retried. */ ioErrorCodes?: string[]; + + /** + * The multiplier for exponential back off. The retry delay is calculated using the formula `(2^n) * backOffFactor`, + * where n is the number of retries performed so far. When the backOffFactor is set to 0 retries are not delayed. + * When the backOffFactor is 1, retry duration is doubled each iteration. + */ backOffFactor?: number; + + /** Maximum duration to wait before initiating a retry. */ maxDelayInMillis: number; } @@ -296,10 +309,20 @@ export class HttpClient { return Promise.resolve(); } + /** + * Checks if a failed request is eligible for a retry, and if so returns the duration to wait before initiating + * the retry. + * + * @param {number} retryAttempts Number of retries completed up to now. + * @param {LowLevelError} err The last encountered error. + * @returns {[number, boolean]} A 2-tuple where the 1st element is the duration to wait before another retry, and the + * 2nd element is a boolean indicating whether the request is eligible for a retry or not. + */ private getRetryDelayMillis(retryAttempts: number, err: LowLevelError): [number, boolean] { if (!this.isRetryEligible(retryAttempts, err)) { return [0, false]; } + const response = err.response; if (response && response.headers['retry-after']) { const delayMillis = this.parseRetryAfterIntoMillis(response.headers['retry-after']); diff --git a/test/unit/utils/api-request.spec.ts b/test/unit/utils/api-request.spec.ts index 28599749a3..503137a5f4 100644 --- a/test/unit/utils/api-request.spec.ts +++ b/test/unit/utils/api-request.spec.ts @@ -96,6 +96,7 @@ describe('HttpClient', () => { let mockedRequests: nock.Scope[] = []; let transportSpy: sinon.SinonSpy = null; let delayStub: sinon.SinonStub = null; + let clock: sinon.SinonFakeTimers = null; const sampleMultipartData = '--boundary\r\n' + 'Content-type: application/json\r\n\r\n' @@ -116,6 +117,10 @@ describe('HttpClient', () => { delayStub.restore(); delayStub = null; } + if (clock) { + clock.restore(); + clock = null; + } }); const invalidNumbers: any[] = ['string', null, undefined, {}, [], true, false, NaN, -1]; @@ -711,7 +716,7 @@ describe('HttpClient', () => { }).should.eventually.be.rejectedWith(err).and.have.property('code', 'app/network-error'); }); - it('should not retry when for error codes that are not configured', () => { + it('should not retry when error codes are not configured', () => { mockedRequests.push(mockRequestWithError({message: 'connection reset 1', code: 'ECONNRESET'})); const client = new HttpClient({ maxRetries: 1, @@ -963,7 +968,9 @@ describe('HttpClient', () => { }); it('should wait when retry-after expressed as a timestamp', () => { - const timestamp = new Date(Date.now() + 30 * 1000); + clock = sinon.useFakeTimers(); + clock.setSystemTime(1000); + const timestamp = new Date(clock.now + 30 * 1000); const scope1 = nock('https://' + mockHost) .get(mockPath) @@ -996,7 +1003,7 @@ describe('HttpClient', () => { expect(resp.data).to.deep.equal(respData); expect(resp.isJson()).to.be.true; expect(delayStub.callCount).to.equal(1); - expect(delayStub.args[0][0]).to.be.gt(27 * 1000).and.to.be.lte(30 * 1000); + expect(delayStub.args[0][0]).to.equal(30 * 1000); }); }); From d53a9aec588d99c5c2d4fb272c4e9be0cb1a5538 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Wed, 22 May 2019 11:33:16 -0700 Subject: [PATCH 4/5] Fixed a typo; Updated comment --- src/utils/api-request.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/utils/api-request.ts b/src/utils/api-request.ts index 27dacef9d3..06fc476b1a 100644 --- a/src/utils/api-request.ts +++ b/src/utils/api-request.ts @@ -173,16 +173,16 @@ export interface RetryConfig { /** Maximum number of times to retry a given request. */ maxRetries: number; - /** HTTP status codes taht should be retried. */ + /** HTTP status codes that should be retried. */ statusCodes?: number[]; /** Low-level I/O error codes that should be retried. */ ioErrorCodes?: string[]; /** - * The multiplier for exponential back off. The retry delay is calculated using the formula `(2^n) * backOffFactor`, - * where n is the number of retries performed so far. When the backOffFactor is set to 0 retries are not delayed. - * When the backOffFactor is 1, retry duration is doubled each iteration. + * The multiplier for exponential back off. The retry delay is calculated in seconds using the formula + * `(2^n) * backOffFactor`, where n is the number of retries performed so far. When the backOffFactor is set + * to 0, retries are not delayed. When the backOffFactor is 1, retry duration is doubled each iteration. */ backOffFactor?: number; From c9c836c47e37595418e2f8a8da89a47efcfde9a7 Mon Sep 17 00:00:00 2001 From: hiranya911 Date: Wed, 22 May 2019 11:39:04 -0700 Subject: [PATCH 5/5] Trigger builds