From acabe08b10efeee68900e9356a3d4bcced103bb1 Mon Sep 17 00:00:00 2001 From: Benjamin Gruenbaum Date: Thu, 4 Feb 2021 12:17:44 +0200 Subject: [PATCH] lib: add weak event handlers PR-URL: https://github.com/nodejs/node/pull/36607 Reviewed-By: James M Snell Reviewed-By: Gus Caplan --- lib/events.js | 4 +- lib/internal/event_target.js | 66 +++++++++++++++---- .../test-events-static-geteventlisteners.js | 11 +++- test/parallel/test-eventtarget.js | 31 ++++++++- 4 files changed, 95 insertions(+), 17 deletions(-) diff --git a/lib/events.js b/lib/events.js index 7a7abc5c4b3339..797b5bc90c6bb8 100644 --- a/lib/events.js +++ b/lib/events.js @@ -697,7 +697,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 fe588088cc81f1..f79d1eb23faf70 100644 --- a/lib/internal/event_target.js +++ b/lib/internal/event_target.js @@ -15,11 +15,12 @@ const { ReflectApply, SafeArrayIterator, SafeMap, + SafeWeakMap, + SafeWeakSet, String, Symbol, SymbolFor, SymbolToStringTag, - SafeWeakSet, } = primordials; const { @@ -47,6 +48,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'); @@ -190,6 +192,21 @@ class NodeCustomEvent extends Event { } } } + +// Weak listener cleanup +// This has to be lazy for snapshots to work +let weakListenersState = null; +// The resource needs to retain the callback so that it doesn't +// get garbage collected now that it's weak. +let objectToWeakListenerMap = null; +function weakListeners() { + weakListenersState ??= new globalThis.FinalizationRegistry( + (listener) => listener.remove() + ); + objectToWeakListenerMap ??= new SafeWeakMap(); + return { registry: weakListenersState, map: objectToWeakListenerMap }; +} + // 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 @@ -198,7 +215,8 @@ 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; @@ -210,15 +228,26 @@ class Listener { this.passive = passive; this.isNodeStyleListener = isNodeStyleListener; this.removed = false; - - this.callback = - typeof listener === 'function' ? - listener : - FunctionPrototypeBind(listener.handleEvent, listener); + this.weak = Boolean(weak); // Don't retain the object + + if (this.weak) { + this.callback = new globalThis.WeakRef(listener); + weakListeners().registry.register(listener, this, this); + // Make the retainer retain the listener in a WeakMap + weakListeners().map.set(weak, listener); + this.listener = this.callback; + } else if (typeof listener === 'function') { + this.callback = listener; + this.listener = listener; + } else { + this.callback = FunctionPrototypeBind(listener.handleEvent, 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() { @@ -227,6 +256,8 @@ class Listener { if (this.next !== undefined) this.next.previous = this.previous; this.removed = true; + if (this.weak) + weakListeners().registry.unregister(this); } } @@ -277,7 +308,8 @@ class EventTarget { capture, passive, signal, - isNodeStyleListener + isNodeStyleListener, + weak, } = validateEventListenerOptions(options); if (!shouldAddListener(listener)) { @@ -302,7 +334,7 @@ class EventTarget { // not prevent the event target from GC. signal.addEventListener('abort', () => { this.removeEventListener(type, listener, options); - }, { once: true }); + }, { once: true, [kWeakHandler]: this }); } let root = this[kEvents].get(type); @@ -310,7 +342,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; @@ -330,7 +363,7 @@ class EventTarget { } new Listener(previous, listener, once, capture, passive, - isNodeStyleListener); + isNodeStyleListener, weak); root.size++; this[kNewListener](root.size, type, listener, once, capture, passive); } @@ -418,7 +451,12 @@ class EventTarget { } else { arg = createEvent(); } - const result = FunctionPrototypeCall(handler.callback, this, arg); + const callback = handler.weak ? + handler.callback.deref() : handler.callback; + let result; + if (callback) { + result = FunctionPrototypeCall(callback, this, arg); + } if (result !== undefined && result !== null) addCatch(this, result, createEvent()); } catch (err) { @@ -569,6 +607,7 @@ function validateEventListenerOptions(options) { capture: Boolean(options.capture), passive: Boolean(options.passive), signal: options.signal, + weak: options[kWeakHandler], isNodeStyleListener: Boolean(options[kIsNodeStyleListener]) }; } @@ -671,5 +710,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 71d9b0a28ddc75..eba92de4a00c90 100644 --- a/test/parallel/test-events-static-geteventlisteners.js +++ b/test/parallel/test-events-static-geteventlisteners.js @@ -1,6 +1,7 @@ 'use strict'; - +// Flags: --expose-internals --no-warnings const common = require('../common'); +const { kWeakHandler } = require('internal/event_target'); const { deepStrictEqual, @@ -41,3 +42,11 @@ const { getEventListeners, EventEmitter } = require('events'); getEventListeners('INVALID_EMITTER'); }, /ERR_INVALID_ARG_TYPE/); } +{ + // Test weak listeners + const target = new EventTarget(); + const fn = common.mustNotCall(); + target.addEventListener('foo', fn, { [kWeakHandler]: {} }); + const listeners = getEventListeners(target, 'foo'); + deepStrictEqual(listeners, [fn]); +} diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js index dd3a5a106b5a01..1789d794f41ea3 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, @@ -570,3 +573,27 @@ let asyncTest = Promise.resolve(); const et = new EventTarget(); strictEqual(et.constructor.name, 'EventTarget'); } +{ + // Weak event handlers work + const et = new EventTarget(); + const listener = common.mustCall(); + et.addEventListener('foo', listener, { [kWeakHandler]: et }); + 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]: et }); + et.removeEventListener('foo', listener); + et.dispatchEvent(new Event('foo')); +} +{ + // Test listeners are held weakly + const et = new EventTarget(); + et.addEventListener('foo', common.mustNotCall(), { [kWeakHandler]: {} }); + setImmediate(() => { + global.gc(); + et.dispatchEvent(new Event('foo')); + }); +}