AbortSignal.any() leaks when any of the provided signal is long-lived

[mika-fischer]

Version

v22.9.0

Platform

Microsoft Windows NT 10.0.22631.0 x64

Subsystem

No response

What steps will reproduce the bug?

const ac = new AbortController();

let i = 0;
function run() {
    AbortSignal.any([ac.signal]);
    if (++i % 100_000 === 0) {
        const mem = process.memoryUsage().rss / 1024 / 1024;
        const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).filter(
            (s) => s.toString() === 'Symbol(kDependantSignals)'
        )[0];
        const signals = ac.signal[kDependantSignals];
        console.log(`${i} - ${mem.toFixed(2)} MiB - ${signals?.size} signals`);
    }
    setImmediate(run);
}
run();

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

AbortSignal.any should not case memory leaks

What do you see instead?

❯ node ..\test.js
node .\test.js
100000 - 82.29 MiB - 100000 signals
200000 - 97.98 MiB - 200000 signals
300000 - 126.98 MiB - 300000 signals
400000 - 131.91 MiB - 400000 signals
500000 - 152.07 MiB - 500000 signals
600000 - 162.52 MiB - 600000 signals
700000 - 208.69 MiB - 700000 signals
800000 - 219.15 MiB - 800000 signals
900000 - 221.80 MiB - 900000 signals
1000000 - 244.98 MiB - 1000000 signals
1100000 - 285.09 MiB - 1100000 signals
1200000 - 265.71 MiB - 1200000 signals
1300000 - 265.71 MiB - 1300000 signals
1400000 - 311.45 MiB - 1400000 signals
1500000 - 378.47 MiB - 1500000 signals
1600000 - 377.95 MiB - 1600000 signals
1700000 - 378.01 MiB - 1700000 signals
1800000 - 378.65 MiB - 1800000 signals
1900000 - 406.42 MiB - 1900000 signals
2000000 - 423.68 MiB - 2000000 signals
2100000 - 503.71 MiB - 2100000 signals
2200000 - 503.72 MiB - 2200000 signals
2300000 - 464.14 MiB - 2300000 signals
2400000 - 464.20 MiB - 2400000 signals
2500000 - 464.20 MiB - 2500000 signals
2600000 - 464.20 MiB - 2600000 signals
2700000 - 481.40 MiB - 2700000 signals
2800000 - 548.31 MiB - 2800000 signals
2900000 - 611.93 MiB - 2900000 signals
3000000 - 678.66 MiB - 3000000 signals
3100000 - 685.27 MiB - 3100000 signals
3200000 - 685.36 MiB - 3200000 signals
3300000 - 685.36 MiB - 3300000 signals
3400000 - 685.36 MiB - 3400000 signals
3500000 - 685.36 MiB - 3500000 signals
3600000 - 686.01 MiB - 3600000 signals
3700000 - 686.08 MiB - 3700000 signals
3800000 - 748.08 MiB - 3800000 signals
3900000 - 775.59 MiB - 3900000 signals
...

Additional information

It's pretty clear that AbortSignal.any attaches the combined signal to all its parent signals. But it only gets removed if the parent signals are actually aborted. If there is a long living signal among the parents, for instance something like a SIGINT handler, then it keeps accumulating WeakRefs to no longer existing AbortSignals.

Related issues:

• Related deno issue: AbortSignal.any() leaks when any of the provided signal is long-lived  denoland/deno#24842
• Partly related issue: AbortSignal.any() not destroyed when goes out of scope #48419
  • Particularly case 3 of AbortSignal.any() not destroyed when goes out of scope #48419 (comment)
• Different issue: AbortController/AbortSignal memory leak #55328
  • Another leak when not properly removing listeners from AbortSignal returned by AbortSignal.any
• Probably not an issue at all: AbortSignal.any() causes memory leak #54614

[geeksilva97]

Hm, the memory is being allocated just for refs - potentially pointing to nothing?

Something like this would work?

const finalizers = new SafeFinalizationRegistry((signal) => {
  signal[kDependantSignals].forEach(ref => {
    if (!ref.deref()) {
      signal[kDependantSignals].delete(ref);
    }
  });
});

Then in the loop having this

// ...
for (let i = 0; i < signalsArray.length; i++) {
  const signal = signalsArray[I];
   finalizers.register(resultSignal, signal);

  // ...

Executing this repro after this (maybe naive?) change

./node test.js
100000 - 102.05 MiB - 29116 signals
200000 - 110.38 MiB - 26503 signals
300000 - 110.88 MiB - 21856 signals
400000 - 110.95 MiB - 17219 signals
500000 - 110.41 MiB - 12624 signals
600000 - 110.17 MiB - 8016 signals
700000 - 110.00 MiB - 3414 signals
800000 - 112.42 MiB - 33650 signals
900000 - 111.17 MiB - 29002 signals
1000000 - 111.22 MiB - 24377 signals
1100000 - 111.31 MiB - 19735 signals
1200000 - 110.69 MiB - 15108 signals
1300000 - 110.69 MiB - 10480 signals
1400000 - 110.50 MiB - 5784 signals
1500000 - 110.33 MiB - 1181 signals
1600000 - 111.47 MiB - 31432 signals
1700000 - 112.11 MiB - 26792 signals
1800000 - 112.16 MiB - 22141 signals
1900000 - 112.17 MiB - 17532 signals
2000000 - 111.53 MiB - 12900 signals
2100000 - 111.53 MiB - 8255 signals
2200000 - 111.08 MiB - 3607 signals
2300000 - 113.48 MiB - 33826 signals
2400000 - 112.30 MiB - 29157 signals
2500000 - 112.30 MiB - 24488 signals
2600000 - 112.30 MiB - 19846 signals
2700000 - 111.75 MiB - 15259 signals
2800000 - 111.77 MiB - 10527 signals
2900000 - 111.44 MiB - 5933 signals
3000000 - 111.28 MiB - 1325 signals

[geeksilva97]

Ref: chromium/chromium@d5b7539 - I think it would be the "settled" case(?)

[siddhant0410]

Is anyone working on this issue?

[jasnell]

Can confirm that we have a problem on this, however, it is not AbortSignal instances that are being leaked, it's WeakRefs. Specifically, the set of dependent signals known to the AbortSignal are kept in an internal Set using WeakRefs. The AbortSignals are being properly gc'd but the Set is never cleaned out of the WeakRefs making those leak.

[geeksilva97]

Is anyone working on this issue?

I have a pr opened to address this issue.

[siddhant0410]

Is anyone working on this issue?

I have a pr opened to address this issue.

So if I want to work on this issue do I need to contact someone or I can start working right away?

[mika-fischer]

Is anyone working on this issue?
I have a pr opened to address this issue.
So if I want to work on this issue do I need to contact someone or I can start working right away?

@siddhant0410 The PR here #55354 by @geeksilva97 already fixes this issue, so there's nothing else to do here but wait for the PR to be merged and backported.

[siddhant0410]

Is anyone working on this issue?
I have a pr opened to address this issue.
So if I want to work on this issue do I need to contact someone or I can start working right away?

@siddhant0410 The PR here #55354 by @geeksilva97 already fixes this issue, so there's nothing else to do here but wait for the PR to be merged and backported.

Thanks for the update!