From 1cadbff7c4c0988f40d5bae018c503bace5b9868 Mon Sep 17 00:00:00 2001 From: Chukwuemeka Ajima Date: Thu, 10 Feb 2022 12:18:00 +0100 Subject: [PATCH 1/5] - add retryCodes option to HttpMiddlewareOptions - add flow types - add documentation - add unit test --- docs/sdk/api/sdkMiddlewareHttp.md | 4 +- packages/sdk-middleware-http/src/http.js | 37 +++++--- .../sdk-middleware-http/test/http.spec.js | 91 +++++++++++++++++++ types/sdk.js | 1 + 4 files changed, 118 insertions(+), 15 deletions(-) diff --git a/docs/sdk/api/sdkMiddlewareHttp.md b/docs/sdk/api/sdkMiddlewareHttp.md index 411a724ee..75198404e 100644 --- a/docs/sdk/api/sdkMiddlewareHttp.md +++ b/docs/sdk/api/sdkMiddlewareHttp.md @@ -33,7 +33,7 @@ The HTTP middleware can run in either a browser or Node.js environment. For Node 4. `includeOriginalRequest` _(Boolean)_: flag whether to include the original request sent in the response. Can be useful if you want to see the final request being sent. 5. `maskSensitiveHeaderData` _(Boolean)_: flag to mask sensitie data in the header. e.g. Authorization token 6. `enableRetry` _(Boolean)_: flag to enable retry on network errors and `500` response. (Default: false) -7. `retryConfig` _(Object)_: Field required in the object listed below +7. `retryCodes` - _(Array)_: array of numbers used to retry requests when the status (error) code matches an entry in the list. 8. `maxRetries` _(Number)_: number of times to retry the request before failing the request. (Default: 50) 9. `retryDelay` _(Number)_: amount of milliseconds to wait before retrying the next request. (Default: 200) 10. `backoff` _(Boolean)_: activates exponential backoff. Recommended to prevent spamming of the server. (Default: true) @@ -42,6 +42,7 @@ The HTTP middleware can run in either a browser or Node.js environment. For Node 13. `fetch` _(Function)_: A `fetch` implementation which can be e.g. `node-fetch` or `unfetch` but also the native browser `fetch` function 14. `timeout` _(Number)_: Request/response timeout in ms. Must be globally available or passed in `AbortController` 15. `abortController` or `getAbortController` depending on what you chose to handle the timeout (_abortController_): This property accepts the `AbortController` instance. Could be [abort-controller](https://www.npmjs.com/package/abort-controller) or a globally available one. +16. `retryConfig` _(Object)_: Field required in the object listed below #### Retrying requests @@ -67,6 +68,7 @@ const client = createClient({ includeOriginalRequest: true, maskSensitiveHeaderData: true, enableRetry: true, + retryCodes: [500, 504], retryConfig: { maxRetries: 2, retryDelay: 300, //milliseconds diff --git a/packages/sdk-middleware-http/src/http.js b/packages/sdk-middleware-http/src/http.js index 76888d4c2..76bb1161b 100644 --- a/packages/sdk-middleware-http/src/http.js +++ b/packages/sdk-middleware-http/src/http.js @@ -35,9 +35,9 @@ function calcDelayDuration( if (backoff) return retryCount !== 0 // do not increase if it's the first retry ? Math.min( - Math.round((Math.random() + 1) * retryDelay * 2 ** retryCount), - maxDelay - ) + Math.round((Math.random() + 1) * retryDelay * 2 ** retryCount), + maxDelay + ) : retryDelay return retryDelay } @@ -59,6 +59,7 @@ export default function createHttpMiddleware({ maskSensitiveHeaderData = true, enableRetry, timeout, + retryCodes = [], retryConfig: { // encourage exponential backoff to prevent spamming the server if down maxRetries = 10, @@ -114,9 +115,9 @@ export default function createHttpMiddleware({ // Ensure body is a string if content type is application/json const body = requestHeader['Content-Type'] === 'application/json' && - typeof request.body !== 'string' + typeof request.body !== 'string' ? // NOTE: `stringify` of `null` gives the String('null') - JSON.stringify(request.body || undefined) + JSON.stringify(request.body || undefined) : request.body if (body && (typeof body === 'string' || Buffer.isBuffer(body))) { @@ -132,15 +133,16 @@ export default function createHttpMiddleware({ } if (!retryOnAbort) { - if (timeout || getAbortController || _abortController) + if (timeout || getAbortController || _abortController) { // eslint-disable-next-line - abortController = + abortController = (getAbortController ? getAbortController() : null) || _abortController || new AbortController() - if (abortController) { - fetchOptions.signal = abortController.signal + if (abortController) { + fetchOptions.signal = abortController.signal + } } } @@ -151,16 +153,18 @@ export default function createHttpMiddleware({ // wrap in a fn so we can retry if error occur function executeFetch() { if (retryOnAbort) { - if (timeout || getAbortController || _abortController) + if (timeout || getAbortController || _abortController) { // eslint-disable-next-line - abortController = + abortController = (getAbortController ? getAbortController() : null) || _abortController || new AbortController() - if (abortController) { - fetchOptions.signal = abortController.signal + if (abortController) { + fetchOptions.signal = abortController.signal + } } + } // Kick off timer for abortController directly before fetch. let timer @@ -225,7 +229,11 @@ export default function createHttpMiddleware({ }) return } - if (res.status === 503 && enableRetry) + + if ( + enableRetry && + (res.status === 503 || retryCodes.includes(res.status)) + ) { if (retryCount < maxRetries) { setTimeout( executeFetch, @@ -240,6 +248,7 @@ export default function createHttpMiddleware({ retryCount += 1 return } + } // Server responded with an error. Try to parse it as JSON, then // return a proper error type with all necessary meta information. diff --git a/packages/sdk-middleware-http/test/http.spec.js b/packages/sdk-middleware-http/test/http.spec.js index a3d813077..c484a620b 100644 --- a/packages/sdk-middleware-http/test/http.spec.js +++ b/packages/sdk-middleware-http/test/http.spec.js @@ -677,6 +677,97 @@ describe('Http', () => { httpMiddleware(next)(request, response) })) + test('should retry when status (error) code is part of retryCodes', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + expect(res.error.name).toBe('InternalServerError') + expect(res.error.originalRequest).toBeDefined() + expect(res.body).toBeUndefined() + expect(res.statusCode).toBe(500) + expect(res.error.retryCount).toBe(2) + resolve() + } + const options = { + host: testHost, + enableRetry: true, + retryCodes: [500, 501, 502], + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + } + const httpMiddleware = createHttpMiddleware(options) + nock(testHost).get('/foo/bar').times(3).reply(500) + + httpMiddleware(next)(request, response) + })) + + test('should not retry when status (error) code is not part of retryCodes', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + expect(res.error.name).toBe('InternalServerError') + expect(res.error.originalRequest).toBeDefined() + expect(res.body).toBeUndefined() + expect(res.statusCode).toBe(500) + expect(res.error.retryCount).toBe(0) + resolve() + } + const options = { + host: testHost, + enableRetry: true, + retryCodes: [501, 502], + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + } + const httpMiddleware = createHttpMiddleware(options) + nock(testHost).get('/foo/bar').times(3).reply(500) + + httpMiddleware(next)(request, response) + })) + + test('should not retry when enableRetry is set to false ', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + expect(res.error.name).toBe('InternalServerError') + expect(res.error.originalRequest).toBeDefined() + expect(res.body).toBeUndefined() + expect(res.statusCode).toBe(500) + expect(res.error.retryCount).toBe(0) + resolve() + } + const options = { + host: testHost, + enableRetry: false, + retryCodes: [500, 502], + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + } + const httpMiddleware = createHttpMiddleware(options) + nock(testHost).get('/foo/bar').times(3).reply(500) + + httpMiddleware(next)(request, response) + })) + + test( 'should toggle `exponential backoff` off', () => diff --git a/types/sdk.js b/types/sdk.js index b2cfd2c0a..c467404e7 100644 --- a/types/sdk.js +++ b/types/sdk.js @@ -246,6 +246,7 @@ export type HttpMiddlewareOptions = { maskSensitiveHeaderData?: boolean, timeout?: number, enableRetry?: boolean, + retryCodes?: Array, retryConfig?: { maxRetries?: number, retryDelay?: number, From b7c4a591fed193aa5dd332fdaec77c71f48e269a Mon Sep 17 00:00:00 2001 From: Chukwuemeka Ajima Date: Mon, 14 Feb 2022 12:30:44 +0100 Subject: [PATCH 2/5] feat(http-error-retryCodes): improvement - improve and refactor code - add check to ensure retryCodes is not null --- packages/sdk-middleware-http/src/http.js | 8 +++++++- packages/sdk-middleware-http/test/http.spec.js | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/sdk-middleware-http/src/http.js b/packages/sdk-middleware-http/src/http.js index 76bb1161b..827b90086 100644 --- a/packages/sdk-middleware-http/src/http.js +++ b/packages/sdk-middleware-http/src/http.js @@ -95,6 +95,12 @@ export default function createHttpMiddleware({ fetchFunction = fetch } + if (!Array.isArray(retryCodes)) { + throw new Error( + '`retryCodes` option must be an array of retry status (error) codes.' + ) + } + return (next: Next): Next => (request: MiddlewareRequest, response: MiddlewareResponse) => { let abortController: any @@ -232,7 +238,7 @@ export default function createHttpMiddleware({ if ( enableRetry && - (res.status === 503 || retryCodes.includes(res.status)) + (res.status === 503 || (retryCodes?.indexOf(res.status) !== -1)) ) { if (retryCount < maxRetries) { setTimeout( diff --git a/packages/sdk-middleware-http/test/http.spec.js b/packages/sdk-middleware-http/test/http.spec.js index c484a620b..a5e40b539 100644 --- a/packages/sdk-middleware-http/test/http.spec.js +++ b/packages/sdk-middleware-http/test/http.spec.js @@ -45,6 +45,16 @@ describe('Http', () => { ) }) + test('throw when a non-array option is passed as retryCodes in the httpMiddlewareOptions', () => { + expect(() => { + createHttpMiddleware({ host: testHost, retryCodes: null, fetch }) + }).toThrow( + new Error( + '`retryCodes` option must be an array of retry status (error) codes.' + ) + ) + }) + test('execute a get request (success)', () => new Promise((resolve, reject) => { const request = createTestRequest({ From 0aa84a734210cbf40deab44021d2daa8ec5bd7b6 Mon Sep 17 00:00:00 2001 From: Chukwuemeka Ajima Date: Wed, 16 Feb 2022 18:14:02 +0100 Subject: [PATCH 3/5] feat(http-error-retryCodes): improvements - improve the feature to include status messages - add tests to assert this functionality - rewrite documentation --- docs/sdk/api/sdkMiddlewareHttp.md | 4 +- packages/sdk-middleware-http/src/http.js | 42 +++---- .../sdk-middleware-http/test/http.spec.js | 103 ++++++++++++++++++ types/sdk.js | 2 +- 4 files changed, 128 insertions(+), 23 deletions(-) diff --git a/docs/sdk/api/sdkMiddlewareHttp.md b/docs/sdk/api/sdkMiddlewareHttp.md index 75198404e..eb43b49bd 100644 --- a/docs/sdk/api/sdkMiddlewareHttp.md +++ b/docs/sdk/api/sdkMiddlewareHttp.md @@ -33,7 +33,7 @@ The HTTP middleware can run in either a browser or Node.js environment. For Node 4. `includeOriginalRequest` _(Boolean)_: flag whether to include the original request sent in the response. Can be useful if you want to see the final request being sent. 5. `maskSensitiveHeaderData` _(Boolean)_: flag to mask sensitie data in the header. e.g. Authorization token 6. `enableRetry` _(Boolean)_: flag to enable retry on network errors and `500` response. (Default: false) -7. `retryCodes` - _(Array)_: array of numbers used to retry requests when the status (error) code matches an entry in the list. +7. `retryCodes` - _(Array)_: array of `statusCodes` [`numbers`] and error code or messages [`string`] for retring requests when the statusCodes and/or error message/code matches an entry in the list. 8. `maxRetries` _(Number)_: number of times to retry the request before failing the request. (Default: 50) 9. `retryDelay` _(Number)_: amount of milliseconds to wait before retrying the next request. (Default: 200) 10. `backoff` _(Boolean)_: activates exponential backoff. Recommended to prevent spamming of the server. (Default: true) @@ -68,7 +68,7 @@ const client = createClient({ includeOriginalRequest: true, maskSensitiveHeaderData: true, enableRetry: true, - retryCodes: [500, 504], + retryCodes: [504, 'ETIMEDOUT', 'ECONNREFUSED', 503], retryConfig: { maxRetries: 2, retryDelay: 300, //milliseconds diff --git a/packages/sdk-middleware-http/src/http.js b/packages/sdk-middleware-http/src/http.js index 827b90086..0a9dab9ea 100644 --- a/packages/sdk-middleware-http/src/http.js +++ b/packages/sdk-middleware-http/src/http.js @@ -236,26 +236,6 @@ export default function createHttpMiddleware({ return } - if ( - enableRetry && - (res.status === 503 || (retryCodes?.indexOf(res.status) !== -1)) - ) { - if (retryCount < maxRetries) { - setTimeout( - executeFetch, - calcDelayDuration( - retryCount, - retryDelay, - maxRetries, - backoff, - maxDelay - ) - ) - retryCount += 1 - return - } - } - // Server responded with an error. Try to parse it as JSON, then // return a proper error type with all necessary meta information. res.text().then((text: any) => { @@ -276,6 +256,28 @@ export default function createHttpMiddleware({ ? { message: parsed.message, body: parsed } : { message: parsed, body: parsed }), }) + + if ( + enableRetry && + ([503, ...retryCodes].indexOf(error.statusCode) !== -1 || + retryCodes?.indexOf(error.message) !== -1) + ) { + if (retryCount < maxRetries) { + setTimeout( + executeFetch, + calcDelayDuration( + retryCount, + retryDelay, + maxRetries, + backoff, + maxDelay + ) + ) + retryCount += 1 + return + } + } + maskAuthData(error.originalRequest, maskSensitiveHeaderData) // Let the final resolver to reject the promise const parsedResponse = { diff --git a/packages/sdk-middleware-http/test/http.spec.js b/packages/sdk-middleware-http/test/http.spec.js index a5e40b539..4c0b523cb 100644 --- a/packages/sdk-middleware-http/test/http.spec.js +++ b/packages/sdk-middleware-http/test/http.spec.js @@ -717,6 +717,109 @@ describe('Http', () => { httpMiddleware(next)(request, response) })) + test('should retry when status (error) message is part of retryCodes', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + expect(res.error.name).toBe('InternalServerError') + expect(res.error.message).toBe('ETIMEDOUT') + expect(res.error.retryCount).toBe(2) + expect(res.error.statusCode).toBe(500) + resolve() + } + + const httpMiddleware = createHttpMiddleware({ + host: testHost, + enableRetry: true, + retryCodes: ['ETIMEDOUT', 'ECONNREFUSED', 'write EPIPE'], + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + }) + nock(testHost) + .defaultReplyHeaders({ + 'Content-Type': 'application/json' + }) + .persist() + .get('/foo/bar') + .reply(500, 'ETIMEDOUT') + + httpMiddleware(next)(request, response) + })) + + test('should retry when status (error) code and message are both part of retryCodes', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + expect(res.error.name).toBe('HttpError') + expect(res.error.message).toBe('ETIMEDOUT') + expect(res.error.retryCount).toBe(2) + expect(res.error.statusCode).toBe(502) + resolve() + } + + const httpMiddleware = createHttpMiddleware({ + host: testHost, + enableRetry: true, + retryCodes: ['ETIMEDOUT', 503, 502, 'ECONNREFUSED', 'write EPIPE'], + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + }) + nock(testHost) + .defaultReplyHeaders({ + 'Content-Type': 'application/json' + }) + .persist() + .get('/foo/bar') + .reply(502, 'ETIMEDOUT') + + httpMiddleware(next)(request, response) + })) + + test('should not retry when status (error) message is not part of retryCodes', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + expect(res.error.name).toBe('InternalServerError') + expect(res.error.originalRequest).toBeDefined() + expect(res.body).toBeUndefined() + expect(res.statusCode).toBe(500) + expect(res.error.retryCount).toBe(0) + resolve() + } + const options = { + host: testHost, + enableRetry: true, + retryCodes: ['Not Included'], + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + } + const httpMiddleware = createHttpMiddleware(options) + nock(testHost) + .get('/foo/bar') + .times(3) + .reply(500, 'Internal Server Error') + + httpMiddleware(next)(request, response) + })) + test('should not retry when status (error) code is not part of retryCodes', () => new Promise((resolve, reject) => { const request = createTestRequest({ diff --git a/types/sdk.js b/types/sdk.js index c467404e7..b661ed1c9 100644 --- a/types/sdk.js +++ b/types/sdk.js @@ -246,7 +246,7 @@ export type HttpMiddlewareOptions = { maskSensitiveHeaderData?: boolean, timeout?: number, enableRetry?: boolean, - retryCodes?: Array, + retryCodes?: Array, retryConfig?: { maxRetries?: number, retryDelay?: number, From 3205631d5772ad4e7c1d4c13879d51d5f5fb34ea Mon Sep 17 00:00:00 2001 From: Chukwuemeka Ajima Date: Fri, 18 Feb 2022 16:07:53 +0100 Subject: [PATCH 4/5] chore(retryConfig): retryCodes - move retryCodes into retryConfig options --- docs/sdk/api/sdkMiddlewareHttp.md | 7 ++++++- packages/sdk-middleware-http/src/http.js | 2 +- packages/sdk-middleware-http/test/http.spec.js | 14 ++++++++++---- types/sdk.js | 4 ++-- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/docs/sdk/api/sdkMiddlewareHttp.md b/docs/sdk/api/sdkMiddlewareHttp.md index eb43b49bd..3e323b61e 100644 --- a/docs/sdk/api/sdkMiddlewareHttp.md +++ b/docs/sdk/api/sdkMiddlewareHttp.md @@ -68,12 +68,17 @@ const client = createClient({ includeOriginalRequest: true, maskSensitiveHeaderData: true, enableRetry: true, - retryCodes: [504, 'ETIMEDOUT', 'ECONNREFUSED', 503], retryConfig: { maxRetries: 2, retryDelay: 300, //milliseconds maxDelay: 5000, //milliseconds retryOnAbort: false, + retryCodes: [ + 504, + 'ETIMEDOUT', + 'ECONNREFUSED', + 503 + ] }, // Optional if not globally available diff --git a/packages/sdk-middleware-http/src/http.js b/packages/sdk-middleware-http/src/http.js index 0a9dab9ea..43b945ada 100644 --- a/packages/sdk-middleware-http/src/http.js +++ b/packages/sdk-middleware-http/src/http.js @@ -59,7 +59,6 @@ export default function createHttpMiddleware({ maskSensitiveHeaderData = true, enableRetry, timeout, - retryCodes = [], retryConfig: { // encourage exponential backoff to prevent spamming the server if down maxRetries = 10, @@ -67,6 +66,7 @@ export default function createHttpMiddleware({ retryDelay = 200, maxDelay = Infinity, retryOnAbort = false, + retryCodes = [], } = {}, fetch: fetcher, abortController: _abortController, diff --git a/packages/sdk-middleware-http/test/http.spec.js b/packages/sdk-middleware-http/test/http.spec.js index 4c0b523cb..ea94f250e 100644 --- a/packages/sdk-middleware-http/test/http.spec.js +++ b/packages/sdk-middleware-http/test/http.spec.js @@ -47,7 +47,7 @@ describe('Http', () => { test('throw when a non-array option is passed as retryCodes in the httpMiddlewareOptions', () => { expect(() => { - createHttpMiddleware({ host: testHost, retryCodes: null, fetch }) + createHttpMiddleware({ host: testHost, retryConfig: { retryCodes: null }, fetch }) }).toThrow( new Error( '`retryCodes` option must be an array of retry status (error) codes.' @@ -704,10 +704,12 @@ describe('Http', () => { const options = { host: testHost, enableRetry: true, - retryCodes: [500, 501, 502], retryConfig: { maxRetries: 2, retryDelay: 300, + retryCodes: [ + 500, 501, 502 + ], }, fetch, } @@ -734,10 +736,12 @@ describe('Http', () => { const httpMiddleware = createHttpMiddleware({ host: testHost, enableRetry: true, - retryCodes: ['ETIMEDOUT', 'ECONNREFUSED', 'write EPIPE'], retryConfig: { maxRetries: 2, retryDelay: 300, + retryCodes: [ + 'ETIMEDOUT', 'ECONNREFUSED', 'write EPIPE' + ] }, fetch, }) @@ -769,10 +773,12 @@ describe('Http', () => { const httpMiddleware = createHttpMiddleware({ host: testHost, enableRetry: true, - retryCodes: ['ETIMEDOUT', 503, 502, 'ECONNREFUSED', 'write EPIPE'], retryConfig: { maxRetries: 2, retryDelay: 300, + retryCodes: [ + 'ETIMEDOUT', 503, 502, 'ECONNREFUSED', 'write EPIPE' + ] }, fetch, }) diff --git a/types/sdk.js b/types/sdk.js index b661ed1c9..958bb5d4a 100644 --- a/types/sdk.js +++ b/types/sdk.js @@ -246,13 +246,13 @@ export type HttpMiddlewareOptions = { maskSensitiveHeaderData?: boolean, timeout?: number, enableRetry?: boolean, - retryCodes?: Array, retryConfig?: { maxRetries?: number, retryDelay?: number, backoff?: boolean, maxDelay?: number, - retryOnAbort: boolean + retryOnAbort: boolean, + retryCodes?: Array, }, fetch?: typeof fetch, /** From 8f3a895300d72d2b96e33214697be3a5546928ab Mon Sep 17 00:00:00 2001 From: Chukwuemeka Ajima Date: Tue, 22 Feb 2022 16:24:48 +0100 Subject: [PATCH 5/5] chore(retryCodes): make '503' a default retryCode --- packages/sdk-middleware-http/src/http.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sdk-middleware-http/src/http.js b/packages/sdk-middleware-http/src/http.js index 43b945ada..c40bb1e45 100644 --- a/packages/sdk-middleware-http/src/http.js +++ b/packages/sdk-middleware-http/src/http.js @@ -66,7 +66,7 @@ export default function createHttpMiddleware({ retryDelay = 200, maxDelay = Infinity, retryOnAbort = false, - retryCodes = [], + retryCodes = [503], } = {}, fetch: fetcher, abortController: _abortController, @@ -259,7 +259,7 @@ export default function createHttpMiddleware({ if ( enableRetry && - ([503, ...retryCodes].indexOf(error.statusCode) !== -1 || + (retryCodes.indexOf(error.statusCode) !== -1 || retryCodes?.indexOf(error.message) !== -1) ) { if (retryCount < maxRetries) {