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.

Sign up for GitHub

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

Fix issue with intermittent promise rejection error #1859

Merged
merged 4 commits into from
Jul 3, 2023
Merged

Fix issue with intermittent promise rejection error #1859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d5cf21
fix(sdk-middleware-http): issue with intermittent promise rejection e…
ajimae Jun 18, 2023
cc6df50
Create khaki-radios-destroy.md
ajimae Jun 18, 2023
5075964
chore(tests): write tests
ajimae Jun 18, 2023
f2907a1
chore(feedback): implement feedback
ajimae Jul 3, 2023
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
5 changes: 5 additions & 0 deletions .changeset/khaki-radios-destroy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@commercetools/sdk-middleware-http": patch
---

Fix intermittent unhandled promise rejection error
139 changes: 86 additions & 53 deletions packages/sdk-middleware-http/src/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -171,7 +179,6 @@ export default function createHttpMiddleware({
fetchOptions.signal = abortController.signal
}
}

}
// Kick off timer for abortController directly before fetch.
let timer
Expand Down Expand Up @@ -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) => {
Expand Down
110 changes: 97 additions & 13 deletions packages/sdk-middleware-http/test/http.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
Expand Down Expand Up @@ -707,9 +711,7 @@ describe('Http', () => {
retryConfig: {
maxRetries: 2,
retryDelay: 300,
retryCodes: [
500, 501, 502
],
retryCodes: [500, 501, 502],
},
fetch,
}
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -886,7 +884,6 @@ describe('Http', () => {
httpMiddleware(next)(request, response)
}))


test(
'should toggle `exponential backoff` off',
() =>
Expand Down Expand Up @@ -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({
Expand Down