From 183f0867046e0a8cda91cdda822ab1978bfaa5df Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 8 Dec 2025 15:12:46 +0000 Subject: [PATCH] Address PR feedback: Add configurable waitingTimeout and improve implementation - Add waitingTimeout property to CacheProperties for configurable timeout - Extract timeout logic to waiting-timeout.ts utility function - Store request ID outside timeout callback to allow earlier GC - Add unref() to timeout in Node.js to prevent keeping process alive - Fix timeout logic to handle axios default timeout of 0 - Only create timeout when explicitly configured or config.timeout > 0 Changes address comments 2598940999, 2598944866, and 2598947753. Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com> --- src/cache/cache.ts | 16 +++++++ src/cache/create.ts | 4 +- src/interceptors/request.ts | 42 ++++++++++-------- src/util/waiting-timeout.ts | 43 +++++++++++++++++++ test/interceptors/waiting-memory-leak.test.ts | 40 +++++++++-------- 5 files changed, 110 insertions(+), 35 deletions(-) create mode 100644 src/util/waiting-timeout.ts diff --git a/src/cache/cache.ts b/src/cache/cache.ts index aef4cf6..6e366a7 100644 --- a/src/cache/cache.ts +++ b/src/cache/cache.ts @@ -213,6 +213,22 @@ export interface CacheProperties { | CachedStorageValue | StaleStorageValue ) => void | Promise); + + /** + * The maximum time (in milliseconds) that a waiting entry can remain in the waiting map + * before being automatically cleaned up. This prevents memory leaks when cache entries + * are evicted before their responses complete. + * + * This timeout is independent of the axios request timeout and specifically controls + * how long deferred promises remain in the waiting map. + * + * If not set, falls back to `config.timeout`. If neither is set, no timeout is applied + * and waiting entries will only be cleaned up when their responses complete normally. + * + * @default undefined (falls back to config.timeout, then no timeout) + * @see https://axios-cache-interceptor.js.org/config/request-specifics#cache-waitingtimeout + */ + waitingTimeout?: number; } /** diff --git a/src/cache/create.ts b/src/cache/create.ts index 6719c9e..edbd3e1 100644 --- a/src/cache/create.ts +++ b/src/cache/create.ts @@ -84,7 +84,9 @@ export function setupCache(axios: AxiosInstance, options: CacheOptions = {}): Ax override: options.override ?? false, - hydrate: options.hydrate ?? undefined + hydrate: options.hydrate ?? undefined, + + waitingTimeout: options.waitingTimeout }; // Apply interceptors diff --git a/src/interceptors/request.ts b/src/interceptors/request.ts index aa1e216..0704816 100644 --- a/src/interceptors/request.ts +++ b/src/interceptors/request.ts @@ -3,6 +3,7 @@ import type { AxiosCacheInstance, CacheAxiosResponse } from '../cache/axios.js'; import { Header } from '../header/headers.js'; import type { CachedResponse, CachedStorageValue, LoadingStorageValue } from '../storage/types.js'; import { regexOrStringMatch } from '../util/cache-predicate.js'; +import { createWaitingTimeout } from '../util/waiting-timeout.js'; import type { RequestInterceptor } from './build.js'; import { type ConfigWithCache, @@ -163,25 +164,32 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance): RequestInt // Set a timeout to automatically clean up the waiting entry to prevent memory leaks // when entries are evicted from storage before the response completes. - // Use the axios timeout if configured, otherwise use a reasonable default. - const timeout = config.timeout || 30000; // Default 30 seconds if no timeout configured - const timeoutId = setTimeout(() => { - const waiting = axios.waiting.get(config.id!); - if (waiting === def) { - waiting.reject(new Error('Request timeout - waiting entry cleaned up')); - axios.waiting.delete(config.id!); + // Prefer waitingTimeout, then config.timeout, then no timeout (Infinity) + // The timeout should match the request timeout to avoid premature cleanup + // Note: axios defaults timeout to 0, which means no timeout + const waitingTimeout = config.cache.waitingTimeout; + const configTimeout = config.timeout; + const timeout = + waitingTimeout !== undefined + ? waitingTimeout + : configTimeout && configTimeout > 0 + ? configTimeout + : Infinity; - if (__ACI_DEV__) { - axios.debug({ - id: config.id, - msg: 'Cleaned up waiting entry due to timeout' - }); + // Only set up the timeout if it's a positive number + let timeoutId: ReturnType | undefined; + if (timeout !== Infinity && timeout > 0) { + // Store the id outside the timeout callback to allow earlier GC of config + const requestId = config.id; + timeoutId = createWaitingTimeout(axios, requestId, def, timeout); + + // Clear the timeout if the deferred is resolved/rejected to avoid unnecessary cleanup + def.finally(() => { + if (timeoutId !== undefined) { + clearTimeout(timeoutId); } - } - }, timeout); - - // Clear the timeout if the deferred is resolved/rejected to avoid unnecessary cleanup - def.finally(() => clearTimeout(timeoutId)); + }); + } await axios.storage.set( config.id, diff --git a/src/util/waiting-timeout.ts b/src/util/waiting-timeout.ts new file mode 100644 index 0000000..82051ac --- /dev/null +++ b/src/util/waiting-timeout.ts @@ -0,0 +1,43 @@ +import type { Deferred } from 'fast-defer'; +import type { AxiosCacheInstance } from '../cache/axios.js'; + +/** + * Creates and manages a timeout for a waiting entry to prevent memory leaks. + * The timeout automatically cleans up waiting entries that are not resolved + * within the specified time period. + * + * @param axios - The axios cache instance + * @param id - The request ID + * @param def - The deferred promise + * @param timeout - The timeout duration in milliseconds + * @returns The timeout ID (for cleanup purposes) + */ +export function createWaitingTimeout( + axios: AxiosCacheInstance, + id: string, + def: Deferred, + timeout: number +): ReturnType { + const timeoutId = setTimeout(() => { + const waiting = axios.waiting.get(id); + if (waiting === def) { + waiting.reject(new Error('Request timeout - waiting entry cleaned up')); + axios.waiting.delete(id); + + if (__ACI_DEV__) { + axios.debug({ + id, + msg: 'Cleaned up waiting entry due to timeout' + }); + } + } + }, timeout); + + // In Node.js, unref the timeout to prevent it from keeping the process alive + // This is safe because the timeout is only for cleanup purposes + if (typeof timeoutId === 'object' && 'unref' in timeoutId) { + timeoutId.unref(); + } + + return timeoutId; +} diff --git a/test/interceptors/waiting-memory-leak.test.ts b/test/interceptors/waiting-memory-leak.test.ts index 03c33dc..57869ec 100644 --- a/test/interceptors/waiting-memory-leak.test.ts +++ b/test/interceptors/waiting-memory-leak.test.ts @@ -7,10 +7,7 @@ describe('Waiting Memory Leak', () => { it('should clean up waiting map when entry is evicted from storage due to maxEntries', async () => { // Create storage with maxEntries=2 to force eviction const storage = buildMemoryStorage(false, false, 2); - const axios = mockAxios({ storage }); - - // Set a short timeout (100ms) for testing the cleanup mechanism - axios.defaults.timeout = 100; + const axios = mockAxios({ storage, waitingTimeout: 100 }); // Make 3 concurrent requests to different URLs // The first request should be evicted when the third one starts @@ -21,9 +18,11 @@ describe('Waiting Memory Leak', () => { // Wait for all requests to complete await Promise.all([promise1, promise2, promise3]); - // Wait for the timeout to clean up any evicted entries - // Need to wait longer than the timeout to ensure cleanup completes - await new Promise((resolve) => setTimeout(resolve, 200)); + // Poll until waiting map is empty or timeout + const startTime = Date.now(); + while (axios.waiting.size > 0 && Date.now() - startTime < 300) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } // The waiting map should be empty after timeout cleanup assert.equal(axios.waiting.size, 0, 'waiting map should be empty after all requests complete'); @@ -32,8 +31,7 @@ describe('Waiting Memory Leak', () => { it('should clean up waiting map when loading entry is evicted during concurrent requests', async () => { // Create storage with maxEntries=1 to force aggressive eviction const storage = buildMemoryStorage(false, false, 1); - const axios = mockAxios({ storage }); - axios.defaults.timeout = 100; + const axios = mockAxios({ storage, waitingTimeout: 100 }); // Start two concurrent requests const promise1 = axios.get('url1'); @@ -46,8 +44,11 @@ describe('Waiting Memory Leak', () => { assert.ok(result1.data); assert.ok(result2.data); - // Wait for the timeout to clean up any evicted entries - await new Promise((resolve) => setTimeout(resolve, 200)); + // Poll until waiting map is empty or timeout + const startTime = Date.now(); + while (axios.waiting.size > 0 && Date.now() - startTime < 300) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } // The waiting map should be empty assert.equal(axios.waiting.size, 0, 'waiting map should be empty but has entries'); @@ -55,22 +56,27 @@ describe('Waiting Memory Leak', () => { it('should handle multiple waves of concurrent requests with maxEntries', async () => { const storage = buildMemoryStorage(false, false, 2); - const axios = mockAxios({ storage }); - axios.defaults.timeout = 100; + const axios = mockAxios({ storage, waitingTimeout: 100 }); // First wave of requests await Promise.all([axios.get('url1'), axios.get('url2'), axios.get('url3')]); - // Wait for the timeout to clean up any evicted entries - await new Promise((resolve) => setTimeout(resolve, 200)); + // Poll until waiting map is empty or timeout + let startTime = Date.now(); + while (axios.waiting.size > 0 && Date.now() - startTime < 300) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } assert.equal(axios.waiting.size, 0, 'waiting map should be empty after first wave'); // Second wave of requests await Promise.all([axios.get('url4'), axios.get('url5'), axios.get('url6')]); - // Wait for the timeout to clean up any evicted entries - await new Promise((resolve) => setTimeout(resolve, 200)); + // Poll until waiting map is empty or timeout + startTime = Date.now(); + while (axios.waiting.size > 0 && Date.now() - startTime < 300) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } assert.equal(axios.waiting.size, 0, 'waiting map should be empty after second wave'); });