Skip to content

Fix memory leak caused by shared abort signals #683

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.

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

Merged
merged 4 commits into from
Mar 31, 2025

Conversation

sebinsua
Copy link
Contributor

When using a shared AbortSignal across multiple requests, event
listeners were being added but never removed. This resulted in
memory leaks as each request's context remained in memory indefinitely.

The fix uses AbortSignal.any() to properly combine signals without
leaking memory, and eliminates a need for a manual event listener
approach. Additionally, this already handles the case where a signal is
already aborted, making the explicit .aborted check unnecessary.

When using a shared `AbortSignal` across multiple requests, event
listeners were being added but never removed. This resulted in
memory leaks as each request's context remained in memory indefinitely.

The fix uses `AbortSignal.any()` to properly combine signals without
leaking memory, and eliminates a need for a manual event listener
approach. Additionally, this already handles the case where a signal is
already aborted, making the explicit `.aborted` check unnecessary.
@sebinsua sebinsua changed the title Fix abort signal memory leak Fix memory leak caused by shared abort signals Mar 25, 2025
@sebinsua sebinsua force-pushed the fix-abort-signal-memory-leak branch 5 times, most recently from 756f1f2 to 1c199ad Compare March 25, 2025 16:25
@sholladay
Copy link
Collaborator

Awesome! Thanks for writing a test.

Is there a good way to detect the leak without importing a dependency? For example, if duplicate event listeners were being installed, maybe they could increment a counter in the test, which could be asserted at the end to have the expected count. Or if that doesn't work because the old listeners aren't triggered, then maybe we could directly count how many listeners there are with getEventListeners()? Not sure if that method is supported by abort signals.

@sebinsua
Copy link
Contributor Author

sebinsua commented Mar 25, 2025

The logic for jest-leak-detector is actually very simple so I could inline and simplify it if that would be preferred?

However, I'd recommend testing in this way instead of counting the number of event listeners, because otherwise you're testing the current cause of the leak instead of whether there is a leak or not, and (in an http client) you'll eventually get more bang-for-buck by having a generalisable way of unit testing for leaks. I also don't think the list of event listeners is generally exposed outside of Devtools, too. FWIW, I learnt the technique of testing for leaks from reading the unit tests of jotai.

FYI, the leak itself is due to the event handler closing over everything passed into Ky.create and then being attached onto something that is potentially long-lived (e.g. options.signal). You can forget to remove event listeners and it's fine normally as they'll be cleared up if the resource(s) they're attached to goes out of scope, but when they're attached to something that is long-lived it's a different situation. You can see in my test that without passing in an AbortSignal there isn't a 'leak' as by the end of the request nothing points towards this memory.

@sholladay
Copy link
Collaborator

If the goal is to detect regressions for this specific issue, I think counting listeners would be perfectly fine.

If the goal is to create a more general purpose system for detecting leaks throughout the Ky tests, then I agree that checking actual memory usage is much better. However, in that case, it would be preferable to implement it as a reusable utility, perhaps as a test macro so it can run assertions.

@sholladay
Copy link
Collaborator

I'll merge this now so we can get the fix out. It would be nice to have this refactored as a utility, though.

@sholladay sholladay merged commit e48386e into sindresorhus:main Mar 31, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants