From cc5c8bb451b5169057df25dc4e98ea47ac2b5991 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Tue, 22 Dec 2020 18:21:12 +0200 Subject: [PATCH] lib: add weak event handlers --- .eslintrc.js | 2 + lib/events.js | 4 +- lib/internal/event_target.js | 59 ++++++++++++++----- .../test-events-static-geteventlisteners.js | 12 +++- test/parallel/test-eventtarget.js | 39 +++++++++++- 5 files changed, 96 insertions(+), 20 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index af1249eae6436e..7dce033e8d6507 100644 --- a/.eslintrc.js +++ b/.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', }, }; diff --git a/lib/events.js b/lib/events.js index dc08042578bc7a..4292a2dd666cfb 100644 --- a/lib/events.js +++ b/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; diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js index a0efe8c18e875a..cd93aaa923d91b 100644 --- a/lib/internal/event_target.js +++ b/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,11 +322,9 @@ 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); @@ -308,7 +332,8 @@ class EventTarget { 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, }; diff --git a/test/parallel/test-events-static-geteventlisteners.js b/test/parallel/test-events-static-geteventlisteners.js index d717ff74242f90..ab372f04d282dd 100644 --- a/test/parallel/test-events-static-geteventlisteners.js +++ b/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]); +} diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js index 45c63e0dd18be3..13b668d49d6e79 100644 --- a/test/parallel/test-eventtarget.js +++ b/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')); + }); +}