Copilot 5a8cc53cdd
Fix memory leak in axios.waiting and improve storage handling (#1140)
* Initial plan

* Fix axios.waiting memory leak when maxEntries is set

- Added cleanup of waiting map entries in response interceptor when cache state is not 'loading'
- This prevents memory leaks when entries are evicted from storage due to maxEntries limit
- Added comprehensive tests to verify the fix
- Updated existing test to reflect correct behavior

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* Remove pnpm-workspace.yaml build artifact and add to gitignore

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* Refactor: Extract resolveWaiting helper to reduce code duplication

- Created resolveWaiting helper function to centralize the logic for resolving and cleaning up waiting entries
- Simplified test error message to avoid redundancy
- This improves code maintainability by avoiding duplication

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* Implement timeout-based cleanup for waiting entries

- Add timeout mechanism to automatically clean up waiting entries after axios timeout period
- Use axios config timeout or default to 30 seconds if not configured
- This prevents memory leaks when entries are evicted from storage before response completes
- Updated tests to use short timeouts and wait for cleanup to complete

Note: This implements the approach requested by @arthurfiorette to use axios timeout
instead of immediately resolving/rejecting waiting entries when cache state is not loading.

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* 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>

* Simplify configuration: Remove waitingTimeout property, use config.timeout

- Removed waitingTimeout from CacheProperties (too much configuration)
- Moved timeout logic into createWaitingTimeout utility function
- Use config.timeout or axios.defaults.timeout for timeout value
- Only add finally handler if timeout was actually created
- Updated tests to use axios.defaults.timeout instead

Addresses comments 2599033372, 2599040440, and 2599040616.

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* Code cleanup: simplify comment and remove unnecessary variable

- Simplified comment on line 165 to explain WHY (prevent memory leaks) instead of WHAT
- Removed unnecessary requestId variable, passing config.id directly to createWaitingTimeout

Addresses feedback on lines 165 and 169.

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* Remove redundant condition check in waiting-timeout

The condition `timeout <= 0` is redundant since the logic above (lines 30-35)
already ensures that timeout is either a positive number or undefined.

Addresses comment 2599097978.

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

* changes

* Fix maxEntries eviction logic, web storage quota handling, and update docs

**Code improvements:**
- Fixed memory storage maxEntries logic: now properly evicts when at capacity
- Fixed web storage isDomQuotaExceededError to work with jsdom's DOMException
- Added duck-typing check for DOMException to handle cross-context scenarios

**Documentation updates:**
- Added maxStaleAge parameter documentation for both memory and web storage
- Documented that memory storage now uses Map internally
- Updated default values in code examples (cleanupInterval: 5min, maxEntries: 1024)
- Added options section for web storage explaining all parameters

**Test fixes:**
- All 150 tests now passing
- Fixed web storage quota tests that were failing due to DOMException instanceof check

Addresses @arthurfiorette's request to review/improve code, update documentation, and fix tests.

Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: arthurfiorette <47537704+arthurfiorette@users.noreply.github.com>
Co-authored-by: Arthur Fiorette <me@arthur.place>
2025-12-08 15:11:19 -03:00

132 lines
4.3 KiB
TypeScript

import { buildStorage, canRemoveStorageEntry, storageEntriesSorter } from './build.js';
import type { AxiosStorage, StorageValue } from './types.js';
/* c8 ignore start */
/**
* Clones an object using the structured clone algorithm if available, otherwise it uses
* JSON.parse(JSON.stringify(value)).
*/
const clone: <T>(value: T) => T =
// https://caniuse.com/mdn-api_structuredclone (10/18/2023 92.51%)
typeof structuredClone === 'function'
? structuredClone
: (value) => JSON.parse(JSON.stringify(value));
/* c8 ignore stop */
/**
* Creates a simple in-memory storage. This means that if you need to persist data between
* page or server reloads, this will not help.
*
* This is the storage used by default.
*
* If you need to modify it's data, you can do by the `data` property.
*
* @example
*
* ```js
* const memoryStorage = buildMemoryStorage();
*
* setupCache(axios, { storage: memoryStorage });
*
* // Simple example to force delete the request cache
*
* const { id } = axios.get('url');
*
* delete memoryStorage.data[id];
* ```
*
* @param {boolean | 'double'} cloneData Use `true` if the data returned by `find()`
* should be cloned to avoid mutating the original data outside the `set()` method. Use
* `'double'` to also clone before saving value in storage using `set()`. Disabled is
* default
* @param {number | false} cleanupInterval The interval in milliseconds to run a
* setInterval job of cleaning old entries. If false, the job will not be created.
* 5 minutes (300_000) is default
* @param {number | false} maxEntries The maximum number of entries to keep in the
* storage. Its hard to determine the size of the entries, so a smart FIFO order is used
* to determine eviction. If false, no check will be done and you may grow up memory
* usage. 1024 is default
* @param {number} maxStaleAge The maximum age in milliseconds a stale entry can stay
* in the storage before being removed. Otherwise, stale-able entries would stay
* indefinitely causing a memory leak eventually. 1 hour (3_600_000) is default
*/
export function buildMemoryStorage(
cloneData: boolean | 'double' = false,
cleanupInterval: number | false = 5 * 60 * 1000,
maxEntries: number | false = 1024,
maxStaleAge: number = 60 * 60 * 1000
) {
function sortedEntries() {
return Array.from(storage.data.entries()).sort(storageEntriesSorter);
}
const storage = buildStorage({
set: (key, value) => {
// More entries than allowed, evict oldest ones
if (maxEntries && storage.data.size >= maxEntries) {
storage.cleanup();
// After cleanup, if still at or over capacity, manually evict entries
if (storage.data.size >= maxEntries) {
for (const [key] of sortedEntries()) {
storage.data.delete(key);
if (storage.data.size < maxEntries) {
break;
}
}
}
}
// Clone the value before storing to prevent future mutations
// from affecting cached data.
storage.data.set(key, cloneData === 'double' ? clone(value) : value);
},
remove: (key) => {
storage.data.delete(key);
},
find: (key) => {
const value = storage.data.get(key);
return cloneData && value !== undefined ? clone(value) : value;
},
clear: () => {
storage.data.clear();
}
}) as MemoryStorage;
storage.data = new Map();
// When this program gets running for more than the specified interval, there's a good
// chance of it being a long-running process or at least have a lot of entries. Therefore,
// "faster" loop is more important than code readability.
storage.cleanup = () => {
for (const [key, value] of sortedEntries()) {
if (canRemoveStorageEntry(value, maxStaleAge)) {
storage.data.delete(key);
}
}
};
if (cleanupInterval) {
storage.cleaner = setInterval(storage.cleanup, cleanupInterval);
// Attempt to unref the interval to not block Node.js from exiting
if (typeof storage.cleaner === 'object' && 'unref' in storage.cleaner) {
storage.cleaner.unref();
}
}
return storage;
}
export interface MemoryStorage extends AxiosStorage {
data: Map<string, StorageValue>;
/** The job responsible to cleaning old entries */
cleaner: ReturnType<typeof setInterval>;
/** Tries to remove any invalid entry from the memory */
cleanup: () => void;
}