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.

Sign up for GitHub

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

Jump to bottom

events: add max listener warning for EventTarget #36001

Closed
wants to merge 1 commit into from
Closed

events: add max listener warning for EventTarget #36001

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 6, 2020

Fixes: #35990

Signed-off-by: James M Snell jasnell@gmail.com

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

@jasnell jasnell requested a review from benjamingr November 6, 2020 16:12
@jasnell jasnell added 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. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 6, 2020
benjamingr
benjamingr reviewed Nov 6, 2020
View reviewed changes
doc/api/events.md Outdated Show resolved Hide resolved
benjamingr
benjamingr reviewed Nov 6, 2020
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Looks good - except the static on EventTarget.

Domenic pointed me towards https://heycam.github.io/webidl/#create-an-interface-object that does not allow adding additional properties.

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

This comment has been minimized.

Sign in to view
@jasnell jasnell force-pushed the max-listeners-for-eventtarget branch from 34e87e4 to 53dc04d Compare November 6, 2020 17:10
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@benjamingr
Copy link
Member

@jasnell would it be possible to make it static on events rather than a symbol on EventTarget so there are no additional methods tacked onto the EventTarget instance? (Even behind a symbol)

I think the DOM people and spec feel very strongly that once there are things added it's no longer a spec compliant EventTarget.

@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2020

Possible, yes, ideal, no? It would end up needing to be something like...

events.setEventTargetMaxListeners(eventTarget, 5);

Because the limit should be settable per EventTarget instance to match the behavior on EventEmitter and NodeEventTarget

@jasnell
Copy link
Member Author

jasnell commented Nov 6, 2020

Ok, this is the approach I just implemented:

const events = require('events')
const et1 = new EventTarget();
const et2 = new EventTarget();

events.setEventTargetMaxListeners(15); // Set the default for all EventTargets

events.setEventTargetMaxListeners(15, et1); // Set the max for et1
events.setEventTargetMaxListeners(15, et1, et2); // Set the max for et1 and et2

It's not pretty but it works.

@jasnell jasnell force-pushed the max-listeners-for-eventtarget branch from 53dc04d to abb7409 Compare November 6, 2020 18:54
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@jasnell jasnell requested a review from benjamingr November 6, 2020 19:00
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@jasnell jasnell force-pushed the max-listeners-for-eventtarget branch from abb7409 to 1f87c08 Compare November 6, 2020 19:16
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot
Copy link
Collaborator

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

benjamingr
benjamingr reviewed Nov 7, 2020
View reviewed changes
doc/api/events.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Nov 12, 2020

The answer may very well be "yes" but I want to make sure that's a conscious choice.

I wouldn't see why not to be honest. They are functionally consistent with each other.

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 12, 2020
@jasnell jasnell mentioned this pull request Nov 12, 2020
Closed
3 tasks
Trott
Trott reviewed Nov 13, 2020
View reviewed changes
doc/api/events.md
-->

* `n` {number} A non-negative number. The maximum number of listeners per
`EventTarget` event.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`EventTarget` event.
`EventTarget` or `EventEmitter`.

Trott
Trott reviewed Nov 13, 2020
View reviewed changes
doc/api/events.md
`EventTarget` event.
* `...eventsTargets` {EventTarget[]|EventEmitter[]} Zero or more {EventTarget}
or {EventEmitter} instances. If none are specified, `n` is set as the default
max for all newly created {EventTarget} and {EventEmitter} objects.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
max for all newly created {EventTarget} and {EventEmitter} objects.
maximum for all newly-created {EventTarget} and {EventEmitter} objects.

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

Landed in cd31340...dc79f3f

nodejs-github-bot pushed a commit that referenced this pull request Nov 16, 2020
@jasnell @nodejs-github-bot
events: add max listener warning for EventTarget
dc79f3f 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36001
Fixes: #35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@github-actions github-actions bot closed this Nov 16, 2020
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@jasnell @codebytere
events: add max listener warning for EventTarget
9ce9b01 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36001
Fixes: #35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere added a commit that referenced this pull request Nov 22, 2020
@codebytere
2020-11-24, Version 15.3.0 (Current)
ac213a0 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: TODO
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
codebytere added a commit that referenced this pull request Nov 24, 2020
@codebytere
2020-11-24, Version 15.3.0 (Current)
636a649 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: TODO
codebytere added a commit that referenced this pull request Nov 24, 2020
@codebytere
2020-11-24, Version 15.3.0 (Current)
63e7dc1 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: #36232
codebytere added a commit that referenced this pull request Nov 24, 2020
@codebytere
2020-11-24, Version 15.3.0 (Current)
7b5b8ef 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: #36232
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
@jasnell @targos
events: add max listener warning for EventTarget
c63d539 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36001
Fixes: nodejs#35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
@jasnell @targos
events: add max listener warning for EventTarget
89d15fe 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36001
Fixes: nodejs#35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@jasnell @targos
events: add max listener warning for EventTarget
6b3a21c 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36001
Fixes: nodejs#35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
@jasnell @targos
events: add max listener warning for EventTarget
3006302 
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36001
Backport-PR-URL: #38386
Fixes: #35990
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
@ejose19 ejose19 mentioned this pull request Jan 15, 2022
Merged
7 tasks
@mcollina mcollina mentioned this pull request Sep 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn on EventTarget maxListeners > THRESHOLD
5 participants
@jasnell @nodejs-github-bot @benjamingr @Trott @targos