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

Timeout leaks when converted into a primitive and not cleared #53335

Closed
mitsuhiko opened this issue Jun 4, 2024 · 1 comment · Fixed by #53337
Closed

Timeout leaks when converted into a primitive and not cleared #53335

mitsuhiko opened this issue Jun 4, 2024 · 1 comment · Fixed by #53337
Labels
timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Comments

@mitsuhiko
Copy link

mitsuhiko commented Jun 4, 2024

Version

21.2.0

Platform

Darwin cheetah.local 23.5.0 Darwin Kernel Version 23.5.0: Wed May 1 20:12:58 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T6000 arm64

Subsystem

timers.js

What steps will reproduce the bug?

On node calling setTimeout returns a Timeout object. That object is tracked in an internal list of timers and that list is maintained in two places. On the one hand in unenroll which is used by clearTimeout (and clearInterval) and one when the timer runs.

However only the unenroll path also removes a timer from the internal knownTimersById map. This map is updated whenever the Timeout is converted into a primitive. From that moment onwards a timer can be cleared by it's internal async id.

So to get a setTimeout to leak you just need to call +setTimeout(...) and it wait for it to complete. The entry from the knownTimersById map is not removed and we leak.

The repro case is trivial:

// leaks
for (i = 0; i < 500000; i++) {
  +setTimeout(() => {}, 0);
}

This will create 500000 un-collectable Timeouts that can be found in the knownTimersById map in timers.js. Removing the + fixes it.

Timer is removed here from the list but not from knownTimersById:

node/lib/internal/timers.js

Lines 544 to 545 in 7d14d1f

// The actual logic for when a timeout happens.
L.remove(timer);

Compare this to how unenroll clears:

node/lib/timers.js

Lines 86 to 93 in 7d14d1f

if (item[kHasPrimitive])
delete knownTimersById[item[async_id_symbol]];
// Fewer checks may be possible, but these cover everything.
if (destroyHooksExist() && item[async_id_symbol] !== undefined)
emitDestroy(item[async_id_symbol]);
L.remove(item);

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Not leak memory

What do you see instead?

Leaks memory

Additional information

We ran into this with the Sentry SDK though it's not entirely clear yet what actually converts the value there into a primitive. Might be some monkey patching going on somewhere.

The code looks the same on the latest version but I did not try to repro it there yet.

Refs getsentry/sentry-javascript#12317 (comment)

@anonrig
Copy link
Member

anonrig commented Jun 4, 2024

cc @nodejs/timers

@anonrig anonrig added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Jun 4, 2024
@theanarkh theanarkh mentioned this issue Jun 4, 2024
4 tasks
nodejs-github-bot pushed a commit that referenced this issue Jun 7, 2024
PR-URL: #53337
Fixes: #53335
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this issue Jun 20, 2024
PR-URL: #53337
Fixes: #53335
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#53337
Fixes: nodejs#53335
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#53337
Fixes: nodejs#53335
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53337
Fixes: #53335
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
PR-URL: #53337
Fixes: #53335
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants