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.

Sign up for GitHub

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

Jump to bottom

lib: fix timer leak #53337

Merged
merged 1 commit into from
Jun 7, 2024
Merged

lib: fix timer leak #53337

merged 1 commit into from
Jun 7, 2024

Conversation

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

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 theanarkh added request-ci Add this label to start a Jenkins CI on a PR. labels Jun 5, 2024
@github-actions 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
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@theanarkh theanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 5, 2024
@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024
@nodejs-github-bot 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 nodejs-github-bot merged commit 2740cd4 into nodejs:main Jun 7, 2024
65 checks passed
@nodejs-github-bot
Copy link
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: #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>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
@theanarkh @EliphazBouye
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>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@theanarkh @bmeck
lib: fix timer leak
0ea6ea9 
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 targos mentioned this pull request Jun 25, 2024
Merged
@soccermax
Copy link

soccermax commented Jun 26, 2024 •
edited
Loading

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
Copy link
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
Copy link

@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 Karliz24 mentioned this pull request Jul 9, 2024
Open
@ivan-kleshnin ivan-kleshnin mentioned this pull request Jul 9, 2024
Open
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@theanarkh @marco-ippolito
lib: fix timer leak
9d000a9 
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 pushed a commit that referenced this pull request Jul 19, 2024
@theanarkh @marco-ippolito
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 marco-ippolito mentioned this pull request Jul 19, 2024
Merged
@richardlau
Copy link
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@anonrig anonrig anonrig approved these changes

@F3n67u F3n67u F3n67u approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@theanarkh @mitsuhiko @nodejs-github-bot @soccermax @atlowChemi @richardlau @benjamingr @anonrig @F3n67u