FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: fix timer leak by theanarkh · Pull Request #53337 · nodejs/node · GitHub

/ node Public

lib: fix timer leak - #53337

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
theanarkh:fix_timer_leak
Jun 7, 2024
ConversationCommits (1)ChecksFiles changed
Merged

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

Conversation

theanarkh 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-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 force-pushed the fix_timer_leak branch 2 times, most recently from e25d0ab to 4eda87f Compare June 4, 2024 16:02

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.

theanarkh added request-ci Add this label to start a Jenkins CI on a PR. labels Jun 5, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 5, 2024

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 commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 5, 2024

Copy link
Copy Markdown
Collaborator

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

theanarkh added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 5, 2024
theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024
nodejs-github-bot merged commit 2740cd4 into nodejs:main Jun 7, 2024

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
lib: fix timer leak
515dd24 
PR-URL: #53337
Fixes: #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
lib: fix timer leak
b3c967c 
PR-URL: nodejs#53337
Fixes: 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>
targos mentioned this pull request Jun 25, 2024
Merged

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 commented Jun 26, 2024

Copy link
Copy Markdown
Member

@soccermax v22.4.0 is the current Node.js release, which means new commits from main which are not semver-major PRs 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 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.

Karliz24 mentioned this pull request Jul 9, 2024
Open
ivan-kleshnin mentioned this pull request Jul 9, 2024
Closed
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
lib: fix timer leak
d1a7800 
PR-URL: #53337
Fixes: #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-ippolito mentioned this pull request Jul 19, 2024
Merged

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.

linfanx mentioned this pull request Oct 15, 2024
Closed
3 tasks
lubieowoce mentioned this pull request Feb 6, 2025
Closed
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
anonrig anonrig approved these changes
atlowChemi atlowChemi approved these changes
+1 more reviewer
F3n67u F3n67u approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. timers Issues 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

Back | FazBrowse Home | New Git URL