From 1d66ae61ea734ec7d19a73cd2b4547bff7ec53bb Mon Sep 17 00:00:00 2001 From: Ayush Chaudhary Date: Thu, 2 Jul 2026 21:50:14 +0530 Subject: [PATCH] diagnostics_channel: replace using with try-finally --- lib/diagnostics_channel.js | 165 +++++++++++------- .../test-diagnostics-channel-no-erm.js | 20 +++ 2 files changed, 125 insertions(+), 60 deletions(-) create mode 100644 test/parallel/test-diagnostics-channel-no-erm.js diff --git a/lib/diagnostics_channel.js b/lib/diagnostics_channel.js index 7b78851208df66..a4d9abc59c66d9 100644 --- a/lib/diagnostics_channel.js +++ b/lib/diagnostics_channel.js @@ -23,6 +23,7 @@ const { codes: { ERR_INVALID_ARG_TYPE, }, + NodeAggregateError, } = require('internal/errors'); const { validateFunction, @@ -88,8 +89,8 @@ class RunStoresScope { #stack; constructor(activeChannel, data) { - // eslint-disable-next-line no-restricted-globals - using stack = new DisposableStack(); + // TODO: use native DisposableStack once supported in core + const stack = []; // Enter stores using withScope if (activeChannel._stores) { @@ -109,7 +110,7 @@ class RunStoresScope { } } - stack.use(store.withScope(newContext)); + ArrayPrototypePush(stack, store.withScope(newContext)); } } @@ -117,11 +118,31 @@ class RunStoresScope { activeChannel.publish(data); // Transfer ownership of the stack - this.#stack = stack.move(); + this.#stack = stack; } [SymbolDispose]() { - this.#stack[SymbolDispose](); + if (this.#stack !== undefined) { + const stack = this.#stack; + this.#stack = undefined; + + const errors = []; + for (let i = stack.length - 1; i >= 0; i--) { + try { + stack[i][SymbolDispose](); + } catch (error) { + ArrayPrototypePush(errors, error); + } + } + + if (errors.length === 0) { + return; + } else if (errors.length === 1) { + throw errors[0]; + } else { + throw new NodeAggregateError(errors); + } + } } } @@ -197,9 +218,12 @@ class ActiveChannel { } runStores(data, fn, thisArg, ...args) { - // eslint-disable-next-line no-unused-vars - using scope = this.withStoreScope(data); - return ReflectApply(fn, thisArg, args); + const scope = this.withStoreScope(data); + try { + return ReflectApply(fn, thisArg, args); + } finally { + scope[SymbolDispose](); + } } } @@ -396,9 +420,12 @@ class BoundedChannel { run(context, fn, thisArg, ...args) { context ??= {}; - // eslint-disable-next-line no-unused-vars - using scope = this.withScope(context); - return ReflectApply(fn, thisArg, args); + const scope = this.withScope(context); + try { + return ReflectApply(fn, thisArg, args); + } finally { + scope[SymbolDispose](); + } } } @@ -521,16 +548,19 @@ class TracingChannel { const { error } = this; - // eslint-disable-next-line no-unused-vars - using scope = this.#callWindow.withScope(context); + const scope = this.#callWindow.withScope(context); try { - const result = ReflectApply(fn, thisArg, args); - context.result = result; - return result; - } catch (err) { - context.error = err; - error.publish(context); - throw err; + try { + const result = ReflectApply(fn, thisArg, args); + context.result = result; + return result; + } catch (err) { + context.error = err; + error.publish(context); + throw err; + } + } finally { + scope[SymbolDispose](); } } @@ -550,9 +580,12 @@ class TracingChannel { context.error = err; error.publish(context); // Use continuation window for asyncStart/asyncEnd - // eslint-disable-next-line no-unused-vars - using scope = continuationWindow.withScope(context); - // TODO: Is there a way to have asyncEnd _after_ the continuation? + const scope = continuationWindow.withScope(context); + try { + // TODO: Is there a way to have asyncEnd _after_ the continuation? + } finally { + scope[SymbolDispose](); + } } function onRejectWithRethrow(err) { @@ -563,38 +596,44 @@ class TracingChannel { function onResolve(result) { context.result = result; // Use continuation window for asyncStart/asyncEnd - // eslint-disable-next-line no-unused-vars - using scope = continuationWindow.withScope(context); - // TODO: Is there a way to have asyncEnd _after_ the continuation? - return result; + const scope = continuationWindow.withScope(context); + try { + // TODO: Is there a way to have asyncEnd _after_ the continuation? + return result; + } finally { + scope[SymbolDispose](); + } } - // eslint-disable-next-line no-unused-vars - using scope = this.#callWindow.withScope(context); + const scope = this.#callWindow.withScope(context); try { - const result = ReflectApply(fn, thisArg, args); - // If the return value is not a thenable, return it directly with a warning. - // Do not publish to asyncStart/asyncEnd. - if (typeof result?.then !== 'function') { - emitNonThenableWarning(fn); - context.result = result; + try { + const result = ReflectApply(fn, thisArg, args); + // If the return value is not a thenable, return it directly with a warning. + // Do not publish to asyncStart/asyncEnd. + if (typeof result?.then !== 'function') { + emitNonThenableWarning(fn); + context.result = result; + return result; + } + // isPromise() matches sub-classes, but we need to match only direct + // instances of the native Promise type to safely use PromisePrototypeThen. + if (isPromise(result) && ObjectGetPrototypeOf(result) === PromisePrototype) { + return PromisePrototypeThen(result, onResolve, onRejectWithRethrow); + } + // For non-native thenables, subscribe to the result but return the + // original thenable so the consumer can continue handling it directly. + // Non-native thenables don't have unhandledRejection tracking, so + // swallowing the rejection here doesn't change existing behaviour. + result.then(onResolve, onReject); return result; + } catch (err) { + context.error = err; + error.publish(context); + throw err; } - // isPromise() matches sub-classes, but we need to match only direct - // instances of the native Promise type to safely use PromisePrototypeThen. - if (isPromise(result) && ObjectGetPrototypeOf(result) === PromisePrototype) { - return PromisePrototypeThen(result, onResolve, onRejectWithRethrow); - } - // For non-native thenables, subscribe to the result but return the - // original thenable so the consumer can continue handling it directly. - // Non-native thenables don't have unhandledRejection tracking, so - // swallowing the rejection here doesn't change existing behaviour. - result.then(onResolve, onReject); - return result; - } catch (err) { - context.error = err; - error.publish(context); - throw err; + } finally { + scope[SymbolDispose](); } } @@ -615,23 +654,29 @@ class TracingChannel { } // Use continuation window for asyncStart/asyncEnd around callback - // eslint-disable-next-line no-unused-vars - using scope = continuationWindow.withScope(context); - return ReflectApply(callback, this, arguments); + const scope = continuationWindow.withScope(context); + try { + return ReflectApply(callback, this, arguments); + } finally { + scope[SymbolDispose](); + } } const callback = ArrayPrototypeAt(args, position); validateFunction(callback, 'callback'); ArrayPrototypeSplice(args, position, 1, wrappedCallback); - // eslint-disable-next-line no-unused-vars - using scope = this.#callWindow.withScope(context); + const scope = this.#callWindow.withScope(context); try { - return ReflectApply(fn, thisArg, args); - } catch (err) { - context.error = err; - error.publish(context); - throw err; + try { + return ReflectApply(fn, thisArg, args); + } catch (err) { + context.error = err; + error.publish(context); + throw err; + } + } finally { + scope[SymbolDispose](); } } } diff --git a/test/parallel/test-diagnostics-channel-no-erm.js b/test/parallel/test-diagnostics-channel-no-erm.js new file mode 100644 index 00000000000000..e42b014cf63754 --- /dev/null +++ b/test/parallel/test-diagnostics-channel-no-erm.js @@ -0,0 +1,20 @@ +'use strict'; +require('../common'); +const assert = require('node:assert'); +const { spawnSync } = require('node:child_process'); + +// This test ensures that using diagnostics_channel does not cause a segfault +// when explicit resource management is disabled in V8. +// Refs: https://github.com/nodejs/node/issues/64230 + +const child = spawnSync(process.execPath, [ + '--no-js-explicit-resource-management', + '--eval', + 'require("diagnostics_channel").tracingChannel("foo").traceSync(() => {})' +]); + +// If it segfaults, signal will be something like 'SIGSEGV'. +// We assert that the process exited cleanly (status 0). +assert.strictEqual(child.signal, null); +assert.strictEqual(child.status, 0); +assert.strictEqual(child.stderr.toString(), ''); \ No newline at end of file