Skip to content

timers: do not retain a reference to the async store after firing #53443

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

timers: do not retain a reference to the async store after firing #53443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00931d1
timers: do not retain a reference to the async store after firing
mcollina Jun 13, 2024
5321fa8
fixup
mcollina Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,4 +381,8 @@ module.exports = {
asyncWrapProviders: ObjectFreeze({ __proto__: null, ...asyncWrap.Providers }),
// Embedder API
AsyncResource,
// TODO(mcollina): make the list private again.
getActiveStores () {
return [...storageList];
}
};
20 changes: 20 additions & 0 deletions lib/internal/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ let debug = require('internal/util/debuglog').debuglog('timer', (fn) => {
debug = fn;
});

let asyncHooks = null;

// *Must* match Environment::ImmediateInfo::Fields in src/env.h.
const kCount = 0;
const kRefCount = 1;
Expand Down Expand Up @@ -164,6 +166,7 @@ function initAsyncResource(resource, type) {
if (initHooksExist())
emitInit(asyncId, type, triggerAsyncId, resource);
}

class Timeout {
// Timer constructor function.
// The entire prototype is defined in lib/timers.js
Expand Down Expand Up @@ -429,6 +432,20 @@ function setPosition(node, pos) {
node.priorityQueuePosition = pos;
}

function removeAllStores (timer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// TODO(mcollina): move the list of stores to private
asyncHooks ??= require('async_hooks');

// TODO(mcollina): this does a copy for safety, we
// should be fast and unsafe.
const activeStores = asyncHooks.getActiveStores();

// Use for loop for speed
for (let i = 0; i < activeStores.length; i++) {
timer[activeStores[i].kResourceStore] = undefined;
}
}

function getTimerCallbacks(runNextTicks) {
// If an uncaught exception was thrown during execution of immediateQueue,
// this queue will store all remaining Immediates that need to run upon
Expand Down Expand Up @@ -594,6 +611,9 @@ function getTimerCallbacks(runNextTicks) {
if (timer[kRefed])
timeoutInfo[0]--;

removeAllStores(timer);
timer._onTimeout = undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe clear also _timerArgs.


if (destroyHooksExist())
emitDestroy(asyncId);
}
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/timeout-async-store-leak.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';

const common = require('../common');
const { AsyncLocalStorage } = require('async_hooks');
const assert = require('assert');

const asyncLocalStorage = new AsyncLocalStorage();
asyncLocalStorage.run({}, common.mustCall(() => {
const timeout = setTimeout(common.mustCall(() => {
setImmediate(common.mustCall(() => {
assert.strictEqual(timeout[asyncLocalStorage.kResourceStore], undefined);
assert.strictEqual(timeout._onTimeout, undefined);
}))
}));
}));