AbortSignal.any() not destroyed when goes out of scope

[zdm]

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 new AbortSignal instance.
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.

[zdm]

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.

[zdm]

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 1

watch - watch for object destroyed by garbage collector using FinalizationRegistry.

As you can see, signals, created with AbortSignak.any() are not destroyed at all.

[bnoordhuis]

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.

[zdm]

As 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.

[zdm]

Please, reopen this issue, it requires fix.

[MoLow]

this behavior is expected and is more related to events than to AbortSignal.any

in 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 });

[zdm]

It 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

[MoLow]

I am reopening.
I agree AbortSignal.any( [] ) should not leak memory.

[MoLow]

CC @atlowChemi

[Linkgoron]

The 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() );
    });
});

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef#notes_on_weakrefs

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.

https://dom.spec.whatwg.org/#dom-abortsignal-any

[zdm]

Pkease, 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.

[Linkgoron]

As already stated, AbortSignal is a web platform API (yes it is implemented by Node itself, but it complies to a standard).

https://dom.spec.whatwg.org/#interface-AbortSignal

[zdm]

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

[zdm]

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,

[atlowChemi]

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 AbortSignal is an EventTarget (as defined in spec), which I think causes a slow initialization of the signal. 😕 (nodejs/performance#32)

[zdm]

Could you, please run this:

while ( 1 ) {
    AbortSignal.any( [new AbortController().signal, new AbortController().signal] );
}

You will see that memory not freeing,

[zdm]

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.

[zdm]

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.

[Linkgoron]

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.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/FinalizationRegistry#notes_on_cleanup_callbacks

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 any method.

[zdm]

Yes. you are right.
Thank you for reply.

[davidfiala]

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 WeakRef that 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.