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

events: remove weak listener for event target #48952

Merged
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
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Suggested change
await setTimeout(0);
await Promise.resolve();

Copy link
Member Author

@rluvaton rluvaton Aug 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it won't GC with this

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