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

Document highWaterMark option of events.on() #52078

Closed
ehmicky opened this issue Mar 14, 2024 · 0 comments
Closed

Document highWaterMark option of events.on() #52078

ehmicky opened this issue Mar 14, 2024 · 0 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter. feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.

Comments

@ehmicky
Copy link

ehmicky commented Mar 14, 2024

What is the problem this feature will solve?

#41276 added the highWaterMark option to events.on() in Node 20.0.0.

const highWatermark = options.highWatermark ?? NumberMAX_SAFE_INTEGER;

That option is quite useful when using on(stream, 'data'). on() uses an internal buffer. If too many events happen at once, this buffer can potentially consume lots of memory. When the highWaterMark option is used, the stream is temporarily paused when this happens. Once the buffer is empty, the stream is resumed. Basically, this makes on() stream-friendly.

What is the feature you are proposing to solve the problem?

Document the highWaterMark option of stream.on().

What alternatives have you considered?

An alternative would be, if a Readable or Duplex is passed to on(), to set the highWaterMark option to stream.readableHighWaterMark. This would enable the above streaming behavior by default, when a stream is used.

In many cases, users will just want to enable this feature and use the same highWaterMark as the stream. So enabling this by default for stream might remove the need to expose the option.

@ehmicky ehmicky added the feature request Issues that request new features to be added to Node.js. label Mar 14, 2024
@ehmicky ehmicky changed the title Document highWaterMark option of stream.on() Document highWaterMark option of events.on() Mar 14, 2024
@atlowChemi atlowChemi added events Issues and PRs related to the events subsystem / EventEmitter. stream Issues and PRs related to the stream subsystem. labels Mar 14, 2024
atlowChemi added a commit to atlowChemi/node that referenced this issue Mar 14, 2024
atlowChemi added a commit to atlowChemi/node that referenced this issue Mar 14, 2024
atlowChemi added a commit to atlowChemi/node that referenced this issue Mar 14, 2024
atlowChemi added a commit to atlowChemi/node that referenced this issue Mar 17, 2024
atlowChemi added a commit to atlowChemi/node that referenced this issue Apr 4, 2024
nodejs-github-bot pushed a commit that referenced this issue Apr 5, 2024
Fixes: #52078
Refs: #41276
PR-URL: #52080
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
PR-URL: #52080
Fixes: #52078
Refs: #41276
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 2, 2024
Fixes: #52078
Refs: #41276
PR-URL: #52080
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
PR-URL: #52080
Fixes: #52078
Refs: #41276
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 3, 2024
Fixes: #52078
Refs: #41276
PR-URL: #52080
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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. feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants