From fd482b701863c02586e69b163d37e65d41c0ff9a Mon Sep 17 00:00:00 2001 From: Lam Tran Date: Thu, 13 Jun 2024 11:48:31 +0200 Subject: [PATCH] feat(abortController): add retry on abort (#714) --- .../sdk-v3/error-cases.test.ts | 35 +++- packages/sdk-client-v3/src/types/types.d.ts | 3 +- packages/sdk-client-v3/src/utils/executor.ts | 51 ++++- packages/sdk-client-v3/src/utils/validate.ts | 5 - .../tests/http.test/http-middleware.test.ts | 189 ++++++++++++++++-- 5 files changed, 245 insertions(+), 38 deletions(-) diff --git a/packages/platform-sdk/test/integration-tests/sdk-v3/error-cases.test.ts b/packages/platform-sdk/test/integration-tests/sdk-v3/error-cases.test.ts index 60258331f..8d0d00487 100644 --- a/packages/platform-sdk/test/integration-tests/sdk-v3/error-cases.test.ts +++ b/packages/platform-sdk/test/integration-tests/sdk-v3/error-cases.test.ts @@ -1,10 +1,11 @@ import { ClientBuilder as ClientBuilderV3 } from '@commercetools/ts-client/src' import { - authMiddlewareOptions, + authMiddlewareOptionsV3, httpMiddlewareOptionsV3, projectKey, } from '../test-utils' import { createApiBuilderFromCtpClient } from '../../../src' +import fetch from 'node-fetch' describe('testing error cases', () => { it('should throw error when a product type is not found', async () => { @@ -12,7 +13,7 @@ describe('testing error cases', () => { const ctpClientV3 = new ClientBuilderV3() .withHttpMiddleware(httpMiddlewareOptionsV3) .withConcurrentModificationMiddleware() - .withClientCredentialsFlow(authMiddlewareOptions) + .withClientCredentialsFlow(authMiddlewareOptionsV3) .build() const apiRootV3 = createApiBuilderFromCtpClient( @@ -32,4 +33,34 @@ describe('testing error cases', () => { expect(e.statusCode).toEqual(404) } }) + + it('should retry when aborted', async () => { + let isFirstCall = true + + const client = new ClientBuilderV3() + .withClientCredentialsFlow(authMiddlewareOptionsV3) + .withHttpMiddleware({ + ...httpMiddlewareOptionsV3, + enableRetry: true, + retryConfig: { + retryOnAbort: true, + }, + httpClient: function (url, args) { + if (isFirstCall) { + isFirstCall = false + return fetch(url, { ...args, signal: AbortSignal.timeout(10) }) + } else { + return fetch(url, args) + } + }, + }) + .build() + + const apiRootV3 = createApiBuilderFromCtpClient(client).withProjectKey({ + projectKey, + }) + const response = await apiRootV3.get().execute() + + expect(response.statusCode).toBe(200) + }) }) diff --git a/packages/sdk-client-v3/src/types/types.d.ts b/packages/sdk-client-v3/src/types/types.d.ts index bf57f4387..6e47d6168 100644 --- a/packages/sdk-client-v3/src/types/types.d.ts +++ b/packages/sdk-client-v3/src/types/types.d.ts @@ -222,7 +222,7 @@ export type HttpMiddlewareOptions = { retryConfig?: RetryOptions httpClient: Function getAbortController?: () => AbortController - httpClientOptions?: object + httpClientOptions?: object // will be passed as a second argument to your httpClient function for configuration } export type RetryOptions = RetryMiddlewareOptions @@ -242,6 +242,7 @@ export type RetryMiddlewareOptions = { maxRetries?: number retryDelay?: number maxDelay?: typeof Infinity + retryOnAbort?: boolean retryCodes?: Array } diff --git a/packages/sdk-client-v3/src/utils/executor.ts b/packages/sdk-client-v3/src/utils/executor.ts index 57699ac6e..eb64a4e41 100644 --- a/packages/sdk-client-v3/src/utils/executor.ts +++ b/packages/sdk-client-v3/src/utils/executor.ts @@ -1,8 +1,11 @@ import { HttpClientConfig, IResponse, TResponse } from '../types/types' import { calculateRetryDelay, sleep, validateRetryCodes } from '../utils' -function predicate(retryCodes: Array, response: any) { - return !( +function hasResponseRetryCode( + retryCodes: Array, + response: any +) { + return ( // retryCodes.includes(response?.error?.message) || [503, ...retryCodes].includes(response?.status || response?.statusCode) ) @@ -33,13 +36,15 @@ export default async function executor(request: HttpClientConfig) { const data: TResponse = await executeHttpClientRequest( async (options: HttpClientConfig): Promise => { - const { enableRetry, retryConfig } = rest + const { enableRetry, retryConfig, abortController } = rest const { retryCodes = [], maxDelay = Infinity, maxRetries = 3, backoff = true, retryDelay = 200, + // If set to true reinitialize the abort controller when the timeout is reached and apply the retry config + retryOnAbort = true, } = retryConfig || {} let result: string, @@ -68,17 +73,41 @@ export default async function executor(request: HttpClientConfig) { } async function executeWithRetry(): Promise { - // first attempt - let _response = await execute() - - if (predicate(retryCodes, _response)) return _response + const executeWithTryCatch = async (retryCodes, retryWhenAborted) => { + let _response = {} as any + try { + _response = await execute() + if ( + _response.status > 299 && + hasResponseRetryCode(retryCodes, _response) + ) + return { _response, shouldRetry: true } + } catch (e) { + if (e.name.includes('AbortError') && retryWhenAborted) { + return { _response: e, shouldRetry: true } + } else { + throw e + } + } + return { _response, shouldRetry: false } + } + const retryWhenAborted = + retryOnAbort || !abortController || !abortController.signal + // first attempt + let { _response, shouldRetry } = await executeWithTryCatch( + retryCodes, + retryWhenAborted + ) // retry attempts - while (enableRetry && retryCount < maxRetries) { + while (enableRetry && shouldRetry && retryCount < maxRetries) { retryCount++ - _response = await execute() - - if (predicate(retryCodes, _response)) return _response + const execution = await executeWithTryCatch( + retryCodes, + retryWhenAborted + ) + _response = execution._response + shouldRetry = execution.shouldRetry // delay next execution const timer = calculateRetryDelay({ diff --git a/packages/sdk-client-v3/src/utils/validate.ts b/packages/sdk-client-v3/src/utils/validate.ts index 9fd45acb6..8cee5d80d 100644 --- a/packages/sdk-client-v3/src/utils/validate.ts +++ b/packages/sdk-client-v3/src/utils/validate.ts @@ -19,11 +19,6 @@ export function validateHttpOptions(options: HttpMiddlewareOptions) { throw new Error( 'An `httpClient` is not available, please pass in a `fetch` or `axios` instance as an option or have them globally available.' ) - - if (options.timeout && !options.getAbortController) - throw new Error( - '`AbortController` is not available. Please pass in `getAbortController` as an option or have AbortController globally available when using timeout.' - ) } /** diff --git a/packages/sdk-client-v3/tests/http.test/http-middleware.test.ts b/packages/sdk-client-v3/tests/http.test/http-middleware.test.ts index 8cc06d085..e2f31a6e0 100644 --- a/packages/sdk-client-v3/tests/http.test/http-middleware.test.ts +++ b/packages/sdk-client-v3/tests/http.test/http-middleware.test.ts @@ -1,6 +1,7 @@ import { HttpMiddlewareOptions, MiddlewareRequest } from '../../src' import { Buffer } from 'buffer/' import { createHttpMiddleware } from '../../src/middleware' +import fetch from 'node-fetch' function createTestRequest(options) { return { @@ -52,23 +53,6 @@ describe('Http Middleware.', () => { ).toThrow() }) - test('should throw if timeout is provided and `AbortController` is not.', () => { - const response = createTestResponse({}) - const httpMiddlewareOptions = { - host: 'http://api-host.com', - httpClient: jest.fn(), - timeout: 2000, - getAbortController: null, - } - - const next = () => response - expect(() => - createHttpMiddleware(httpMiddlewareOptions)(next)(createTestRequest({})) - ).toThrow( - /`AbortController` is not available. Please pass in `getAbortController` as an option or have AbortController globally available when using timeout./ - ) - }) - test('should throw if the set timeout value elapses', async () => { const response = createTestResponse({ statusCode: 0 }) const httpMiddlewareOptions: HttpMiddlewareOptions = { @@ -474,7 +458,7 @@ describe('Http Middleware.', () => { test('should retry request based on response status code', async () => { const response = createTestResponse({ data: {}, - statusCode: 504, + status: 504, }) const request = createTestRequest({ @@ -515,7 +499,7 @@ describe('Http Middleware.', () => { test('should retry request with exponential backoff', async () => { const response = createTestResponse({ data: {}, - statusCode: 503, + status: 503, }) const request = createTestRequest({ @@ -552,5 +536,172 @@ describe('Http Middleware.', () => { await createHttpMiddleware(httpMiddlewareOptions)(next)(request) }) + + test('should not retry request by default when aborted', async () => { + const request = createTestRequest({ + uri: '/delay/1', + method: 'GET', + }) + + const next = (req: MiddlewareRequest) => { + return createTestResponse(req.response) + } + + const httpMiddlewareOptions: HttpMiddlewareOptions = { + host: 'https://httpbin.org', + httpClient: fetch, + timeout: 500, + enableRetry: true, + retryConfig: { + maxRetries: 2, + retryOnAbort: false, + }, + } + + const response = await createHttpMiddleware(httpMiddlewareOptions)(next)( + request + ) + + expect(response.error.message).toContain('aborted') + }) + + test('should throw an error when httpClient is invalid', async () => { + const request = createTestRequest({ + uri: '/test', + method: 'GET', + }) + + const testResponse = createTestResponse({ + data: {}, + statusCode: 503, + }) + + const next = (req: MiddlewareRequest) => { + return testResponse + } + const testError = new Error() + + const httpMiddlewareOptions: HttpMiddlewareOptions = { + host: 'https://httpbin.org', + httpClient: jest.fn(() => { + throw testError + }), + timeout: 500, + enableRetry: true, + retryConfig: { + maxRetries: 2, + retryOnAbort: false, + }, + } + + try { + await createHttpMiddleware(httpMiddlewareOptions)(next)(request) + } catch (e) { + expect(e).toEqual(testError) + } + }) + + test('should retry request on aborted when retryOnAbort=true', async () => { + const testData = 'this is a string response data' + + const request = createTestRequest({ + uri: '/delay/1', + method: 'GET', + }) + + const next = (req: MiddlewareRequest) => { + return createTestResponse(req.response) + } + + let httpClientCalled = false + + const httpMiddlewareOptions: HttpMiddlewareOptions = { + host: 'https://httpbin.org', + httpClient: (url, options) => { + if (httpClientCalled) { + return createTestResponse({ + data: testData, + statusCode: 200, + headers: { + 'server-time': '05:07', + }, + }) + } else { + httpClientCalled = true + return fetch(url, options) + } + }, + timeout: 500, + enableRetry: true, + retryConfig: { + maxRetries: 2, + retryDelay: 500, + retryOnAbort: true, + }, + } + + const response = await createHttpMiddleware(httpMiddlewareOptions)(next)( + request + ) + + expect(response.error).toBeUndefined() + expect(response.statusCode).toEqual(200) + expect(response.body).toEqual(testData) + }) + + test('should retry CTP request when retry=true and retryOnAbort=false', async () => { + const testData = 'this is a string response data' + const testResponse = createTestResponse({ + data: {}, + statusCode: 504, + }) + + const request = createTestRequest({ + uri: '/error-url/retry', + method: 'POST', + body: { id: 'test-id' }, + headers: { + 'Content-Type': 'image/jpeg', + }, + }) + + let httpClientCalled = false + + const httpMiddlewareOptions: HttpMiddlewareOptions = { + host: 'https://httpbin.org', + httpClient: (url, options) => { + if (httpClientCalled) { + return createTestResponse({ + data: testData, + statusCode: 200, + headers: { + 'server-time': '05:07', + }, + }) + } else { + httpClientCalled = true + return testResponse + } + }, + enableRetry: true, + retryConfig: { + maxRetries: 3, + backoff: false, + retryDelay: 200, + retryCodes: [504], + retryOnAbort: false, + }, + } + + const next = (req: MiddlewareRequest) => { + return testResponse + } + + const response = await createHttpMiddleware(httpMiddlewareOptions)(next)( + request + ) + + expect(response).toBeDefined() + }) }) })