Skip to content

Commit 637251e

Browse files
committed
timers: do not retain a reference to the async store after firing
After firing timers, we can clean them up by iterating over all active stores and setting the relevant symbols to undefined. This also covers immediates and intervals. Refs: #53408 PR-URL: #53443 Fixes: #53408 Signed-off-by: Matteo Collina <hello@matteocollina.com>
1 parent 81c4f0a commit 637251e

5 files changed

Lines changed: 210 additions & 22 deletions

File tree

lib/internal/async_hooks.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,7 @@ const before_symbol = Symbol('before');
105105
const after_symbol = Symbol('after');
106106
const destroy_symbol = Symbol('destroy');
107107
const promise_resolve_symbol = Symbol('promiseResolve');
108+
const async_local_storage_context_symbol = Symbol('kAsyncLocalStorageContext');
108109
const emitBeforeNative = emitHookFactory(before_symbol, 'emitBeforeNative');
109110
const emitAfterNative = emitHookFactory(after_symbol, 'emitAfterNative');
110111
const emitDestroyNative = emitHookFactory(destroy_symbol, 'emitDestroyNative');
@@ -595,7 +596,8 @@ module.exports = {
595596
symbols: {
596597
async_id_symbol, trigger_async_id_symbol,
597598
init_symbol, before_symbol, after_symbol, destroy_symbol,
598-
promise_resolve_symbol, owner_symbol,
599+
promise_resolve_symbol, async_local_storage_context_symbol,
600+
owner_symbol,
599601
},
600602
constants: {
601603
kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve,

lib/internal/async_local_storage/async_hooks.js

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ const {
1212
const {
1313
validateObject,
1414
} = require('internal/validators');
15+
const {
16+
symbols: {
17+
async_local_storage_context_symbol,
18+
},
19+
} = require('internal/async_hooks');
1520

1621
const {
1722
AsyncResource,
@@ -23,6 +28,11 @@ const RunScope = require('internal/async_local_storage/run_scope');
2328
const { kEmptyObject } = require('internal/util');
2429

2530
const storageList = [];
31+
32+
function getOrCreateResourceStore(resource) {
33+
return resource[async_local_storage_context_symbol] ??= { __proto__: null };
34+
}
35+
2636
const storageHook = createHook({
2737
init(asyncId, type, triggerAsyncId, resource) {
2838
const currentResource = executionAsyncResource();
@@ -91,16 +101,18 @@ class AsyncLocalStorage {
91101

92102
// Propagate the context from a parent resource to a child one
93103
_propagate(resource, triggerResource, type) {
94-
const store = triggerResource[this.kResourceStore];
104+
const store = triggerResource[async_local_storage_context_symbol]?.[this.kResourceStore];
95105
if (this.enabled) {
96-
resource[this.kResourceStore] = store;
106+
const resourceStore = getOrCreateResourceStore(resource);
107+
resourceStore[this.kResourceStore] = store;
97108
}
98109
}
99110

100111
enterWith(store) {
101112
this._enable();
102113
const resource = executionAsyncResource();
103-
resource[this.kResourceStore] = store;
114+
const resourceStore = getOrCreateResourceStore(resource);
115+
resourceStore[this.kResourceStore] = store;
104116
}
105117

106118
run(store, callback, ...args) {
@@ -112,14 +124,15 @@ class AsyncLocalStorage {
112124
this._enable();
113125

114126
const resource = executionAsyncResource();
115-
const oldStore = resource[this.kResourceStore];
127+
const resourceStore = getOrCreateResourceStore(resource);
128+
const oldStore = resourceStore[this.kResourceStore];
116129

117-
resource[this.kResourceStore] = store;
130+
resourceStore[this.kResourceStore] = store;
118131

119132
try {
120133
return ReflectApply(callback, null, args);
121134
} finally {
122-
resource[this.kResourceStore] = oldStore;
135+
resourceStore[this.kResourceStore] = oldStore;
123136
}
124137
}
125138

@@ -138,10 +151,11 @@ class AsyncLocalStorage {
138151
getStore() {
139152
if (this.enabled) {
140153
const resource = executionAsyncResource();
141-
if (!(this.kResourceStore in resource)) {
154+
const resourceStore = resource[async_local_storage_context_symbol];
155+
if (resourceStore === undefined || !(this.kResourceStore in resourceStore)) {
142156
return this.#defaultValue;
143157
}
144-
return resource[this.kResourceStore];
158+
return resourceStore[this.kResourceStore];
145159
}
146160
return this.#defaultValue;
147161
}

lib/internal/timers.js

Lines changed: 66 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,9 @@ const {
9797
emitBefore,
9898
emitAfter,
9999
emitDestroy,
100+
symbols: {
101+
async_local_storage_context_symbol,
102+
},
100103
} = require('internal/async_hooks');
101104

102105
// Symbols for storing async id state.
@@ -125,6 +128,35 @@ const AsyncContextFrame = require('internal/async_context_frame');
125128

126129
const async_context_frame = Symbol('kAsyncContextFrame');
127130

131+
function removeStoresFromResource(resource) {
132+
if (!AsyncContextFrame.enabled &&
133+
resource[async_local_storage_context_symbol] !== undefined) {
134+
resource[async_local_storage_context_symbol] = undefined;
135+
}
136+
}
137+
138+
function cleanTimer(timer) {
139+
removeStoresFromResource(timer);
140+
timer._onTimeout = undefined;
141+
timer._timerArgs = undefined;
142+
}
143+
144+
function cleanImmediate(immediate) {
145+
removeStoresFromResource(immediate);
146+
immediate._onImmediate = undefined;
147+
immediate._argv = undefined;
148+
}
149+
150+
function removeStoresIfNotReinserted(resource) {
151+
if (!resource._idleNext && !resource._idlePrev)
152+
removeStoresFromResource(resource);
153+
}
154+
155+
function cleanTimerIfNotReinserted(timer) {
156+
if (!timer._idleNext && !timer._idlePrev)
157+
cleanTimer(timer);
158+
}
159+
128160
// *Must* match Environment::ImmediateInfo::Fields in src/env.h.
129161
const kCount = 0;
130162
const kRefCount = 1;
@@ -498,14 +530,19 @@ function getTimerCallbacks(runNextTicks) {
498530
const asyncId = immediate[async_id_symbol];
499531
emitBefore(asyncId, immediate[trigger_async_id_symbol], immediate);
500532

533+
let threw = true;
501534
try {
502535
const argv = immediate._argv;
503536
if (!argv)
504537
immediate._onImmediate();
505538
else
506539
immediate._onImmediate(...argv);
540+
threw = false;
507541
} finally {
508-
immediate._onImmediate = null;
542+
if (threw)
543+
process.nextTick(cleanImmediate, immediate);
544+
else
545+
cleanImmediate(immediate);
509546

510547
emitDestroy(asyncId);
511548

@@ -577,6 +614,8 @@ function getTimerCallbacks(runNextTicks) {
577614
if (!timer._destroyed) {
578615
timer._destroyed = true;
579616

617+
cleanTimer(timer);
618+
580619
if (timer[kHasPrimitive])
581620
delete knownTimersById[asyncId];
582621

@@ -599,26 +638,40 @@ function getTimerCallbacks(runNextTicks) {
599638
start = binding.getLibuvNow();
600639
}
601640

641+
let threw = true;
602642
try {
603643
const args = timer._timerArgs;
604644
if (args === undefined)
605645
timer._onTimeout();
606646
else
607647
ReflectApply(timer._onTimeout, timer, args);
648+
threw = false;
608649
} finally {
609650
if (timer._repeat && timer._idleTimeout !== -1) {
610651
timer._idleTimeout = timer._repeat;
611652
insert(timer, timer._idleTimeout, start);
612-
} else if (!timer._idleNext && !timer._idlePrev && !timer._destroyed) {
613-
timer._destroyed = true;
614-
615-
if (timer[kHasPrimitive])
616-
delete knownTimersById[asyncId];
617-
618-
if (timer[kRefed])
619-
timeoutInfo[0]--;
620-
621-
emitDestroy(asyncId);
653+
} else if (!timer._idleNext && !timer._idlePrev) {
654+
if (timer._destroyed) {
655+
if (threw)
656+
process.nextTick(cleanTimerIfNotReinserted, timer);
657+
else
658+
cleanTimer(timer);
659+
} else {
660+
if (threw)
661+
process.nextTick(removeStoresIfNotReinserted, timer);
662+
else
663+
removeStoresFromResource(timer);
664+
665+
timer._destroyed = true;
666+
667+
if (timer[kHasPrimitive])
668+
delete knownTimersById[asyncId];
669+
670+
if (timer[kRefed])
671+
timeoutInfo[0]--;
672+
673+
emitDestroy(asyncId);
674+
}
622675
}
623676
}
624677

@@ -703,6 +756,8 @@ module.exports = {
703756
kRefed,
704757
kHasPrimitive,
705758
initAsyncResource,
759+
cleanImmediate,
760+
cleanTimer,
706761
setUnrefTimeout,
707762
getTimerDuration,
708763
immediateQueue,

lib/timers.js

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ const {
3838
async_id_symbol,
3939
Timeout,
4040
Immediate,
41+
cleanImmediate,
42+
cleanTimer,
4143
decRefCount,
4244
immediateInfoFields: {
4345
kCount,
@@ -69,9 +71,12 @@ const {
6971

7072
// Remove a timer. Cancels the timeout and resets the relevant timer properties.
7173
function unenroll(item) {
72-
if (item._destroyed)
74+
if (item._destroyed) {
75+
cleanTimer(item);
7376
return;
77+
}
7478

79+
const wasEnrolled = item._idleNext !== null || item._idlePrev !== null;
7580
item._destroyed = true;
7681

7782
if (item[kHasPrimitive])
@@ -99,6 +104,9 @@ function unenroll(item) {
99104
decRefCount();
100105
}
101106

107+
if (wasEnrolled)
108+
cleanTimer(item);
109+
102110
// If active is called later, then we want to make sure not to insert again
103111
item._idleTimeout = -1;
104112
}
@@ -238,7 +246,7 @@ function clearImmediate(immediate) {
238246

239247
emitDestroy(immediate[async_id_symbol]);
240248

241-
immediate._onImmediate = null;
249+
cleanImmediate(immediate);
242250

243251
immediateQueue.remove(immediate);
244252
}
Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
// Flags: --expose-internals --no-async-context-frame
2+
'use strict';
3+
4+
const common = require('../common');
5+
const { AsyncLocalStorage } = require('async_hooks');
6+
const assert = require('assert');
7+
const {
8+
symbols: {
9+
async_local_storage_context_symbol,
10+
},
11+
} = require('internal/async_hooks');
12+
13+
function getStore(resource, asyncLocalStorage) {
14+
return resource[async_local_storage_context_symbol]?.[asyncLocalStorage.kResourceStore];
15+
}
16+
17+
function assertNoStore(resource) {
18+
assert.strictEqual(resource[async_local_storage_context_symbol], undefined);
19+
}
20+
21+
// Test that setTimeout does not retain a reference to the async store after
22+
// firing. The callback and arguments must stay on the Timeout object so that
23+
// refresh() can reactivate the timer.
24+
{
25+
const asyncLocalStorage = new AsyncLocalStorage();
26+
const store = {};
27+
const arg = {};
28+
asyncLocalStorage.run(store, common.mustCall(() => {
29+
const timeout = setTimeout(common.mustCall((received) => {
30+
assert.strictEqual(received, arg);
31+
setImmediate(common.mustCall(() => {
32+
assertNoStore(timeout);
33+
}));
34+
}), 1, arg);
35+
assert.strictEqual(getStore(timeout, asyncLocalStorage), store);
36+
}));
37+
}
38+
39+
// Test that clearTimeout cleans up the store, callback, and arguments before
40+
// firing.
41+
{
42+
const asyncLocalStorage = new AsyncLocalStorage();
43+
const store = {};
44+
const arg = {};
45+
asyncLocalStorage.run(store, common.mustCall(() => {
46+
const timeout = setTimeout(common.mustNotCall(), common.platformTimeout(1000), arg);
47+
assert.strictEqual(getStore(timeout, asyncLocalStorage), store);
48+
clearTimeout(timeout);
49+
assertNoStore(timeout);
50+
assert.strictEqual(timeout._onTimeout, undefined);
51+
assert.strictEqual(timeout._timerArgs, undefined);
52+
}));
53+
}
54+
55+
// Test that setInterval does not retain a reference to the async store,
56+
// callback, or arguments after it is cleared.
57+
{
58+
const asyncLocalStorage = new AsyncLocalStorage();
59+
const store = {};
60+
const arg = {};
61+
asyncLocalStorage.run(store, common.mustCall(() => {
62+
const interval = setInterval(common.mustCall((received) => {
63+
assert.strictEqual(received, arg);
64+
clearInterval(interval);
65+
assert.strictEqual(asyncLocalStorage.getStore(), store);
66+
setImmediate(common.mustCall(() => {
67+
assertNoStore(interval);
68+
assert.strictEqual(interval._onTimeout, undefined);
69+
assert.strictEqual(interval._timerArgs, undefined);
70+
}));
71+
}), 1, arg);
72+
assert.strictEqual(getStore(interval, asyncLocalStorage), store);
73+
}));
74+
}
75+
76+
// Test that setImmediate does not retain a reference to the async store,
77+
// callback, or arguments after firing.
78+
{
79+
const asyncLocalStorage = new AsyncLocalStorage();
80+
const store = {};
81+
const arg = {};
82+
asyncLocalStorage.run(store, common.mustCall(() => {
83+
const immediate = setImmediate(common.mustCall((received) => {
84+
assert.strictEqual(received, arg);
85+
setImmediate(common.mustCall(() => {
86+
assertNoStore(immediate);
87+
assert.strictEqual(immediate._onImmediate, undefined);
88+
assert.strictEqual(immediate._argv, undefined);
89+
}));
90+
}), arg);
91+
assert.strictEqual(getStore(immediate, asyncLocalStorage), store);
92+
}));
93+
}
94+
95+
// Test that clearImmediate cleans up the store, callback, and arguments before
96+
// firing.
97+
{
98+
const asyncLocalStorage = new AsyncLocalStorage();
99+
const store = {};
100+
const arg = {};
101+
asyncLocalStorage.run(store, common.mustCall(() => {
102+
const immediate = setImmediate(common.mustNotCall(), arg);
103+
assert.strictEqual(getStore(immediate, asyncLocalStorage), store);
104+
clearImmediate(immediate);
105+
assertNoStore(immediate);
106+
assert.strictEqual(immediate._onImmediate, undefined);
107+
assert.strictEqual(immediate._argv, undefined);
108+
}));
109+
}

0 commit comments

Comments
 (0)