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: support signal in EventTarget #36258

Closed
wants to merge 1 commit into from
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
22 changes: 22 additions & 0 deletions lib/internal/event_target.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,12 @@ class Listener {
previous.next = this;
this.previous = previous;
this.listener = listener;
// TODO(benjamingr) these 4 can be 'flags' to save 3 slots
Copy link
Member Author

Choose a reason for hiding this comment

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

This is in order to fix an unrelated bug where we cache the next handler in our dispatchEvent and still call it. Added a test or this case below.

this.once = once;
this.capture = capture;
this.passive = passive;
this.isNodeStyleListener = isNodeStyleListener;
this.removed = false;

this.callback =
typeof listener === 'function' ?
Expand All @@ -220,6 +222,7 @@ class Listener {
this.previous.next = this.next;
if (this.next !== undefined)
this.next.previous = this.previous;
this.removed = true;
}
}

Expand Down Expand Up @@ -269,6 +272,7 @@ class EventTarget {
once,
capture,
passive,
signal,
isNodeStyleListener
} = validateEventListenerOptions(options);

Expand All @@ -286,6 +290,17 @@ class EventTarget {
}
type = String(type);

if (signal) {
if (signal.aborted) {
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
// TODO(benjamingr) make this weak somehow? ideally the signal would
// not prevent the event target from GC.
signal.addEventListener('abort', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW: we should really add support for "weak" event listeners that is:

signal[kAddWeakListener]('abort', () => {
  this.removeEventListener(type, listener, options);
});

That would not retain the event target only because the signal exists (since if no one cares about the event retaining it for abort is mostly meaningless). I think a good path forward is:

  • Make our listener list accept WeakRefs to listeners
  • Put said listener behind a WeakRef
  • Make our dispatch unwrap WeakRefs when it finds them.

@addaleax @jasnell if you think that's reasonable I'm happy to do this (either here or in a new PR)

Copy link
Member

Choose a reason for hiding this comment

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

I think that’s reasonable here, yes… it’s way more common to think that you need a weak listener than it is to actually need one, so I wouldn’t expose this as public API, but for internal usage this sounds good :)

Copy link
Member

Choose a reason for hiding this comment

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

My only concern there would be the potential additional performance cost. If the dispatch only incurs that cost for weak handlers then I'm definitely +1

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell great, I'll do that at a separate pull request then.

@addaleax there are discussions for other use cases for this e.g. creating linked signals whatwg/dom#920 - there is a good explanation here: #35990 (comment)

I think we're running into the same issue with AbortSignal multiple times: If the resource will no longer be aborted and we're "done" with it we need to make sure that retaining the signal does not retain the resource. With some resources (like http.request, streams in general and timers) this is a relatively easy problem since we know when abort will be meaningless but with addEventListener and other resources this is less clear cut (hence the advantage of a weak handler to not have to manage this).

this.removeEventListener(type, listener, options);
}, { once: true });
}

let root = this[kEvents].get(type);

if (root === undefined) {
Expand Down Expand Up @@ -382,6 +397,12 @@ class EventTarget {
// Cache the next item in case this iteration removes the current one
next = handler.next;

if (handler.removed) {
// Deal with the case an event is removed while event handlers are
// Being processed (removeEventListener called from a listener)
handler = next;
continue;
}
if (handler.once) {
handler.remove();
root.size--;
Expand Down Expand Up @@ -550,6 +571,7 @@ function validateEventListenerOptions(options) {
once: Boolean(options.once),
capture: Boolean(options.capture),
passive: Boolean(options.passive),
signal: options.signal,
isNodeStyleListener: Boolean(options[kIsNodeStyleListener])
};
}
Expand Down
155 changes: 155 additions & 0 deletions test/parallel/test-eventtarget-whatwg-signal.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
'use strict';

require('../common');

const {
strictEqual,
} = require('assert');

// Manually ported from: wpt@dom/events/AddEventListenerOptions-signal.any.js

{
// Passing an AbortSignal to addEventListener does not prevent
// removeEventListener
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
et.addEventListener('test', handler, { signal: controller.signal });
et.dispatchEvent(new Event('test'));
strictEqual(count, 1, 'Adding a signal still adds a listener');
et.dispatchEvent(new Event('test'));
strictEqual(count, 2, 'The listener was not added with the once flag');
controller.abort();
et.dispatchEvent(new Event('test'));
strictEqual(count, 2, 'Aborting on the controller removes the listener');
et.addEventListener('test', handler, { signal: controller.signal });
et.dispatchEvent(new Event('test'));
strictEqual(count, 2, 'Passing an aborted signal never adds the handler');
}

{
// Passing an AbortSignal to addEventListener works with the once flag
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
et.addEventListener('test', handler, { signal: controller.signal });
et.removeEventListener('test', handler);
et.dispatchEvent(new Event('test'));
strictEqual(count, 0, 'The listener was still removed');
}

{
// Removing a once listener works with a passed signal
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
const options = { signal: controller.signal, once: true };
et.addEventListener('test', handler, options);
controller.abort();
et.dispatchEvent(new Event('test'));
strictEqual(count, 0, 'The listener was still removed');
}

{
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
const options = { signal: controller.signal, once: true };
et.addEventListener('test', handler, options);
et.removeEventListener('test', handler);
et.dispatchEvent(new Event('test'));
strictEqual(count, 0, 'The listener was still removed');
}

{
// Passing an AbortSignal to multiple listeners
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
const options = { signal: controller.signal, once: true };
et.addEventListener('first', handler, options);
et.addEventListener('second', handler, options);
controller.abort();
et.dispatchEvent(new Event('first'));
et.dispatchEvent(new Event('second'));
strictEqual(count, 0, 'The listener was still removed');
}

{
// Passing an AbortSignal to addEventListener works with the capture flag
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
const options = { signal: controller.signal, capture: true };
et.addEventListener('test', handler, options);
controller.abort();
et.dispatchEvent(new Event('test'));
strictEqual(count, 0, 'The listener was still removed');
}

{
// Aborting from a listener does not call future listeners
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
const options = { signal: controller.signal };
et.addEventListener('test', () => {
controller.abort();
}, options);
et.addEventListener('test', handler, options);
et.dispatchEvent(new Event('test'));
strictEqual(count, 0, 'The listener was still removed');
}

{
// Adding then aborting a listener in another listener does not call it
let count = 0;
function handler() {
count++;
}
const et = new EventTarget();
const controller = new AbortController();
et.addEventListener('test', () => {
et.addEventListener('test', handler, { signal: controller.signal });
controller.abort();
}, { signal: controller.signal });
et.dispatchEvent(new Event('test'));
strictEqual(count, 0, 'The listener was still removed');
}

{
// Aborting from a nested listener should remove it
const et = new EventTarget();
const ac = new AbortController();
let count = 0;
et.addEventListener('foo', () => {
et.addEventListener('foo', () => {
count++;
if (count > 5) ac.abort();
et.dispatchEvent(new Event('foo'));
}, { signal: ac.signal });
et.dispatchEvent(new Event('foo'));
}, { once: true });
et.dispatchEvent(new Event('foo'));
}
9 changes: 9 additions & 0 deletions test/parallel/test-eventtarget.js
Original file line number Diff line number Diff line change
Expand Up @@ -532,3 +532,12 @@ let asyncTest = Promise.resolve();
target.dispatchEvent(new Event('foo'));
deepStrictEqual(output, [1, 2, 3, 4]);
}
{
const et = new EventTarget();
const listener = common.mustNotCall();
et.addEventListener('foo', common.mustCall((e) => {
et.removeEventListener('foo', listener);
}));
et.addEventListener('foo', listener);
et.dispatchEvent(new Event('foo'));
}