lib: add weak event handlers

.eslintrc.js

@@ -299,12 +299,14 @@ module.exports = {
     BigUint64Array: 'readable',
     Event: 'readable',
     EventTarget: 'readable',
+    FinalizationRegistry: 'readable',
     MessageChannel: 'readable',
     MessageEvent: 'readable',
     MessagePort: 'readable',
     TextEncoder: 'readable',
     TextDecoder: 'readable',
     queueMicrotask: 'readable',
+    WeakRef: 'readable',
     globalThis: 'readable',
   },
 };

lib/events.js

@@ -695,7 +695,9 @@ function getEventListeners(emitterOrTarget, type) {
     const listeners = [];
     let handler = root?.next;
     while (handler?.listener !== undefined) {
-      ArrayPrototypePush(listeners, handler.listener);
+      const listener = handler.listener?.deref ?
+        handler.listener.deref() : handler.listener;
+      ArrayPrototypePush(listeners, listener);
       handler = handler.next;
     }
     return listeners;

lib/internal/event_target.js

@@ -45,6 +45,7 @@ const kEvents = Symbol('kEvents');
 const kStop = Symbol('kStop');
 const kTarget = Symbol('kTarget');
 const kHandlers = Symbol('khandlers');
+const kWeakHandler = Symbol('kWeak');
 
 const kHybridDispatch = SymbolFor('nodejs.internal.kHybridDispatch');
 const kCreateEvent = Symbol('kCreateEvent');
@@ -188,6 +189,19 @@ class NodeCustomEvent extends Event {
     }
   }
 }
+
+// Weak listener cleanup
+// This has to be lazy for snapshots to work
+let weakListenersState = null;
+function weakListeners() {
+  if (!weakListenersState) {
+    weakListenersState = new FinalizationRegistry(
+      (listener) => listener.remove()
+    );
+  }
+  return weakListenersState;
+}
+
 // The listeners for an EventTarget are maintained as a linked list.
 // Unfortunately, the way EventTarget is defined, listeners are accounted
 // using the tuple [handler,capture], and even if we don't actually make
@@ -196,27 +210,36 @@ class NodeCustomEvent extends Event {
 // the linked list makes dispatching faster, even if adding/removing is
 // slower.
 class Listener {
-  constructor(previous, listener, once, capture, passive, isNodeStyleListener) {
+  constructor(previous, listener, once, capture, passive,
+              isNodeStyleListener, weak) {
     this.next = undefined;
     if (previous !== undefined)
       previous.next = this;
     this.previous = previous;
-    this.listener = listener;
     // TODO(benjamingr) these 4 can be 'flags' to save 3 slots
     this.once = once;
     this.capture = capture;
     this.passive = passive;
     this.isNodeStyleListener = isNodeStyleListener;
     this.removed = false;
-
-    this.callback =
-      typeof listener === 'function' ?
-        listener :
-        listener.handleEvent.bind(listener);
+    this.weak = weak;
+
+    if (this.weak) {
+      this.callback = new WeakRef(listener);
+      weakListeners().register(listener, this, this);
+      this.listener = this.callback;
+    } else if (typeof listener === 'function') {
+      this.callback = listener;
+      this.listener = listener;
+    } else {
+      this.callback = listener.handleEvent.bind(listener);
+      this.listener = listener;
+    }
   }
 
   same(listener, capture) {
-    return this.listener === listener && this.capture === capture;
+    const myListener = this.weak ? this.listener.deref() : this.listener;
+    return myListener === listener && this.capture === capture;
   }
 
   remove() {
@@ -225,6 +248,8 @@ class Listener {
     if (this.next !== undefined)
       this.next.previous = this.previous;
     this.removed = true;
+    if (this.weak)
+      weakListeners().unregister(this);
   }
 }
 
@@ -275,7 +300,8 @@ class EventTarget {
       capture,
       passive,
       signal,
-      isNodeStyleListener
+      isNodeStyleListener,
+      weak,
     } = validateEventListenerOptions(options);
 
     if (!shouldAddListener(listener)) {
@@ -296,19 +322,18 @@ class EventTarget {
       if (signal.aborted) {
         return false;
       }
-      // TODO(benjamingr) make this weak somehow? ideally the signal would
-      // not prevent the event target from GC.
       signal.addEventListener('abort', () => {
         this.removeEventListener(type, listener, options);
-      }, { once: true });
+      }, { once: true, [kWeakHandler]: true });
     }
 
     let root = this[kEvents].get(type);
 
     if (root === undefined) {
       root = { size: 1, next: undefined };
       // This is the first handler in our linked list.
-      new Listener(root, listener, once, capture, passive, isNodeStyleListener);
+      new Listener(root, listener, once, capture, passive,
+                   isNodeStyleListener, weak);
       this[kNewListener](root.size, type, listener, once, capture, passive);
       this[kEvents].set(type, root);
       return;
@@ -328,7 +353,7 @@ class EventTarget {
     }
 
     new Listener(previous, listener, once, capture, passive,
-                 isNodeStyleListener);
+                 isNodeStyleListener, weak);
     root.size++;
     this[kNewListener](root.size, type, listener, once, capture, passive);
   }
@@ -419,7 +444,9 @@ class EventTarget {
         } else {
           arg = createEvent();
         }
-        const result = handler.callback.call(this, arg);
+        const callback = handler.weak ?
+          handler.callback.deref() : handler.callback;
+        const result = callback?.call(this, arg);
         if (result !== undefined && result !== null)
           addCatch(this, result, createEvent());
       } catch (err) {
@@ -573,6 +600,7 @@ function validateEventListenerOptions(options) {
     once: Boolean(options.once),
     capture: Boolean(options.capture),
     passive: Boolean(options.passive),
+    weak: Boolean(options[kWeakHandler]),
     signal: options.signal,
     isNodeStyleListener: Boolean(options[kIsNodeStyleListener])
   };
@@ -675,5 +703,6 @@ module.exports = {
   kTrustEvent,
   kRemoveListener,
   kEvents,
+  kWeakHandler,
   isEventTarget,
 };

test/parallel/test-events-static-geteventlisteners.js

@@ -1,7 +1,7 @@
 'use strict';
-
+// Flags: --expose-internals --no-warnings
 const common = require('../common');
-
+const { kWeakHandler } = require('internal/event_target');
 const {
   deepStrictEqual,
 } = require('assert');
@@ -34,3 +34,11 @@ const { getEventListeners, EventEmitter } = require('events');
   deepStrictEqual(getEventListeners(target, 'bar'), []);
   deepStrictEqual(getEventListeners(target, 'baz'), [fn1]);
 }
+{
+  // Test weak listeners
+  const target = new EventTarget();
+  const fn = common.mustNotCall();
+  target.addEventListener('foo', fn, { [kWeakHandler]: true });
+  const listeners = getEventListeners(target, 'foo');
+  deepStrictEqual(listeners, [fn]);
+}

test/parallel/test-eventtarget.js

@@ -1,8 +1,11 @@
-// Flags: --expose-internals --no-warnings
+// Flags: --expose-internals --no-warnings --expose-gc
 'use strict';
 
 const common = require('../common');
-const { defineEventHandler } = require('internal/event_target');
+const {
+  defineEventHandler,
+  kWeakHandler
+} = require('internal/event_target');
 
 const {
   ok,
@@ -541,3 +544,35 @@ let asyncTest = Promise.resolve();
   et.addEventListener('foo', listener);
   et.dispatchEvent(new Event('foo'));
 }
+{
+  // Weak event handlers work
+  const et = new EventTarget();
+  const listener = common.mustCall();
+  et.addEventListener('foo', listener, { [kWeakHandler]: true });
+  et.dispatchEvent(new Event('foo'));
+}
+{
+  // Weak event handlers can be removed and weakness is not part of the key
+  const et = new EventTarget();
+  const listener = common.mustNotCall();
+  et.addEventListener('foo', listener, { [kWeakHandler]: true });
+  et.removeEventListener('foo', listener);
+  et.dispatchEvent(new Event('foo'));
+}
+{
+  // Weak event handlers can be removed and weakness is not part of the key
+  const et = new EventTarget();
+  const listener = common.mustNotCall();
+  et.addEventListener('foo', listener, { [kWeakHandler]: true });
+  et.removeEventListener('foo', listener);
+  et.dispatchEvent(new Event('foo'));
+}
+{
+  // Test listeners are held weakly
+  const et = new EventTarget();
+  et.addEventListener('foo', common.mustNotCall(), { [kWeakHandler]: true });
+  setImmediate(() => {
+    global.gc();
+    et.dispatchEvent(new Event('foo'));
+  });
+}