diff --git a/src/common/interfaces/get-options.interface.ts b/src/common/interfaces/get-options.interface.ts index 494fba798..3212565d6 100644 --- a/src/common/interfaces/get-options.interface.ts +++ b/src/common/interfaces/get-options.interface.ts @@ -6,4 +6,6 @@ export interface GetOptions { skipApiKeyCheck?: boolean; /** Maximum number of retries for this request, overriding the client-wide `maxRetries`. */ maxRetries?: number; + /** Aborts the request, rejecting with the signal's reason. An aborted request is not retried. */ + signal?: AbortSignal; } diff --git a/src/common/interfaces/http-client.interface.ts b/src/common/interfaces/http-client.interface.ts index be6d7decd..ff1a8302e 100644 --- a/src/common/interfaces/http-client.interface.ts +++ b/src/common/interfaces/http-client.interface.ts @@ -7,6 +7,8 @@ export type RequestOptions = { * `maxRetries`. Set to `0` to disable retries for this request. */ maxRetries?: number; + /** Aborts the request. An aborted request is not retried. */ + signal?: AbortSignal; }; export type ResponseHeaderValue = string | string[]; export type ResponseHeaders = Record; diff --git a/src/common/net/fetch-client.spec.ts b/src/common/net/fetch-client.spec.ts index a4c283483..cb8dd28bd 100644 --- a/src/common/net/fetch-client.spec.ts +++ b/src/common/net/fetch-client.spec.ts @@ -661,6 +661,77 @@ describe('FetchHttpClient with timeout', () => { }); }); +describe('FetchHttpClient with an abort signal', () => { + beforeEach(() => fetch.resetMocks()); + + it('aborts the request in flight and fails with the signal reason instead of retrying a timeout', async () => { + const mockFetch = jest.fn(); + mockFetch.mockImplementation( + (_, options) => + new Promise((_, reject) => { + options.signal.addEventListener('abort', () => { + const error = new Error('AbortError'); + error.name = 'AbortError'; + reject(error); + }); + }), + ); + const client = new FetchHttpClient( + 'https://api.example.com', + {}, + mockFetch, + ); + const controller = new AbortController(); + const reason = new Error('Stop'); + + const request = client.get('/users', { signal: controller.signal }); + controller.abort(reason); + + await expect(request).rejects.toBe(reason); + expect(mockFetch).toHaveBeenCalledTimes(1); + expect(mockFetch.mock.calls[0][1].signal.aborted).toBe(true); + }); + + it('fails with a null signal reason instead of returning no response', async () => { + const mockFetch = jest.fn(); + mockFetch.mockImplementation( + (_, options) => + new Promise((_, reject) => { + options.signal.addEventListener('abort', () => { + const error = new Error('AbortError'); + error.name = 'AbortError'; + reject(error); + }); + }), + ); + const client = new FetchHttpClient( + 'https://api.example.com', + {}, + mockFetch, + ); + const controller = new AbortController(); + + const request = client.get('/users', { signal: controller.signal }); + controller.abort(null); + + await expect(request).rejects.toBeNull(); + expect(mockFetch).toHaveBeenCalledTimes(1); + }); + + it('sends no further attempt once the signal aborts between retries', async () => { + fetchOnce({}, { status: 500 }); + const controller = new AbortController(); + const reason = new Error('Stop'); + const mockSleep = jest.spyOn(fetchClient, 'sleep'); + mockSleep.mockImplementation(async () => controller.abort(reason)); + + await expect( + fetchClient.get('/organizations', { signal: controller.signal }), + ).rejects.toBe(reason); + expect(fetch.mock.calls.length).toBe(1); + }); +}); + describe('request timeout covers the response body (GH-1679)', () => { // `fetch` in this file is the jest-fetch-mock import. type FetchFn = typeof globalThis.fetch; diff --git a/src/common/net/fetch-client.ts b/src/common/net/fetch-client.ts index 1b76c4f41..f13998cfe 100644 --- a/src/common/net/fetch-client.ts +++ b/src/common/net/fetch-client.ts @@ -56,6 +56,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { null, options.headers, options.maxRetries, + options.signal, ); } @@ -79,6 +80,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { ...options.headers, }, options.maxRetries, + options.signal, ); } @@ -102,6 +104,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { ...options.headers, }, options.maxRetries, + options.signal, ); } @@ -125,6 +128,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { ...options.headers, }, options.maxRetries, + options.signal, ); } @@ -144,6 +148,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { null, options.headers, options.maxRetries, + options.signal, ); } @@ -167,6 +172,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { ...options.headers, }, options.maxRetries, + options.signal, ); } @@ -175,7 +181,10 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { method: string, body?: any, headers?: RequestHeaders, + signal?: AbortSignal, ): Promise { + signal?.throwIfAborted(); + // For methods which expect payloads, we should always pass a body value // even when it is empty. Without this, some JS runtimes (eg. Deno) will // inject a second Content-Length header. @@ -193,17 +202,26 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { const timeoutId = setTimeout(() => { abortController.abort(); }, timeout); + const abortRequest = () => abortController.abort(); + signal?.addEventListener('abort', abortRequest, { once: true }); + const release = () => { + clearTimeout(timeoutId); + signal?.removeEventListener('abort', abortRequest); + }; // Pass the response headers once they are known so a timeout while - // reading the body still carries the request ID and Retry-After. - const timeoutError = (responseHeaders?: Headers) => - new HttpClientError({ - message: `Request timeout after ${timeout}ms`, - response: { - status: 408, - headers: responseHeaders ?? new Headers(), - data: { error: 'Request timeout' }, - }, - }); + // reading the body still carries the request ID and Retry-After. A + // request the caller aborted fails with the caller's reason instead. + const abortError = (responseHeaders?: Headers) => + signal?.aborted + ? signal.reason + : new HttpClientError({ + message: `Request timeout after ${timeout}ms`, + response: { + status: 408, + headers: responseHeaders ?? new Headers(), + data: { error: 'Request timeout' }, + }, + }); // Set once the headers arrive. let res: Response | undefined; @@ -226,7 +244,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { // Read the error body under the same deadline, inside the attempt, // so a stalled error response is retried like any other timeout. const rawBody = await res.text(); - clearTimeout(timeoutId); + release(); let responseJson: any; @@ -269,10 +287,10 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { // AbortError a fetch implementation raises for an aborted body may // come from another realm. throw abortController.signal.aborted - ? timeoutError(response.headers) + ? abortError(response.headers) : error; } finally { - clearTimeout(timeoutId); + release(); } })(); rawBody.catch(() => undefined); @@ -280,11 +298,11 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { return new FetchHttpClientResponse(res, rawBody); } catch (error) { // Clear timeout if request failed - clearTimeout(timeoutId); + release(); - // Handle timeout errors + // Handle timeouts and aborts if (abortController.signal.aborted) { - throw timeoutError(res?.headers); + throw abortError(res?.headers); } throw error; @@ -297,6 +315,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { body?: any, headers?: RequestHeaders, maxRetries?: number, + signal?: AbortSignal, ): Promise { const maxRetryAttempts = maxRetries ?? this.MAX_RETRY_ATTEMPTS; @@ -316,14 +335,24 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { const makeRequest = async (): Promise => { let requestError: any = null; + // Tracked apart from the error: an abort can reject with a null reason. + let failed = false; try { - response = await this.fetchRequest(url, method, body, requestHeaders); + response = await this.fetchRequest( + url, + method, + body, + requestHeaders, + signal, + ); } catch (e) { requestError = e; + failed = true; } if ( + !signal?.aborted && this.shouldRetryRequest(requestError, retryAttempts, maxRetryAttempts) ) { retryAttempts++; @@ -334,7 +363,7 @@ export class FetchHttpClient extends HttpClient implements HttpClientInterface { return makeRequest(); } - if (requestError != null) { + if (failed) { throw requestError; } diff --git a/src/feature-flags/runtime-client.spec.ts b/src/feature-flags/runtime-client.spec.ts index 6b4d2a82c..9fa63cf1b 100644 --- a/src/feature-flags/runtime-client.spec.ts +++ b/src/feature-flags/runtime-client.spec.ts @@ -148,6 +148,53 @@ describe('FeatureFlagsRuntimeClient', () => { // Only the initial poll should have happened expect(fetch.mock.calls.length).toBe(1); }); + + it('aborts the in-flight poll request', async () => { + fetch.mockResponseOnce(() => new Promise(() => {})); + const client = workos.featureFlags.createRuntimeClient(); + await jest.advanceTimersByTimeAsync(0); + const signal = fetch.mock.calls[0][1]?.signal; + expect(signal?.aborted).toBe(false); + + client.close(); + + expect(signal?.aborted).toBe(true); + }); + }); + + describe('requestTimeoutMs', () => { + it('emits a timeout error when a poll takes too long', async () => { + fetch.mockResponseOnce(() => new Promise(() => {})); + const client = workos.featureFlags.createRuntimeClient({ + requestTimeoutMs: 50, + }); + const errors: Error[] = []; + client.on('error', (err) => errors.push(err)); + + await jest.advanceTimersByTimeAsync(50); + + expect(errors).toHaveLength(1); + expect(errors[0].message).toBe('Request timed out'); + + client.close(); + }); + + it('aborts the request of a poll that takes too long', async () => { + fetch.mockResponseOnce(() => new Promise(() => {})); + const client = workos.featureFlags.createRuntimeClient({ + requestTimeoutMs: 50, + }); + client.on('error', () => {}); + + await jest.advanceTimersByTimeAsync(49); + const signal = fetch.mock.calls[0][1]?.signal; + expect(signal?.aborted).toBe(false); + + await jest.advanceTimersByTimeAsync(1); + expect(signal?.aborted).toBe(true); + + client.close(); + }); }); describe('isEnabled', () => { diff --git a/src/feature-flags/runtime-client.ts b/src/feature-flags/runtime-client.ts index cd2a99c03..d5f0365fa 100644 --- a/src/feature-flags/runtime-client.ts +++ b/src/feature-flags/runtime-client.ts @@ -233,38 +233,29 @@ export class FeatureFlagsRuntimeClient extends EventEmitter this.pollAbortController = new AbortController(); const { signal } = this.pollAbortController; - let timeoutId: ReturnType; + const timeoutId = setTimeout(() => { + this.pollAbortController?.abort(new Error('Request timed out')); + }, this.requestTimeoutMs); const fetchPromise = this.workos - .get('/sdk/feature-flags?payload_version=2') + .get('/sdk/feature-flags?payload_version=2', { signal }) .then(({ data }) => data); - const timeoutPromise = new Promise((_, reject) => { - timeoutId = setTimeout(() => { - this.pollAbortController?.abort(); - reject(new Error('Request timed out')); - }, this.requestTimeoutMs); - }); - + // Settles the poll as soon as it is aborted, even while the HTTP client + // is between retries. const abortPromise = new Promise((_, reject) => { if (signal.aborted) { - reject(new Error('Poll aborted')); + reject(signal.reason); return; } - signal.addEventListener( - 'abort', - () => reject(new Error('Poll aborted')), - { - once: true, - }, - ); + signal.addEventListener('abort', () => reject(signal.reason), { + once: true, + }); }); - return Promise.race([fetchPromise, timeoutPromise, abortPromise]).finally( - () => { - clearTimeout(timeoutId); - }, - ); + return Promise.race([fetchPromise, abortPromise]).finally(() => { + clearTimeout(timeoutId); + }); } private scheduleNextPoll(): void { diff --git a/src/workos.spec.ts b/src/workos.spec.ts index 2e61a1107..4a7781259 100644 --- a/src/workos.spec.ts +++ b/src/workos.spec.ts @@ -614,6 +614,54 @@ describe('WorkOS', () => { expect(error.requestID).toBe('a-request-id'); }); }); + + describe('when the signal aborts', () => { + it('aborts the request and rejects with the signal reason', async () => { + const pendingFetch = jest.fn( + (_url: any, init: any) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + const abortError = new Error('Aborted'); + abortError.name = 'AbortError'; + reject(abortError); + }); + }) as any, + ); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + fetchFn: pendingFetch as any, + }); + const controller = new AbortController(); + const reason = new Error('Stop'); + + const request = workos.get('/path', { signal: controller.signal }); + controller.abort(reason); + + await expect(request).rejects.toBe(reason); + expect(pendingFetch.mock.calls[0][1].signal.aborted).toBe(true); + }); + + it('rejects with a null signal reason', async () => { + const pendingFetch = jest.fn( + (_url: any, init: any) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + const abortError = new Error('Aborted'); + abortError.name = 'AbortError'; + reject(abortError); + }); + }) as any, + ); + const workos = new WorkOS('sk_test_Sz3IQjepeSWaI4cMS4ms4sMuU', { + fetchFn: pendingFetch as any, + }); + const controller = new AbortController(); + + const request = workos.get('/path', { signal: controller.signal }); + controller.abort(null); + + await expect(request).rejects.toBeNull(); + }); + }); }); describe('put', () => { diff --git a/src/workos.ts b/src/workos.ts index 780e51f43..d869a7763 100644 --- a/src/workos.ts +++ b/src/workos.ts @@ -284,8 +284,10 @@ export class WorkOS { params: options.query, headers: requestHeaders, maxRetries: options.maxRetries, + signal: options.signal, }); } catch (error) { + options.signal?.throwIfAborted(); this.handleHttpError({ path, error }); throw error;