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

Conversation

benjamingr
Copy link
Member

Fixes: #36073

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

}
// 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).

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

Can this get a review @jasnell @addaleax ?

@benjamingr
Copy link
Member Author

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.

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

github-actions bot commented Dec 2, 2020

Landed in 4a741b8...ef8d0e9

@github-actions github-actions bot closed this Dec 2, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 2, 2020
PR-URL: #36258
Fixes: #36073
Reviewed-By: James M Snell <[email protected]>
@benjamingr benjamingr deleted the event-target-signal branch December 2, 2020 15:10
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
PR-URL: #36258
Fixes: #36073
Reviewed-By: James M Snell <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
danielleadams added a commit that referenced this pull request Dec 7, 2020
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)
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
danielleadams added a commit that referenced this pull request Dec 9, 2020
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)
danielleadams added a commit that referenced this pull request Dec 9, 2020
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
Copy link
Member

targos commented Aug 4, 2021

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
Copy link
Member Author

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

aduh95 added a commit to aduh95/node that referenced this pull request May 21, 2022
nodejs-github-bot pushed a commit that referenced this pull request May 25, 2022
Refs: #36258

PR-URL: #43170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
bengl pushed a commit that referenced this pull request May 30, 2022
Refs: #36258

PR-URL: #43170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Refs: #36258

PR-URL: #43170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Refs: #36258

PR-URL: #43170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Refs: #36258

PR-URL: #43170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Refs: nodejs/node#36258

PR-URL: nodejs/node#43170
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Masashi Hirano <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. eventtarget Issues and PRs related to the EventTarget implementation. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC - EventTarget API Enhancements
5 participants