AbortController/AbortSignal memory leak #55328
Comments
|
Can you try with node 22.9? I believe this was fixed quite recently. |
|
Can you please include a reproduction? |
|
Shall we move this to Node? |
|
Transfered. |
|
Interesting comment on this denoland/deno#24842 (comment) |
Part of the issue with this reproduction code may be that event listeners should be removed; otherwise, gcPersistentSignals will retain those. If that's the case I think the issue becomes the same as reported for |
If you add an event listener directly to |
|
I can confirm that leaving a listener attached to the Better repro: function printMemoryUsage(msg) {
console.log(msg, `${(process.memoryUsage().rss / 1024 / 1024).toFixed(2)} MiB`);
}
printMemoryUsage('Mem before loop');
let i = 0;
const run = () => {
const ac = new AbortController();
// NOTE: Not removing this listener does not cause a leak
ac.signal.addEventListener('abort', () => {});
const composedSignal = AbortSignal.any([ac.signal]);
const handler = () => {};
// NOTE: Not removing this listener DOES cause a leak
composedSignal.addEventListener('abort', handler);
// NOTE: Properly removing the listener fixes the leak
// composedSignal.removeEventListener('abort', handler);
if (++i % 100_000 === 0) {
printMemoryUsage(`Mem after ${i} iterations`);
}
setImmediate(run);
};
run(); |
|
|
Sure, but this seems to be a real leak. Run this with function printMemoryUsage(msg) {
console.log(msg, `${(process.memoryUsage().rss / 1024 / 1024).toFixed(2)} MiB`);
}
printMemoryUsage('Mem before loop');
let i = 0;
const run = () => {
const ac = new AbortController();
// NOTE: Not removing this listener does not cause a leak
ac.signal.addEventListener('abort', () => {});
const composedSignal = AbortSignal.any([ac.signal]);
const handler = () => {};
// NOTE: Not removing this listener DOES cause a leak
composedSignal.addEventListener('abort', handler);
// NOTE: Properly removing the listener fixes the leak
// composedSignal.removeEventListener('abort', handler);
if (++i % 100_000 === 0) {
if (globalThis.gc) {
console.log('Running GC');
globalThis.gc(true);
}
printMemoryUsage(`Mem after ${i} iterations`);
}
setImmediate(run);
};
run();I pretty quickly come to: |
Yes, I can get the same test results here, I didn't make it clear just now that I was testing the GC speed after uncommenting the |
|
I was looking into this. This is done by definition: https://github.com/nodejs/node/blob/main/lib/internal/abort_controller.js#L278-L281 I wonder if changing this would violate the spec |
It seems that https://dom.spec.whatwg.org/#abortsignal-abort-algorithms
I don't understand why this needs to be made so complicated. My intuition tells me that Node.js does not need to implement this functionality deliberately, as setTimeout itself can directly hold a strong reference to AbortSignal.timeout, thus preventing AbortSignal from being garbage collected due to the lack of a strong reference. Additionally, AbortSignal.any should also hold a strong reference to the signals. Furthermore, I have another question: When AbortSignal.timeout is active and there are abort event listeners present, the DOM standard states that AbortSignal must not be garbage collected at this time. Since AbortSignal cannot be garbage collected during this period, shouldn't AbortSignal.timeout be able to block the process exit? Currently, AbortSignal.timeout does not block the process exit. |
|
IMO the issue here and the issue with an
|
|
See especially here for more details: https://docs.google.com/document/d/1LvmsBLV85p-PhSGvTH-YwgD6onuhh1VXLg8jPlH32H4/edit?pli=1#heading=h.7ut6obnf9fz0 |
Thanks for the reference, @mika-fischer . This week I'll take a look into this. I've got some ideas. |
|
FYI, PR is done |
PR-URL: #56001 Refs: #55328 Fixes: #55328 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
PR-URL: #56001 Refs: #55328 Fixes: #55328 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
PR-URL: #56001 Refs: #55328 Fixes: #55328 Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Bug Description
The memory of the Node process grows infinitely, ultimately resulting in an OOM error. Memory profiling revealed a memory leak related to AbortController.
Similar to #54614,but the difference is that if AbortSignal.any does not have an event listener added, AbortSignal.any will eventually be released and there will be no memory leak.
I am using fetch API with AbortController/AbortSignal in Node.js(not undici directly).Sorry, I misremembered, I was using node:http+signal.
Reproducible By
Expected Behavior
All AbortController/AbortSignal should be destroyed.
Logs & Screenshots
Environment
Node: v22.3.0
Docker Image: node:apline
Additional context
The text was updated successfully, but these errors were encountered: