events: support signal in EventTarget #36258

benjamingr · 2020-11-25T13:02:02Z

This is this feature whatwg/dom#911 I've implemented the spec I worked on in whatwg/dom#919 and ported the WPTs from web-platform-tests/wpt#26472

This is following implementations from Chrome and Firefox and positive signals regarding the spec. I am happy to put this behind an experimental warning if you think that's a good idea.

Ping @jasnell

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

benjamingr · 2020-11-25T13:02:44Z

lib/internal/event_target.js

@@ -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 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.

lib/internal/event_target.js

benjamingr · 2020-11-25T13:06:00Z

lib/internal/event_target.js

+      }
+      // TODO(benjamingr) make this weak somehow? ideally the signal would
+      // not prevent the event target from GC.
+      signal.addEventListener('abort', () => {


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)

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 :)

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

@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).

nodejs-github-bot · 2020-11-25T13:18:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34552/

nodejs-github-bot · 2020-11-25T17:35:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34560/

benjamingr · 2020-11-29T19:50:04Z

Can this get a review @jasnell @addaleax ?

benjamingr · 2020-12-01T17:21:39Z

Kind of embarrassing that all the browsers are going to have this feature before us 😅 Though I am happy to wait a few more days for the whatwg/dom PR to be merged if that's a concern.

github-actions · 2020-12-02T14:55:14Z

Landed in 4a741b8...ef8d0e9

PR-URL: #36258 Fixes: #36073 Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36435 Notable changes: * child_processes: * add AbortSignal support (Benjamin Gruenbaum) (#36308) * deps: * update ICU to 68.1 (Michaël Zasso) (#36187) * events: * support signal in EventTarget (Benjamin Gruenbaum) (#36258) * graduate Event, EventTarget, AbortController (James M Snell) (#35949) * http: * enable call chaining with setHeader() (pooja d.p) (#35924) * module: * add isPreloading indicator (James M Snell) (#36263) * stream: * support abort signal (Benjamin Gruenbaum) (#36061) * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922) * worker: * add experimental BroadcastChannel (James M Snell) (#36271)

PR-URL: nodejs#36258 Fixes: nodejs#36073 Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36435 Notable changes: * child_processes: * add AbortSignal support (Benjamin Gruenbaum) (#36308) * deps: * update ICU to 68.1 (Michaël Zasso) (#36187) * events: * support signal in EventTarget (Benjamin Gruenbaum) (#36258) * graduate Event, EventTarget, AbortController (James M Snell) (#35949) * http: * enable call chaining with setHeader() (pooja d.p) (#35924) * module: * add isPreloading indicator (James M Snell) (#36263) * stream: * support abort signal (Benjamin Gruenbaum) (#36061) * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922) * worker: * add experimental BroadcastChannel (James M Snell) (#36271)

targos · 2021-08-04T12:03:40Z

This pull request lands cleanly on v14.x-staging, but I think the EventTarget constructor is not exposed, so I don't know how if we should adapt the tests somehow or just don't land it.

benjamingr · 2021-08-04T14:15:01Z

@targos I think it's fine not to land given EventTarget is not exposed though it's also fine to land (no harm) in order to make backporting future stuff easier. As you noted the constructor is not exposed so there should not be any user impact.

Refs: nodejs#36258

Refs: #36258 PR-URL: #43170 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Masashi Hirano <shisama07@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Refs: nodejs/node#36258 PR-URL: nodejs/node#43170 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Masashi Hirano <shisama07@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

events: support signal in EventTarget

b70fdff

benjamingr added events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. labels Nov 25, 2020

benjamingr commented Nov 25, 2020

View reviewed changes

lib/internal/event_target.js Show resolved Hide resolved

benjamingr commented Nov 25, 2020

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2020

benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 25, 2020

jasnell approved these changes Dec 1, 2020

View reviewed changes

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 2, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 2, 2020

github-actions bot closed this Dec 2, 2020

nodejs-github-bot pushed a commit that referenced this pull request Dec 2, 2020

events: support signal in EventTarget

ef8d0e9

PR-URL: #36258 Fixes: #36073 Reviewed-By: James M Snell <jasnell@gmail.com>

benjamingr deleted the event-target-signal branch December 2, 2020 15:10

danielleadams pushed a commit that referenced this pull request Dec 7, 2020

events: support signal in EventTarget

855a85c

PR-URL: #36258 Fixes: #36073 Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams mentioned this pull request Dec 7, 2020

v15.4.0 proposal #36435

Merged

cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020

events: support signal in EventTarget

f59c6cd

PR-URL: nodejs#36258 Fixes: nodejs#36073 Reviewed-By: James M Snell <jasnell@gmail.com>

targos added the backport-requested-v14.x label Aug 4, 2021

aduh95 added a commit to aduh95/node that referenced this pull request May 21, 2022

doc: document signal option for EventTarget#addEventListener

096d43b

Refs: nodejs#36258

aduh95 mentioned this pull request May 21, 2022

doc: document signal option for EventTarget#addEventListener #43170

Merged

This was referenced Oct 3, 2023

api.EventTarget.addEventListener - options.signal is supported on node.js mdn/browser-compat-data#18883

Closed

Updated Node support for EventTarget mdn/browser-compat-data#20889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: support signal in EventTarget #36258

events: support signal in EventTarget #36258

benjamingr commented Nov 25, 2020

benjamingr Nov 25, 2020

benjamingr Nov 25, 2020

addaleax Nov 25, 2020

jasnell Nov 25, 2020

benjamingr Nov 25, 2020

nodejs-github-bot commented Nov 25, 2020

nodejs-github-bot commented Nov 25, 2020

benjamingr commented Nov 29, 2020

benjamingr commented Dec 1, 2020

github-actions bot commented Dec 2, 2020

targos commented Aug 4, 2021

benjamingr commented Aug 4, 2021

events: support signal in EventTarget #36258

events: support signal in EventTarget #36258

Conversation

benjamingr commented Nov 25, 2020

Checklist

benjamingr Nov 25, 2020

Choose a reason for hiding this comment

benjamingr Nov 25, 2020

Choose a reason for hiding this comment

addaleax Nov 25, 2020

Choose a reason for hiding this comment

jasnell Nov 25, 2020

Choose a reason for hiding this comment

benjamingr Nov 25, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 25, 2020

nodejs-github-bot commented Nov 25, 2020

benjamingr commented Nov 29, 2020

benjamingr commented Dec 1, 2020

github-actions bot commented Dec 2, 2020

targos commented Aug 4, 2021

benjamingr commented Aug 4, 2021