events: fix weak listener retention overwrite

Project: nodejs/node | Language: JavaScript | Concepts: memory-management, garbage-collection, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/64024

The problem

The weakListeners() retention map in EventTarget stored only a single listener per retainer key. When multiple listeners shared the same retainer (for example, two listeners on one target using the same AbortSignal), they would silently overwrite each other's strong references in the map. This caused evicted listeners to be garbage collected prematurely before their signal aborted. As a result, only the most recently registered listener was actually removed on abort, leading to incorrect behavior.

Why it's non-trivial

A naive fix might simply replace the single listener reference with another, but this would not support multiple listeners sharing the same retainer. The fix requires changing the data structure to hold multiple listeners per retainer and ensuring proper cleanup when listeners are removed. Additionally, the fix must maintain weak retention semantics to avoid memory leaks, and must also fix similar bugs in related code paths like events.aborted() and streams using kWeakHandler.

The approach

The core change is modifying the weak retention map from storing a single listener per retainer key to storing a Set of listeners per retainer key. This allows multiple listeners sharing the same retainer to coexist without overwriting each other.

+  SafeSet,

• Added SafeSet import to use a set for storing multiple listeners.

-      // Make the retainer retain the listener in a WeakMap
-      weakListeners().map.set(weak, listener);
+      // Store only a WeakRef to the retainer here. Listener instances are
+      // Strongly reachable from the EventTarget's listener list for as long
+      // as they're registered, so a plain strong reference would keep the
+      // retainer (and listener) alive forever, defeating weak retention
+      this.weakKeyRef = new SafeWeakRef(weak);
+      let retained = weakListeners().map.get(weak);
+      if (retained === undefined) {
+        retained = new SafeSet();
+        weakListeners().map.set(weak, retained);
+      }
+      retained.add(listener);

• Instead of mapping a retainer key (weak) to a single listener, the code now maps it to a SafeSet of listeners.
• If no set exists for the retainer, a new SafeSet is created and stored.
• The current listener is added to the set.
• A SafeWeakRef to the retainer is stored on the listener instance to allow cleanup later.

-    if (this.weak)
-      weakListeners().registry.unregister(this);
+    if (this.weak) {
+      weakListeners().registry.unregister(this);
+      const weakKey = this.weakKeyRef?.deref();
+      const listener = this.callback?.deref();
+      if (weakKey !== undefined && listener !== undefined) {
+        const retained = weakListeners().map.get(weakKey);
+        if (retained !== undefined) {
+          retained.delete(listener);
+          if (retained.size === 0)
+            weakListeners().map.delete(weakKey);
+        }
+      }
+    }

• When a weak listener is removed, the code unregisters it from the finalization registry.
• It then dereferences the weak references to the retainer and listener.
• If both are still alive, it removes the listener from the retainer's set.
• If the set becomes empty, the retainer key is removed from the map to avoid memory leaks.

Tests added to verify the fix:

test('two weak listeners with the same retainer should both run on abort', async () => {
  // Regression test for https://github.com/nodejs/node/issues/63954
  const ac = new AbortController();
  let aRan = false;
  let bRan = false;

  ac.signal.addEventListener('a', () => { aRan = true; }, { [kWeakHandler]: ac });
  ac.signal.addEventListener('b', () => { bRan = true; }, { [kWeakHandler]: ac });

  await sleep(10);
  globalThis.gc();

  ac.signal.dispatchEvent(new Event('a'));
  ac.signal.dispatchEvent(new Event('b'));

  assert.strictEqual(aRan, true);
  assert.strictEqual(bRan, true);
});

• This test confirms that two weak listeners sharing the same retainer both run on abort, fixing the bug where only one would run.

{
  // Two listeners sharing the same retainer key must NOT evict each
  // other from the weak retention map — both must survive a GC cycle
  // and both must be removable independently.
  // Regression test for https://github.com/nodejs/node/issues/63954
  const et = new EventTarget();
  const aCalled = common.mustNotCall();
  const bCalled = common.mustCall();
  et.addEventListener('a', aCalled, { [kWeakHandler]: et });
  et.addEventListener('b', bCalled, { [kWeakHandler]: et });
  globalThis.gc();
  et.removeEventListener('a', aCalled);
  et.dispatchEvent(new Event('a'));
  et.dispatchEvent(new Event('b'));
}

• This test verifies that after garbage collection, removing one listener does not affect the other, and both behave correctly.

From the review thread

No notable review discussion on this PR.

Takeaways

• When multiple entities share a key in a weak retention map, store a collection (e.g., a Set) rather than a single value to avoid overwriting and premature garbage collection.
• Always clean up entries in retention maps when their associated collections become empty to prevent memory leaks.
• Use weak references carefully to maintain correct object lifetimes without preventing garbage collection, especially in event listener patterns.