events: add listener argument to listenerCount by ShogunPanda · Pull Request #46523 · nodejs/node

ShogunPanda · 2023-02-06T15:14:22Z

This PR introduces a new method for EventEmitter, called hasListener.
It returns true if a listener is already registered for an event.
This can be helpful to avoid adding a listener too many times and therefore helps avoiding leaks.

Fixes #43548.

nodejs-github-bot · 2023-02-06T15:14:27Z

Review requested:

@nodejs/http
@nodejs/net

lib/events.js

test/parallel/test-http-socket-error-listeners.js

lib/_http_server.js

test/parallel/test-http-socket-error-listeners.js

lpinca · 2023-02-06T15:43:56Z

I'm -1 on this but I won't block it if others think it is a good idea.

ronag

I didn't notice the http changes. I would probably be -0 on those. I like the hasListener method since I have often tried using that only to realise it doesn't exists.

lib/events.js

ShogunPanda · 2023-02-09T15:30:24Z

The HTTP changes have been removed and will be sent separately. Only the new methods remains.

lpinca · 2023-02-09T17:40:27Z

I'm neutral now.

ShogunPanda · 2023-02-09T18:55:04Z

Chaotic, lawful or pure?

😄

Sorry, I couldn't resist. Kick me out of the team if you need to.

mcollina

lgtm

cjihrig · 2023-02-11T15:46:31Z

Is there a good reason to add a new API instead of extending emitter.listenerCount(eventName) to take an optional listener argument. This would be more consistent with the rest of the API, while providing basically the same DX as the proposed change here.

jasnell · 2023-02-11T16:47:31Z

If the primary use case is to avoid a handler being registered more than once, then perhaps a new option would be better?

const ee = new EventEmitter();
ee.on('foo', () => {}, {
  unique: true
});

This also gives opportunity to drive closer alignment with EventTarget

Like @cjihrig, I'd rather try to avoid creating new methods on this API.

ShogunPanda · 2023-02-13T08:49:05Z

@jasnell I'll go with @cjihrig suggestion. Right now you are right in saying this is the only use case I can foresee, but users might have more so let's give them flexibility.

ShogunPanda · 2023-02-13T09:45:17Z

@jasnell @cjihrig Done. Please re-review it.

mcollina

still lgtm

doc/api/events.md

nodejs-github-bot · 2023-02-21T10:38:53Z

Landed in 6473e9c

PR-URL: #46523 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47086

Notable changes: buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 src: * (SEMVER-MINOR) add `fs.openAsBlob` to support File-backed Blobs (James M Snell) #45258 tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 wasi: * (SEMVER-MINOR) add support for version when creating WASI (Michael Dawson) #46469 worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47087

PR-URL: #46523 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: TBD

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

Notable changes: Add initial support for single executable applications Compile a JavaScript file into a single executable application: ```console $ echo 'console.log(`Hello, ${process.argv[2]}!`);' > hello.js $ cp $(command -v node) hello $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 $ npx postject hello NODE_JS_CODE hello.js \ --sentinel-fuse NODE_JS_FUSE_fce680ab2cc467b6e072b8b5df1996b2 \ --macho-segment-name NODE_JS $ ./hello world Hello, world! ``` Contributed by Darshan Sen in #45038 Replace url parser with Ada Node.js gets a new URL parser called Ada that is compliant with the WHATWG URL Specification and provides more than 100% performance improvement to the existing implementation. Contributed by Yagiz Nizipli in #46410 Other notable changes: * buffer: * (SEMVER-MINOR) add Buffer.copyBytesFrom(...) (James M Snell) #46500 * doc: * add marco-ippolito to collaborators (Marco Ippolito) #46816 * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 * events: * (SEMVER-MINOR) add listener argument to listenerCount (Paolo Insogna) #46523 * lib: * (SEMVER-MINOR) add AsyncLocalStorage.bind() and .snapshot() (flakey5) #46387 * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 * src: * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 * stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 * tls: * (SEMVER-MINOR) support automatic DHE (Tobias Nießen) #46978 * url: * (SEMVER-MINOR) implement URLSearchParams size getter (James M Snell) #46308 * worker: * (SEMVER-MINOR) add support for worker name in inspector and trace_events (Debadree Chatterjee) #46832 PR-URL: #47502

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: nodejs#46523

yuzheng14 · 2023-06-29T05:14:40Z

I found a bug in your pr and I fixed it in this pr #48592

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: #46523 PR-URL: #48592 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: nodejs#46523 PR-URL: nodejs#48592 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: #46523 PR-URL: #48592 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: nodejs#46523 PR-URL: nodejs#48592 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

When add listener by once, it will be wrapped into another function. And when pass listener and there is just one event listener added by once, it will return 0 even if passed listener equal wrapped event listener. Refs: #46523 PR-URL: #48592 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

ShogunPanda requested a review from lpinca February 6, 2023 15:14

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Feb 6, 2023

ShogunPanda mentioned this pull request Feb 6, 2023

http: do not leak error listeners #43587

Closed

ronag approved these changes Feb 6, 2023

View reviewed changes

lpinca reviewed Feb 6, 2023

View reviewed changes

lib/events.js Outdated Show resolved Hide resolved

test/parallel/test-http-socket-error-listeners.js Outdated Show resolved Hide resolved

lib/_http_server.js Outdated Show resolved Hide resolved

test/parallel/test-http-socket-error-listeners.js Outdated Show resolved Hide resolved

lpinca added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 6, 2023

ronag requested changes Feb 6, 2023

View reviewed changes

aduh95 reviewed Feb 6, 2023

View reviewed changes

lib/events.js Outdated Show resolved Hide resolved

ShogunPanda force-pushed the failure-poc branch 2 times, most recently from ead68cd to 3594b21 Compare February 9, 2023 08:53

mcollina approved these changes Feb 10, 2023

View reviewed changes

ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2023

events: add listener argument to listenerCount

eb50d19

ShogunPanda force-pushed the failure-poc branch from 3594b21 to eb50d19 Compare February 13, 2023 09:45

ShogunPanda changed the title ~~events: add hasListener method~~ events: add listener argument to listenerCount Feb 13, 2023

events: update PR in docs

669ffe7

mcollina approved these changes Feb 13, 2023

View reviewed changes

cjihrig approved these changes Feb 13, 2023

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

doc/api/events.md Outdated Show resolved Hide resolved

doc/api/events.md Outdated Show resolved Hide resolved

lpinca reviewed Feb 13, 2023

View reviewed changes

doc/api/events.md Outdated Show resolved Hide resolved

ShogunPanda deleted the failure-poc branch February 21, 2023 10:39

github-actions bot mentioned this pull request Feb 22, 2023

CI Reliability 2023-02-22 nodejs/reliability#503

Open

34 tasks

ShogunPanda mentioned this pull request Feb 22, 2023

http: use listenerCount when adding noop event #46769

Merged

github-actions bot mentioned this pull request Feb 23, 2023

CI Reliability 2023-02-23 nodejs/reliability#504

Open

31 tasks

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

yuzheng14 mentioned this pull request Jun 29, 2023

events: fix bug listenerCount don't compare wrapped listener #48592

Merged

Uh oh!

Conversation

ShogunPanda commented Feb 6, 2023

Uh oh!

nodejs-github-bot commented Feb 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lpinca commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShogunPanda commented Feb 9, 2023

Uh oh!

lpinca commented Feb 9, 2023

Uh oh!

ShogunPanda commented Feb 9, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 11, 2023

Uh oh!

jasnell commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShogunPanda commented Feb 13, 2023

Uh oh!

ShogunPanda commented Feb 13, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

yuzheng14 commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lpinca commented Feb 6, 2023 •

edited

Loading

jasnell commented Feb 11, 2023 •

edited

Loading