Repository navigation
AbortSignal.any() not destroyed when goes out of scope #48419
Description
Activity
Example:
#!/usr/bin/env node import { setFlagsFromString } from "v8"; import { runInNewContext } from "vm"; setFlagsFromString( "--expose_gc" ); const gc = runInNewContext( "gc" ); const ac = new AbortController(); { let s = AbortSignal.any( [ac.signal] ); s.addEventListener( "abort", console.log ); s = null; } gc(); ac.abort();
Listener is called, even if signal is destroyed and V8 garbage collector is executed.
Another example:
#!/usr/bin/env node import watch from "#core/devel/watch"; import gc from "#core/devel/garbage-collector"; { let s = {} watch( s ).on( "destroy", () => console.log( "--- DESTROY 1" ) ); s = null; gc(); } { let s = AbortSignal.any( [] ); watch( s ).on( "destroy", () => console.log( "--- DESTROY 2" ) ); s = null; gc(); }
Output:
--- DESTROY 1watch- watch for object destroyed by garbage collector usingFinalizationRegistry.As you can see, signals, created with
AbortSignak.any()are not destroyed at all.Let me know if I misunderstand you but
AbortSignal.any()is a web platform API, not a node-specific API, so if you want to change or extend it, you have to go through the proper channels. Closing.Reacted by Debadree ChatterjeeAs I see in the node sources - this method is nodejs specific.
Web api documentation dorsn't has any info about this.Also, it now contain memory leak. I tried to run it un the loop anf see, that resident memory growth without lconstantly.
Please, reopen this issue, it requires fix.
this behavior is expected and is more related to events than to
AbortSignal.anyin your example
s.addEventListener( "abort", console.log );
creates an event listener, but nothing ever removes it, hence the memory leak.
you might want to do something like:const handler = (...args) => { s.removeEventListener("abort", handler); console.log(...args); } s.addEventListener( "abort", handler);
or simply
s.addEventListener( "abort", console.log, { once: true });
Reacted by Chemi Atlow and Benjamin GruenbaumIt contains mem leak. Also, just for info, it works very slow.
for ( let n = 0; n < 3; n++ ) { for ( let n = 0; n < 500_000; n++ ) { const a = AbortSignal.any( [] ); } console.log( process.memoryUsage.rss() ); }
output:
1300037632 2685407232 3345960960
For example,
new AbortController()doesn't leak mem:for ( let n = 0; n < 3; n++ ) { for ( let n = 0; n < 500_000; n++ ) { const a = new AbortController(); } console.log( process.memoryUsage.rss() ); }
output:
37724160 38465536 38465536
I am reopening.
I agreeAbortSignal.any( [] )should not leak memory.CC @atlowChemi
Reacted by Chemi AtlowThe behavior is because of how Weakrefs work, internally AbortSignal.any uses weakrefs and they will not get GCed until after a turn of the event loop.
e.g.:
for ( let n = 0; n < 3; n++ ) { for ( let n = 0; n < 500_000; n++ ) { const a = AbortSignal.any( [] ); } console.log( process.memoryUsage.rss() ); } setTimeout(() => { gc(); setTimeout(() => { gc(); console.log( process.memoryUsage.rss() ); }); });If your code has just created a WeakRef for a target object, or has gotten a target object from a WeakRef's deref method, that target object will not be reclaimed until the end of the current JavaScript [job](https://tc39.es/ecma262/#job) (including any promise reaction jobs that run at the end of a script job). That is, you can only "see" an object get reclaimed between turns of the event loop.The any function always allocates a weakref (even when receiving an empty array). For the empty array case, I think that a guard can/should be added so that it won't allocate the relatively heavy object, but it won't solve the general "issue".
As I see in the node sources - this method is nodejs specific. Web api documentation dorsn't has any info about this.
Reacted by Moshe Atlow and Chemi AtlowPkease, add method, whist will allow correctly destory it, unlink from all watched signals and remove all listeners.
We don;t need to watch for abort, when this signal will foes out of scope, so we need to destory it.Alos, it will be handy to add / reomve signals after it was constructed.
As already stated, AbortSignal is a web platform API (yes it is implemented by Node itself, but it complies to a standard).
1 remaining item
- added a commit that references this issue
on Jun 15, 2023 - added a commit that references this issue
on Jul 3, 2023 Hey, mem leak is still present in node 20.4.0,
Try to run this code in a loop, you will see, that memory rss grows without a limit.
AbortSignal.any( [new AbortController().signal, new AbortController().signal] )... duration: 1 sec 886 ms 131 μs rss memory: 948.4 MB, rss memory delta: +526.4 MB duration: 1 sec 684 ms 146 μs rss memory: 1.1 GB, rss memory delta: +203.7 MB duration: 1 sec 792 ms 624 μs rss memory: 1.6 GB, rss memory delta: +536.8 MB duration: 1 sec 681 ms 752 μs rss memory: 2.1 GB, rss memory delta: +507.8 MB duration: 3 secs 123 ms 19 μs rss memory: 2.3 GB, rss memory delta: +205.9 MB duration: 2 secs 49 ms 703 μs 600 ns rss memory: 2.5 GB, rss memory delta: +109.3 MB duration: 3 secs 754 ms 794 μs rss memory: 2.9 GB, rss memory delta: +408.3 MB duration: 1 sec 920 ms 234 μs 1,000 ns rss memory: 2.9 GB, rss memory delta: +26.2 MB ...I don;t know the particular details of the implementation, but if you are using WeakRefs you need to watch for garbagee collection using FinalizationRegistry anf remove refs manually from the set, otherwise they wull never be deleted.
Please, re-open this issue.
Also , it works very slow, for example, speed is comparable with execution of the sql query to the remove sql server.
Code is overengineered, it is unexpectedly complex.
Sorry for this comment, please don't consider is offensive,Hi @zdm not sure exactly how what you executed, but as @Linkgoron mentioned:
they will not get GCed until after a turn of the event loop.
In addition, an
AbortSignalis anEventTarget(as defined in spec), which I think causes a slow initialization of the signal. 😕 (nodejs/performance#32)Could you, please run this:
while ( 1 ) { AbortSignal.any( [new AbortController().signal, new AbortController().signal] ); }You will see that memory not freeing,
Today node 20.4 was released, it includes your patch, which was directed to fix mem leak.
Bit in fact mem leak is still present.
So, please, re-open this issue.Is it possible to remove this mem leak or not?
Could somebody make it cleat please?
Or we need to replace AbortSignal with the own implementation?
Currently it eats all memory in a short time.
Thank you.Could you, please run this:
while ( 1 ) { AbortSignal.any( [new AbortController().signal, new AbortController().signal] ); }You will see that memory not freeing,
As stated - you will never see the memory freeing without freeing the thread-pool. Using FinalizationRegistry will not help you, as internally they also use weak references, and in addition there is no guarantee on when the callback will be called.
As @atlowChemi noted, it's unclear that the WeakRef is actually the issue here regarding performance (see the nodejs/performance issues). In addition, there is no guarantee from the GC side on which objects will actually be collected and when. I would suggest heavily that your code should not be dependent on GC behaviour.
You are welcome to provide PRs that improve the performance and keep the correctness of the
anymethod.Reacted by Chemi AtlowYes. you are right.
Thank you for reply.- added a commit that references this issue
on Jul 17, 2023 - added a commit that references this issue
on Jul 17, 2023 - added 2 commits that reference this issue
on Aug 14, 2023 I'd like to revisit this to better understand and point what I think is a leak still, even when ticks are processing.
Case 1: Tight loop (will OOM) - discussed above
Based on my understanding, running in a tight loop will cause an OOM. I don't like it, but I see the reasoning above:
while ( 1 ) { AbortSignal.any( [new AbortController().signal, new AbortController().signal] ); }
Case 2: Allow ticks to occur (will not OOM) - implied to be OK above
Allowing ticks will do GC, and we won't OOM:
let i = 0; setInterval(() => { i++; AbortSignal.any( [new AbortController().signal, new AbortController().signal] ); if(i % 10000 === 0) { console.log(i, new Date()); } }, 0);
In this case, the inspector shows a slow climb in
(compiled code)over time, but that's it.Case 3: Allow ticks to occur, lose the derived signal, but keep one original signals - a leak?
In a variation of case 2, which I feel fits the real world, I generate Signal.any with one new signal and one long-lived signal.
let i = 0; let ac = new AbortController(); setInterval(() => { i++; AbortSignal.any( [ac.signal, new AbortController().signal] ); if(i % 10000 === 0) { console.log(i, new Date()); } }, 0);
In case 3, I see a steady climb in
WeakRefthat in my brief experimentation is not decreasing over time.My use of setInterval probably is artificially slowing down how fast I can run this test. Perhaps there's a faster way to prove/disprove my observation that this is a permanent memory leak?
I'm curious if I've misunderstood prior comments, or if we should just expect that use of
AbortSignal.any(..)is just going to be leaky if at least one reference to the one source signal hangs around, even when the derived signal is no longer in scope.Thank you.
Version
20.0.3
Platform
all
Subsystem
all
What steps will reproduce the bug?
AbortSignal.any( ...signals )add events listeners to all passed signals and returns newAbortSignalinstance.This instance is not destroyed, when goes out of scope, because has listeners set.
And there is no possibility to unsubscribe from the child signals.
So, pltase, add method, which will allow to perform cleanup, for example
clear().How often does it reproduce? Is there a required condition?
Please, see above.
What is the expected behavior? Why is that the expected behavior?
Please, see above.
What do you see instead?
Please, see above.
Additional information
Please, see above.