Skip to content

Commit

Permalink
events: remove weak listener for event target
Browse files Browse the repository at this point in the history
Fixes: nodejs#48951
PR-URL: nodejs#48952
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
  • Loading branch information
rluvaton authored and Ceres6 committed Aug 14, 2023
1 parent 3ead299 commit b1a29cf
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 8 deletions.
39 changes: 34 additions & 5 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ const kWeakHandler = Symbol('kWeak');
const kResistStopPropagation = Symbol('kResistStopPropagation');

const kHybridDispatch = SymbolFor('nodejs.internal.kHybridDispatch');
const kRemoveWeakListenerHelper = Symbol('nodejs.internal.removeWeakListenerHelper');
const kCreateEvent = Symbol('kCreateEvent');
const kNewListener = Symbol('kNewListener');
const kRemoveListener = Symbol('kRemoveListener');
Expand Down Expand Up @@ -406,7 +407,7 @@ let weakListenersState = null;
let objectToWeakListenerMap = null;
function weakListeners() {
weakListenersState ??= new SafeFinalizationRegistry(
(listener) => listener.remove(),
({ eventTarget, listener, eventType }) => eventTarget.deref()?.[kRemoveWeakListenerHelper](eventType, listener),
);
objectToWeakListenerMap ??= new SafeWeakMap();
return { registry: weakListenersState, map: objectToWeakListenerMap };
Expand All @@ -428,7 +429,7 @@ const kFlagResistStopPropagation = 1 << 6;
// the linked list makes dispatching faster, even if adding/removing is
// slower.
class Listener {
constructor(previous, listener, once, capture, passive,
constructor(eventTarget, eventType, previous, listener, once, capture, passive,
isNodeStyleListener, weak, resistStopPropagation) {
this.next = undefined;
if (previous !== undefined)
Expand All @@ -455,7 +456,13 @@ class Listener {

if (this.weak) {
this.callback = new SafeWeakRef(listener);
weakListeners().registry.register(listener, this, this);
weakListeners().registry.register(listener, {
__proto__: null,
// Weak ref so the listener won't hold the eventTarget alive
eventTarget: new SafeWeakRef(eventTarget),
listener: this,
eventType,
}, this);
// Make the retainer retain the listener in a WeakMap
weakListeners().map.set(weak, listener);
this.listener = this.callback;
Expand Down Expand Up @@ -621,7 +628,7 @@ class EventTarget {
if (root === undefined) {
root = { size: 1, next: undefined, resistStopPropagation: Boolean(resistStopPropagation) };
// This is the first handler in our linked list.
new Listener(root, listener, once, capture, passive,
new Listener(this, type, root, listener, once, capture, passive,
isNodeStyleListener, weak, resistStopPropagation);
this[kNewListener](
root.size,
Expand All @@ -648,7 +655,7 @@ class EventTarget {
return;
}

new Listener(previous, listener, once, capture, passive,
new Listener(this, type, previous, listener, once, capture, passive,
isNodeStyleListener, weak, resistStopPropagation);
root.size++;
root.resistStopPropagation ||= Boolean(resistStopPropagation);
Expand Down Expand Up @@ -691,6 +698,28 @@ class EventTarget {
}
}

[kRemoveWeakListenerHelper](type, listener) {
const root = this[kEvents].get(type);
if (root === undefined || root.next === undefined)
return;

const capture = listener.capture === true;

let handler = root.next;
while (handler !== undefined) {
if (handler === listener) {
handler.remove();
root.size--;
if (root.size === 0)
this[kEvents].delete(type);
// Undefined is passed as the listener as the listener was GCed
this[kRemoveListener](root.size, type, undefined, capture);
break;
}
handler = handler.next;
}
}

/**
* @param {Event} event
*/
Expand Down
21 changes: 20 additions & 1 deletion test/parallel/test-eventtarget-memoryleakwarning.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
// Flags: --no-warnings
// Flags: --expose-internals --no-warnings --expose-gc
'use strict';
const common = require('../common');
const {
setMaxListeners,
EventEmitter,
} = require('events');
const assert = require('assert');
const { kWeakHandler } = require('internal/event_target');
const { setTimeout } = require('timers/promises');

common.expectWarning({
MaxListenersExceededWarning: [
Expand Down Expand Up @@ -73,3 +75,20 @@ common.expectWarning({
setMaxListeners(2, ee);
assert.strictEqual(ee.getMaxListeners(), 2);
}

{
(async () => {
// Test that EventTarget listener don't emit MaxListenersExceededWarning for weak listeners that GCed
const et = new EventTarget();
setMaxListeners(2, et);

for (let i = 0; i <= 3; i++) {
et.addEventListener('foo', () => {}, {
[kWeakHandler]: {},
});

await setTimeout(0);
global.gc();
}
})().then(common.mustCall(), common.mustNotCall());
}
4 changes: 2 additions & 2 deletions test/parallel/test-eventtarget.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ const {

const { once } = require('events');

const { promisify, inspect } = require('util');
const delay = promisify(setTimeout);
const { inspect } = require('util');
const { setTimeout: delay } = require('timers/promises');

// The globals are defined.
ok(Event);
Expand Down

0 comments on commit b1a29cf

Please sign in to comment.