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

Documentation: performance warning about timeout.unref() is not clear. #42239

Closed
cakoose opened this issue Mar 7, 2022 · 3 comments · Fixed by #42241
Closed

Documentation: performance warning about timeout.unref() is not clear. #42239

cakoose opened this issue Mar 7, 2022 · 3 comments · Fixed by #42241
Labels
doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@cakoose
Copy link

cakoose commented Mar 7, 2022

Affected URL(s)

https://nodejs.org/api/timers.html#timeoutunref

Description of the problem

The documentation for timeout.unref() ends with this warning:

Calling timeout.unref() creates an internal timer that will wake the Node.js event loop. Creating too many of these can adversely impact performance of the Node.js application.

I don't understand what this means.

"creates an internal timer that will wake the Node.js event loop" sounds roughly like what setTimeout does. Is it different?

Tangent: I also noticed that the docs for the new timersPromises.setInterval (link) supports the ref parameter but there's no similar warning there.

@cakoose cakoose added the doc Issues and PRs related to the documentations. label Mar 7, 2022
@Trott
Copy link
Member

Trott commented Mar 7, 2022

@nodejs/timers

@meixg
Copy link
Member

meixg commented Mar 7, 2022

It had been added for a long time: bdd1a74
Maybe we need to check its current status.

@meixg
Copy link
Member

meixg commented Mar 7, 2022

I checked the code in v0.9.1, there is indeed a new Timer process:

node/lib/timers.js

Lines 254 to 267 in e6ce259

Timeout.prototype.unref = function() {
if (!this._handle) {
var delay = this._when - Date.now();
if (delay < 0) delay = 0;
exports.unenroll(this);
this._handle = new Timer();
this._handle.ontimeout = this._onTimeout;
this._handle.start(delay, 0);
this._handle.domain = this.domain;
this._handle.unref();
} else {
this._handle.unref();
}
};

But this is not the case now.

@VoltrexKeyva VoltrexKeyva added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Mar 8, 2022
nodejs-github-bot pushed a commit that referenced this issue Mar 10, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
resolve: #42239

PR-URL: #42241
Fixes: #42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
bengl pushed a commit that referenced this issue Mar 21, 2022
resolve: #42239

PR-URL: #42241
Fixes: #42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022

Verified

This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
resolve: nodejs#42239

PR-URL: nodejs#42241
Fixes: nodejs#42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022

Verified

This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
resolve: #42239

PR-URL: #42241
Fixes: #42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022

Verified

This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
resolve: #42239

PR-URL: #42241
Fixes: #42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022

Verified

This commit was signed with the committer’s verified signature.
danielleadams Danielle Adams
resolve: #42239

PR-URL: #42241
Fixes: #42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this issue Apr 25, 2022
resolve: nodejs#42239

PR-URL: nodejs#42241
Fixes: nodejs#42239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants