Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: add weak event handlers #36607

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
66 changes: 53 additions & 13 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ const {
ReflectApply,
SafeArrayIterator,
SafeMap,
SafeWeakMap,
SafeWeakSet,
String,
Symbol,
SymbolFor,
SymbolToStringTag,
SafeWeakSet,
} = primordials;

const {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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() {
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -277,7 +308,8 @@ class EventTarget {
capture,
passive,
signal,
isNodeStyleListener
isNodeStyleListener,
weak,
} = validateEventListenerOptions(options);

if (!shouldAddListener(listener)) {
Expand All @@ -302,15 +334,16 @@ class EventTarget {
// not prevent the event target from GC.
signal.addEventListener('abort', () => {
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
this.removeEventListener(type, listener, options);
}, { once: true });
}, { once: true, [kWeakHandler]: this });
}

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;
Expand All @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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])
};
}
Expand Down Expand Up @@ -671,5 +710,6 @@ module.exports = {
kTrustEvent,
kRemoveListener,
kEvents,
kWeakHandler,
isEventTarget,
};
11 changes: 10 additions & 1 deletion test/parallel/test-events-static-geteventlisteners.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

// Flags: --expose-internals --no-warnings
const common = require('../common');
const { kWeakHandler } = require('internal/event_target');

const {
deepStrictEqual,
Expand Down Expand Up @@ -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]);
}
31 changes: 29 additions & 2 deletions test/parallel/test-eventtarget.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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'));
});
}