fix: avoid mem leak

[ronag]

Refs: nodejs/node#46435
Refs: #1926

[ronag]

@jamesdiacono @stalkerg Can you try and see if this fixes your issue?

[ronag]

If this helps it almost feels like a V8 bug.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.55 ⚠️

Comparison is base (f0271d4) 90.45% compared to head (a3f6e87) 89.91%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2025      +/-   ##
==========================================
- Coverage   90.45%   89.91%   -0.55%     
==========================================
  Files          71       63       -8     
  Lines        6174     5610     -564     
==========================================
- Hits         5585     5044     -541     
+ Misses        589      566      -23     

Impacted Files	Coverage Δ
lib/fetch/request.js	88.18% <100.00%> (+0.15%)	⬆️

... and 15 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[stalkerg]

I will try today, not so easy however in my case.

[stalkerg]

No, it's not helped me.
If I back WeakRef to ac directly instead this as before, it's becoming ok.

[stalkerg]

I think the reason is somewhere here:

signal.addEventListener('abort', abort, { once: true })
requestFinalizer.register(this, { signal, abort })

Because our abort function borrows ac object, which is bounded to this by this[kSignal] = ac.signal.
I suppose this will not finalize, and requestFinalizer.register(this, { signal, abort }) never executed.

Also possible this:

this[kSignal] = ac.signal
this[kSignal][kRealm] = this[kRealm]

the reason why ac holds this.

[stalkerg]

Very interesting! If I comment this requestFinalizer.register(this, { signal, abort }) it's also help.

[stalkerg]

I found here https://v8.dev/features/weak-references totally the same example and explanation why WeakRef is needed:

const gListenersRegistry = new FinalizationRegistry(({ socket, wrapper }) => {
  socket.removeEventListener('message', wrapper); // 6
});

function addWeakListener(socket, listener) {
  const weakRef = new WeakRef(listener); // 2
  const wrapper = (ev) => { weakRef.deref()?.(ev); }; // 3
  gListenersRegistry.register(listener, { socket, wrapper }); // 4
  socket.addEventListener('message', wrapper); // 5
}

class MovingAvg {
  constructor(socket) {
    this.events = [];
    this.listener = (ev) => { this.events.push(ev); }; // 1
    addWeakListener(socket, this.listener);
  }
}

We make an event listener and assign it to this.listener so that it is strongly referenced by the MovingAvg instance (1). We then wrap the event listener that does the work in a WeakRef to make it garbage-collectible, and to not leak its reference to the MovingAvg instance via this (2). We make a wrapper that deref the WeakRef to check if it is still alive, then call it if so (3). We register the inner listener on the FinalizationRegistry, passing a holding value { socket, wrapper } to the registration (4). We then add the returned wrapper as an event listener on socket (5). Sometime after the MovingAvg instance and the inner listener are garbage-collected, the finalizer may run, with the holding value passed to it. Inside the finalizer, we remove the wrapper as well, making all memory associated with the use of a MovingAvg instance garbage-collectible (6).