timers: do not retain a reference to the async store after firing #53443
Conversation
nodejs-github-bot
added
async_hooks
|
I've opened a draft PR to discuss a possible solution to #53408. cc @nodejs/diagnostics @nodejs/timers let me know what you think. |
Signed-off-by: Matteo Collina <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
| @@ -429,6 +432,20 @@ function setPosition(node, pos) { | |||
| node.priorityQueuePosition = pos; | |||
| } | |||
|
|
|||
| function removeAllStores (timer) { | |||
There was a problem hiding this comment.
What about moving this function into async_hooks to avoid the need to expose getActiveStores() there?
An alternative would be to use a single object on the resource which holds all stores. This would reduce the number of properties added on resource objects - and avoid the need of a for loop to clear it.
There was a problem hiding this comment.
A helper that receives a resource and clears all the store data off it would be better than exposing the stores list as we do now. As it is presently, one could get and retain access to a store in a way unintentional from the perspective of the store owner, which is a bit dangerous from a memory perspective.
| @@ -594,6 +611,9 @@ function getTimerCallbacks(runNextTicks) { | |||
| if (timer[kRefed]) | |||
| timeoutInfo[0]--; | |||
|
|
|||
| removeAllStores(timer); | |||
| timer._onTimeout = undefined; | |||
After firing timers, we can clean them up by iterating over all active stores and setting the relevant symbols to undefined.
This is still a draft because we will need to extend it to immediates and intervals, too.
Fixes #53408