Skip to content

Commit b03f41d

Browse files
committed
lib: format default abort reason stack eagerly
The default AbortError created by AbortController.prototype.abort() and AbortSignal.abort() keeps its stack unformatted. Until it is formatted, V8 holds on to the call sites, which retain the receivers and functions of the calling frames. A signal that outlives the work it cancelled can then keep that work alive, including large buffers. Read the stack once when the default reason is created, as streams already do. Reasons passed by the caller are left untouched. Signed-off-by: Kuldeep Yadav <kuldeeep.yadav1@gmail.com>
1 parent e7d8ab5 commit b03f41d

2 files changed

Lines changed: 63 additions & 3 deletions

File tree

‎lib/internal/abort_controller.js‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,20 @@ function setWeakAbortSignalTimeout(weakRef, delay) {
216216
return timeout;
217217
}
218218

219+
function createDefaultAbortReason() {
220+
const reason = new DOMException('This operation was aborted', 'AbortError');
221+
// Format the stack eagerly. Until it is formatted, V8 keeps the call sites,
222+
// which retain the receivers and functions of the caller's frames, so a
223+
// retained signal would keep the aborted work alive.
224+
// Refs: https://github.com/nodejs/node/pull/34103#issuecomment-652002364
225+
try {
226+
reason.stack; // eslint-disable-line no-unused-expressions
227+
} catch {
228+
// A throwing Error.prepareStackTrace must not make abort() throw.
229+
}
230+
return reason;
231+
}
232+
219233
class AbortSignal extends EventTarget {
220234
#brand;
221235

@@ -293,8 +307,7 @@ class AbortSignal extends EventTarget {
293307
* @param {any} [reason]
294308
* @returns {AbortSignal}
295309
*/
296-
static abort(
297-
reason = new DOMException('This operation was aborted', 'AbortError')) {
310+
static abort(reason = createDefaultAbortReason()) {
298311
return new AbortSignal(kDontThrowSymbol, { aborted: true, reason });
299312
}
300313

@@ -576,7 +589,7 @@ class AbortController {
576589
/**
577590
* @param {any} [reason]
578591
*/
579-
abort(reason = new DOMException('This operation was aborted', 'AbortError')) {
592+
abort(reason = createDefaultAbortReason()) {
580593
abortSignal(this.#signal ??= new AbortSignal(kDontThrowSymbol), reason);
581594
}
582595

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Flags: --expose-gc
2+
3+
import '../common/index.mjs';
4+
import { gcUntil } from '../common/gc.js';
5+
import assert from 'node:assert/strict';
6+
import { it } from 'node:test';
7+
8+
// The default abort reason must not keep the objects on the caller's stack
9+
// alive. Refs: https://github.com/nodejs/node/issues/66192
10+
11+
class Job {
12+
controller = new AbortController();
13+
14+
cancel() {
15+
this.controller.abort();
16+
return this.controller.signal;
17+
}
18+
19+
cancelStatic() {
20+
return AbortSignal.abort();
21+
}
22+
}
23+
24+
for (const method of ['cancel', 'cancelStatic']) {
25+
it(`does not retain the caller through the default reason (${method})`, async () => {
26+
let job = new Job();
27+
const jobRef = new WeakRef(job);
28+
const signal = job[method]();
29+
job = null;
30+
31+
await gcUntil('job is collected', () => jobRef.deref() === undefined);
32+
assert.strictEqual(signal.aborted, true);
33+
assert.strictEqual(signal.reason.name, 'AbortError');
34+
assert.match(signal.reason.stack, new RegExp(`at Job\\.${method} `));
35+
});
36+
}
37+
38+
it('does not throw when Error.prepareStackTrace throws', () => {
39+
const { prepareStackTrace } = Error;
40+
Error.prepareStackTrace = () => { throw new Error('boom'); };
41+
try {
42+
assert.strictEqual(new Job().cancel().reason.name, 'AbortError');
43+
assert.strictEqual(new Job().cancelStatic().reason.name, 'AbortError');
44+
} finally {
45+
Error.prepareStackTrace = prepareStackTrace;
46+
}
47+
});

0 commit comments

Comments
 (0)