Skip to content

Commit

Permalink
lib: add weak event handlers
Browse files Browse the repository at this point in the history
PR-URL: #36607
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
  • Loading branch information
benjamingr authored and danielleadams committed Feb 16, 2021
1 parent 9dac99a commit acabe08
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 17 deletions.
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', () => {
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'));
});
}

0 comments on commit acabe08

Please sign in to comment.