stream: fix eventNames() to not return not defined events by IlyasShabi · Pull Request #51331 · nodejs/node · GitHub

Fix issue #51302
As explained by @Qard, the root cause is that by default, we pre-allocate events here here

Since those events are initially set to undefined, we should check the value of the listener before returning eventNames().
I also believe that we are protected because we are not allowed to set a null or undefined listener.

I did different tests like:

const readableStream = createReadable();
readableStream.on('a', () => {});
readableStream.on('error', () => {});
console.log(readableStream.eventNames()); // ['a', 'error']

const readableStream = createReadable();
readableStream.on('a', () => {});
console.log(readableStream.eventNames()); // ['a']

I will add tests as soon as I can

Yep, that's basically what I had in mind. I'd maybe make it check if it's an array and non-empty though to be more accurate rather than just the simple check that it's not undefined. Can you add some tests? 🙂

LGTM, please add a test :]

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

LGTM, but a suggestion: the tests currently rely on the implementation detail of streams doing the _events pre-allocation. It may be a good idea to manually reproduce the behaviour on a plain event listener as streams could change in the future to not do that anymore. 🤔

I wonder if it is better to override eventNames() only on classes where _events is pre-allocated. We don't do this with the base EventEmitter.

I wonder if it is better to override eventNames() only on classes where _events is pre-allocated. We don't do this with the base EventEmitter.

@lpinca I agree with you, this is related to Stream not EventEmitter, I pushed the changes, could you please check again ?
cc @Qard @benjamingr

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

I think flaky tests 🤔 no ?
@lpinca

Yes.

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

Failed on this one wasi/test-wasi # TODO : Fix flaky test
@lpinca should I do something to merge this one ?

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

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

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

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

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

Commit Queue failed

- Loading data for nodejs/node/pull/51331
✔  Done loading data for nodejs/node/pull/51331
----------------------------------- PR info ------------------------------------
Title      stream: fix eventNames() to not return not defined events (#51331)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     IlyasShabi:stream-event-names -> nodejs:main
Labels     events, needs-ci
Commits    1
 - stream: fix eventNames() to not return not defined events
Committers 1
 - Ilyas Shabi 
PR-URL: https://github.com/nodejs/node/pull/51331
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Stephen Belanger 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51331
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Stephen Belanger 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - stream: fix eventNames() to not return not defined events
   ℹ  This PR was created on Mon, 01 Jan 2024 12:43:44 GMT
   ✔  Approvals: 3
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/51331#pullrequestreview-1799869531
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/51331#pullrequestreview-1890807762
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51331#pullrequestreview-1890926622
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-27T21:35:07Z: https://ci.nodejs.org/job/node-test-pull-request/57477/
- Querying data for job/node-test-pull-request/57477/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8072630772

Landed in a51efa2

PR-URL: #51331 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. labels Jan 1, 2024

IlyasShabi force-pushed the stream-event-names branch 2 times, most recently from 34542ca to bf229ca Compare January 1, 2024 12:52

IlyasShabi changed the title Fix stream eventNames to not return not defined events stream: fix eventNames() to not return not defined events Jan 1, 2024

benjamingr reviewed Jan 1, 2024

View reviewed changes

IlyasShabi force-pushed the stream-event-names branch 2 times, most recently from 9a4ba8d to 0c7adfc Compare January 1, 2024 21:34

benjamingr approved these changes Jan 1, 2024

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2024

Qard approved these changes Jan 2, 2024

View reviewed changes

lpinca reviewed Jan 2, 2024

View reviewed changes

Comment thread lib/events.js Outdated

IlyasShabi force-pushed the stream-event-names branch from 0c7adfc to 05c64c3 Compare February 18, 2024 18:47

IlyasShabi force-pushed the stream-event-names branch from 05c64c3 to feb07ec Compare February 18, 2024 18:50

Qard approved these changes Feb 20, 2024

View reviewed changes

lpinca approved these changes Feb 20, 2024

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024

IlyasShabi force-pushed the stream-event-names branch from feb07ec to cea8fe3 Compare February 24, 2024 23:48

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2024

stream: fix eventNames() to not return not defined events

2bbe1f6

IlyasShabi force-pushed the stream-event-names branch from cea8fe3 to 2bbe1f6 Compare February 26, 2024 15:25

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2024

Qard added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 27, 2024

Qard approved these changes Feb 27, 2024

View reviewed changes

Qard added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 27, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2024

nodejs-github-bot merged commit a51efa2 into nodejs:main Feb 27, 2024

marco-ippolito mentioned this pull request Mar 1, 2024

v21.7.0 proposal #51932

Merged

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

IlyasShabi deleted the stream-event-names branch October 16, 2024 18:43

IlyasShabi mentioned this pull request Oct 29, 2024

eventNames net test DataDog/dd-trace-js#4835

Closed

6 tasks

stream: fix eventNames() to not return not defined events by IlyasShabi · Pull Request #51331 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: fix eventNames() to not return not defined events - #51331

stream: fix eventNames() to not return not defined events#51331nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom IlyasShabi:stream-event-namesIlyasShabi/node:stream-event-namesCopy head branch name to clipboard

Conversation

IlyasShabi commented Jan 1, 2024

Uh oh!

Qard commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jan 1, 2024

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lpinca commented Jan 2, 2024

Uh oh!

IlyasShabi commented Feb 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

IlyasShabi commented Feb 20, 2024

Uh oh!

lpinca commented Feb 20, 2024

Uh oh!

nodejs-github-bot commented Feb 20, 2024

Uh oh!

IlyasShabi commented Feb 23, 2024

Uh oh!

nodejs-github-bot commented Feb 23, 2024

Uh oh!

nodejs-github-bot commented Feb 25, 2024

Uh oh!

nodejs-github-bot commented Feb 27, 2024

Uh oh!

nodejs-github-bot commented Feb 27, 2024

Uh oh!

nodejs-github-bot commented Feb 27, 2024

Uh oh!

nodejs-github-bot commented Feb 27, 2024

Uh oh!

nodejs-github-bot commented Feb 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stream: fix eventNames() to not return not defined events#51331
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
IlyasShabi:stream-event-names

Qard commented Jan 1, 2024 •
edited
Loading

IlyasShabi commented Feb 18, 2024 •
edited
Loading