From 0ffe72433906e74c2b3d287f1d2517b3b69fc382 Mon Sep 17 00:00:00 2001 From: Ajima Chukwuemeka <32770340+ajimae@users.noreply.github.com> Date: Mon, 3 Jul 2023 17:11:45 +0200 Subject: [PATCH] Fix issue with intermittent promise rejection error (#1859) * fix(sdk-middleware-http): issue with intermittent promise rejection error - add an error catch block to handle serialization errors * Create khaki-radios-destroy.md * chore(tests): write tests - DEVX-180 write tests for new bug fix * chore(feedback): implement feedback - use correct error class for error handling. - change network error to server error --- .changeset/khaki-radios-destroy.md | 5 + packages/sdk-middleware-http/src/http.js | 139 +++++++++++------- .../sdk-middleware-http/test/http.spec.js | 110 ++++++++++++-- 3 files changed, 188 insertions(+), 66 deletions(-) create mode 100644 .changeset/khaki-radios-destroy.md diff --git a/.changeset/khaki-radios-destroy.md b/.changeset/khaki-radios-destroy.md new file mode 100644 index 000000000..6234f2e3c --- /dev/null +++ b/.changeset/khaki-radios-destroy.md @@ -0,0 +1,5 @@ +--- +"@commercetools/sdk-middleware-http": patch +--- + +Fix intermittent unhandled promise rejection error diff --git a/packages/sdk-middleware-http/src/http.js b/packages/sdk-middleware-http/src/http.js index 525f4ce3c..e6b805615 100644 --- a/packages/sdk-middleware-http/src/http.js +++ b/packages/sdk-middleware-http/src/http.js @@ -10,7 +10,11 @@ import type { Next, } from 'types/sdk' -import getErrorByCode, { NetworkError, HttpError } from './errors' +import getErrorByCode, { + NetworkError, + HttpError, + InternalServerError, +} from './errors' import parseHeaders from './parse-headers' function createError({ statusCode, message, ...rest }: Object): HttpErrorType { @@ -35,9 +39,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 } @@ -122,10 +126,14 @@ export default function createHttpMiddleware({ delete requestHeader['Content-Type'] } - const body = (['application/json', 'application/graphql'] - .indexOf(requestHeader['Content-Type']) > -1 && typeof request.body === 'string') || - Buffer.isBuffer(request.body) ? request.body : JSON.stringify(request.body || undefined) - + const body = + (['application/json', 'application/graphql'].indexOf( + requestHeader['Content-Type'] + ) > -1 && + typeof request.body === 'string') || + Buffer.isBuffer(request.body) + ? request.body + : JSON.stringify(request.body || undefined) if (body && (typeof body === 'string' || Buffer.isBuffer(body))) { requestHeader['Content-Length'] = Buffer.byteLength(body).toString() @@ -171,7 +179,6 @@ export default function createHttpMiddleware({ fetchOptions.signal = abortController.signal } } - } // Kick off timer for abortController directly before fetch. let timer @@ -239,55 +246,81 @@ export default function createHttpMiddleware({ // 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) => { - // Try to parse the error response as JSON - let parsed - try { - parsed = JSON.parse(text) - } catch (error) { - parsed = text - } + res + .text() + .then((text: any) => { + // Try to parse the error response as JSON + let parsed + try { + parsed = JSON.parse(text) + } catch (error) { + parsed = text + } - const error: HttpErrorType = createError({ - statusCode: res.status, - originalRequest: request, - retryCount, - headers: parseHeaders(res.headers), - ...(typeof parsed === 'object' - ? { message: parsed.message, body: parsed } - : { message: parsed, body: parsed }), - }) + const error: HttpErrorType = createError({ + statusCode: res.status, + originalRequest: request, + retryCount, + headers: parseHeaders(res.headers), + ...(typeof parsed === 'object' + ? { message: parsed.message, body: parsed } + : { message: parsed, body: parsed }), + }) - if ( - enableRetry && - (retryCodes.indexOf(error.statusCode) !== -1 || - retryCodes?.indexOf(error.message) !== -1) - ) { - if (retryCount < maxRetries) { - setTimeout( - executeFetch, - calcDelayDuration( - retryCount, - retryDelay, - maxRetries, - backoff, - maxDelay + if ( + enableRetry && + (retryCodes.indexOf(error.statusCode) !== -1 || + retryCodes?.indexOf(error.message) !== -1) + ) { + if (retryCount < maxRetries) { + setTimeout( + executeFetch, + calcDelayDuration( + retryCount, + retryDelay, + maxRetries, + backoff, + maxDelay + ) ) - ) - retryCount += 1 - return + retryCount += 1 + return + } } - } - maskAuthData(error.originalRequest, maskSensitiveHeaderData) - // Let the final resolver to reject the promise - const parsedResponse = { - ...response, - error, - statusCode: res.status, - } - next(request, parsedResponse) - }) + maskAuthData(error.originalRequest, maskSensitiveHeaderData) + // Let the final resolver to reject the promise + const parsedResponse = { + ...response, + error, + statusCode: res.status, + } + next(request, parsedResponse) + }) + .catch((err: Error) => { + if (enableRetry) + if (retryCount < maxRetries) { + setTimeout( + executeFetch, + calcDelayDuration( + retryCount, + retryDelay, + maxRetries, + backoff, + maxDelay + ) + ) + retryCount += 1 + return + } + + const error = new InternalServerError(err.message, { + originalRequest: request, + retryCount, + }) + maskAuthData(error.originalRequest, maskSensitiveHeaderData) + next(request, { ...response, error, statusCode: 500 }) + }) }, // We know that this is a "network" error thrown by the `fetch` library (e: Error) => { diff --git a/packages/sdk-middleware-http/test/http.spec.js b/packages/sdk-middleware-http/test/http.spec.js index ea94f250e..7cc9886ac 100644 --- a/packages/sdk-middleware-http/test/http.spec.js +++ b/packages/sdk-middleware-http/test/http.spec.js @@ -47,7 +47,11 @@ describe('Http', () => { test('throw when a non-array option is passed as retryCodes in the httpMiddlewareOptions', () => { expect(() => { - createHttpMiddleware({ host: testHost, retryConfig: { retryCodes: null }, fetch }) + createHttpMiddleware({ + host: testHost, + retryConfig: { retryCodes: null }, + fetch, + }) }).toThrow( new Error( '`retryCodes` option must be an array of retry status (error) codes.' @@ -707,9 +711,7 @@ describe('Http', () => { retryConfig: { maxRetries: 2, retryDelay: 300, - retryCodes: [ - 500, 501, 502 - ], + retryCodes: [500, 501, 502], }, fetch, } @@ -739,15 +741,13 @@ describe('Http', () => { retryConfig: { maxRetries: 2, retryDelay: 300, - retryCodes: [ - 'ETIMEDOUT', 'ECONNREFUSED', 'write EPIPE' - ] + retryCodes: ['ETIMEDOUT', 'ECONNREFUSED', 'write EPIPE'], }, fetch, }) nock(testHost) .defaultReplyHeaders({ - 'Content-Type': 'application/json' + 'Content-Type': 'application/json', }) .persist() .get('/foo/bar') @@ -776,15 +776,13 @@ describe('Http', () => { retryConfig: { maxRetries: 2, retryDelay: 300, - retryCodes: [ - 'ETIMEDOUT', 503, 502, 'ECONNREFUSED', 'write EPIPE' - ] + retryCodes: ['ETIMEDOUT', 503, 502, 'ECONNREFUSED', 'write EPIPE'], }, fetch, }) nock(testHost) .defaultReplyHeaders({ - 'Content-Type': 'application/json' + 'Content-Type': 'application/json', }) .persist() .get('/foo/bar') @@ -886,7 +884,6 @@ describe('Http', () => { httpMiddleware(next)(request, response) })) - test( 'should toggle `exponential backoff` off', () => @@ -965,6 +962,93 @@ describe('Http', () => { })) }) + describe('promise rejection error', () => { + test('handle failed response - (server error)', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + const expectedError = new Error('oops') + expectedError.body = { + message: 'oops', + } + expectedError.code = 500 + expectedError.statusCode = 500 + expectedError.headers = { + 'content-type': 'application/json', + } + + expect(res.error.statusCode).toEqual(0) + expect(res.error.retryCount).toEqual(2) + expect(res.error.name).toEqual('NetworkError') + + resolve() + } + const httpMiddleware = createHttpMiddleware({ + host: testHost, + enableRetry: true, + credentialsMode: 'include', + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + }) + nock(testHost) + .defaultReplyHeaders({ + 'Content-Type': 'application/json', + }) + .get('/foo/bar') + .reply(500, { message: 'Internal server error' }) + + httpMiddleware(next)(request, { ...response }) + })) + + test('handle failed response - (server error) - ', () => + new Promise((resolve, reject) => { + const request = createTestRequest({ + uri: '/foo/bar', + }) + const response = { resolve, reject } + const next = (req, res) => { + const expectedError = new Error('oops') + expectedError.body = { + message: 'oops', + } + expectedError.code = 0 + expectedError.statusCode = 0 + expectedError.headers = { + 'content-type': 'application/json', + } + + expect(res.error.code).toEqual(500) + expect(res.error.name).toEqual('InternalServerError') + + resolve() + } + const httpMiddleware = createHttpMiddleware({ + host: testHost, + enableRetry: false, + credentialsMode: 'include', + retryConfig: { + maxRetries: 2, + retryDelay: 300, + }, + fetch, + }) + nock(testHost) + .defaultReplyHeaders({ + 'Content-Type': 'application/json', + }) + .get('/foo/bar') + .reply(400, { message: 'Internal server error' }) + + httpMiddleware(next)(request, { ...response, text: (e) => reject(e) }) + })) + }) + test('handle failed response (api error)', () => new Promise((resolve, reject) => { const request = createTestRequest({