-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
this.once = once; | ||
this.capture = capture; | ||
this.passive = passive; | ||
this.isNodeStyleListener = isNodeStyleListener; | ||
this.removed = false; | ||
|
||
this.callback = | ||
typeof listener === 'function' ? | ||
|
@@ -220,6 +222,7 @@ class Listener { | |
this.previous.next = this.next; | ||
if (this.next !== undefined) | ||
this.next.previous = this.previous; | ||
this.removed = true; | ||
} | ||
} | ||
|
||
|
@@ -269,6 +272,7 @@ class EventTarget { | |
once, | ||
capture, | ||
passive, | ||
signal, | ||
isNodeStyleListener | ||
} = validateEventListenerOptions(options); | ||
|
||
|
@@ -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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
@addaleax @jasnell if you think that's reasonable I'm happy to do this (either here or in a new PR) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
this.removeEventListener(type, listener, options); | ||
}, { once: true }); | ||
} | ||
|
||
let root = this[kEvents].get(type); | ||
|
||
if (root === undefined) { | ||
|
@@ -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--; | ||
|
@@ -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]) | ||
}; | ||
} | ||
|
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')); | ||
} |
There was a problem hiding this comment.
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.