nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
105.69k stars 28.68k forks source link

Clear kResourceStore in timers and intervals #53408

Open mcollina opened 1 month ago

mcollina commented 1 month ago

As noted in this article, the timeout/interval objects keep a reference to the store even after they have been cleared.

I think it would be better to clear up that reference inside the clearTimeout() and clearInterval() functions. We might also want to consider exposing an API for end-users and explicit lifetime tracking.

mcollina commented 1 month ago

cc @nodejs/diagnostics @anonrig

Qard commented 1 month ago

setTimeout should probably actually just clear immediately after running given that it only occurs once. But yes, also clearing in clearInterval makes sense to me too.

Flarna commented 1 month ago

I guess this is not only relevant for timers and intervals, all resources are effected.

const { AsyncLocalStorage, AsyncResource } = require('node:async_hooks');
const als = new AsyncLocalStorage();
let r, p;
als.run("I consume a lot memory", () => {
  r = new AsyncResource("aResource", { requireManualDestroy: true })
  r.emitDestroy()
  p = Promise.resolve()
})
console.log(r, p);
 AsyncResource {
  [Symbol(async_id_symbol)]: 2,
  [Symbol(trigger_async_id_symbol)]: 1,
  [Symbol(kResourceStore)]: 'I consume a lot memory'
} Promise {
  undefined,
  [Symbol(async_id_symbol)]: 3,
  [Symbol(trigger_async_id_symbol)]: 1,
  [Symbol(kResourceStore)]: 'I consume a lot memory'
}

For timers/intervals it's likely easier to find the place to cleanup. Maybe we can extend the cleanup to some more places?

mcollina commented 1 month ago

Absolutely!

Flarna commented 1 month ago

timers allow similar "leaks" even without AsyncLocalStore if one uses arguments for the handler:

const t = setTimeout(() => {}, 100, "I consume a lot memory");
clearTimeout(t);
console.log(t);
console.log(t._timerArgs);
Timeout {
  _idleTimeout: -1,
  _idlePrev: null,
  _idleNext: null,
  _idleStart: 22,
  _onTimeout: null,
  _timerArgs: [Array],
  _repeat: null,
  _destroyed: true,
  [Symbol(refed)]: true,
  [Symbol(kHasPrimitive)]: false,
  [Symbol(asyncId)]: 2,
  [Symbol(triggerId)]: 1
}
[ 'I consume a lot memory' ]

If handler uses some variable from the outer scope it's even more.