node icon indicating copy to clipboard operation
node copied to clipboard

Clear kResourceStore in timers and intervals

Open mcollina opened this issue 1 year ago • 5 comments

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 avatar Jun 10 '24 16:06 mcollina

cc @nodejs/diagnostics @anonrig

mcollina avatar Jun 10 '24 16:06 mcollina

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.

Qard avatar Jun 10 '24 20:06 Qard

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?

Flarna avatar Jun 11 '24 12:06 Flarna

Absolutely!

mcollina avatar Jun 11 '24 13:06 mcollina

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.

Flarna avatar Jun 28 '24 10:06 Flarna