Skip to content

lib: fix timer leak - #53337

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:fix_timer_leak
Jun 7, 2024
Merged

lib: fix timer leak#53337
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:fix_timer_leak

Conversation

@theanarkh

@theanarkhtheanarkh commented Jun 4, 2024

Copy link
Copy Markdown
Contributor

Fixes: #53335

  • 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

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jun 4, 2024
@theanarkh
theanarkhforce-pushed the fix_timer_leak branch 2 times, most recently from e25d0ab to 4eda87fCompareJune 4, 2024 16:02
@mitsuhiko

mitsuhiko commented Jun 4, 2024

Copy link
Copy Markdown

Thank you for fixing this! Small nit I noticed: test/parallel/test-rimitive-timer-leak.js should probably be test/parallel/test-primitive-timer-leak.js.

@theanarkhtheanarkh added request-ci Add this label to start a Jenkins CI on a PR. labels Jun 5, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2024
@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

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

@theanarkhtheanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 5, 2024
@theanarkhtheanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024
@nodejs-github-bot
nodejs-github-bot merged commit 2740cd4 into nodejs:mainJun 7, 2024
@nodejs-github-bot

nodejs-github-bot commented Jun 7, 2024

Copy link
Copy Markdown
Collaborator

Landed in 2740cd4

targos pushed a commit that referenced this pull request Jun 20, 2024
@theanarkh@targos
lib: fix timer leak
515dd24
PR-URL: #53337Fixes: #53335
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@theanarkh@soophoo
lib: fix timer leak
b3c967c
PR-URL: nodejs#53337Fixes: nodejs#53335
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@targostargos mentioned this pull request Jun 25, 2024
Merged
@soccermax

soccermax commented Jun 26, 2024•
edited
Loading

Copy link
Copy Markdown

will this be downported to node v18? I see that PR is included in v22.4.0 but we are still on v18.18.2.
Thanks

@atlowChemi

atlowChemi commented Jun 26, 2024

Copy link
Copy Markdown
Member

@soccermaxv22.4.0 is the current Node.js release, which means new commits from main which are not semver-majorPRs that contain breaking changes and should be released in the next major version. can immediately get in the next release.
V18 is in Maintenance state since 10.18.2023, which means fixes take longer to get in - if they do.

@soccermax

soccermax commented Jun 26, 2024

Copy link
Copy Markdown

@atlowChemi, thanks for the reply. That makes sense.

This issue might cause a significant memory leak. I believe it would be beneficial to backport the fix to Node versions which are still in maintenance. The explanation is that the closure of setTimeout might hold references to memory-intensive objects like database connections, response objects, or similar. Due to this issue, the entire closure of setTimeout, if the return object of setTimeout is used, will be indefinitely retained in memory.

In our project, this leaks memory of around 300MB per day. The problem is that the troubling setTimeout is called in a submodule. Of course, we can patch the library, but in general, it would make sense to get it fixed directly in Node. Given that the fix is quite small, it would be highly appreciated if it could be backported. Could you please check on this? Thanks.

@Karliz24Karliz24 mentioned this pull request Jul 9, 2024
Open
@ivan-kleshninivan-kleshnin mentioned this pull request Jul 9, 2024
Closed
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@theanarkh@marco-ippolito
lib: fix timer leak
d1a7800
PR-URL: #53337Fixes: #53335
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@marco-ippolitomarco-ippolito mentioned this pull request Jul 19, 2024
Merged
@richardlau

richardlau commented Sep 19, 2024

Copy link
Copy Markdown
Member

will this be downported to node v18? I see that PR is included in v22.4.0 but we are still on v18.18.2. Thanks

Unfortunately this doesn't cherry-pick cleanly to v18.x-staging. Since Node.js 18 is in maintenance it's unlikely this will be accepted, but if someone wants to try (on the understanding the PR might be turned down) they can open a backport PR against v18.x-staging.

@linfanxlinfanx mentioned this pull request Oct 15, 2024
Closed
3 tasks
@lubieowocelubieowoce mentioned this pull request Feb 6, 2025
Closed
1 task
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingrbenjamingrbenjamingr approved these changes
@anonriganonriganonrig approved these changes
@atlowChemiatlowChemiatlowChemi approved these changes
+1 more reviewer
@F3n67uF3n67uF3n67u approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.timersIssues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Timeout leaks when converted into a primitive and not cleared

9 participants

@theanarkh@mitsuhiko@nodejs-github-bot@soccermax@atlowChemi@richardlau@benjamingr@anonrig@F3n67u