diagnostics_channel: replace using with try-finally by ayush23chaudhary · Pull Request #64251 · nodejs/node

ayush23chaudhary · 2026-07-02T04:13:35Z

Description

This PR replaces instances of the experimental using syntax with try...finally blocks inside lib/diagnostics_channel.js.

Since Explicit Resource Management is still a flagged feature in V8, encountering using in core causes a segfault when running Node with the --no-js-explicit-resource-management flag. This refactor maintains the exact same resource cleanup semantics by manually calling [SymbolDispose]() in a finally block, avoiding the flagged syntax entirely.

Example of Refactor

Before:

using scope = this.withStoreScope(data);
return ReflectApply(fn, thisArg, args);

After

const scope = this.withStoreScope(data);
try {
  return ReflectApply(fn, thisArg, args);
} finally {
  scope[SymbolDispose]();
}

Copilot

Pull request overview

This PR removes the experimental using syntax from lib/diagnostics_channel.js, replacing it with equivalent try...finally disposal to avoid crashes when Node is run with --no-js-explicit-resource-management.

Changes:

Replaced using statements with explicit try...finally blocks that invoke [SymbolDispose]().
Updated scope handling in ActiveChannel, BoundedChannel, and TracingChannel paths to ensure disposal occurs reliably without relying on flagged syntax.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Renegade334 · 2026-07-02T07:59:52Z

-              triggerUncaughtException(err, false);
-            });
-            continue;
+    const stack = new DisposableStack();


DisposableStack is also a flagged global.

@Renegade334, I did not realize DisposableStack was part of the same flagged feature. I've replaced it with a standard array that manually iterates in reverse during disposal. Just pushed the update.

anonrig

Please add at least a test

Renegade334 · 2026-07-02T15:06:12Z

+    } finally {
+      if (this.#stack === undefined) {
+        for (let i = stack.length - 1; i >= 0; i--) {
+          stack[i][SymbolDispose]();
+        }
+      }
+    }


#stack is not disposed by the constructor. This code should be removed, and the try ... finally block in the constructor is not needed.

Renegade334 · 2026-07-02T15:10:37Z

+      for (let i = this.#stack.length - 1; i >= 0; i--) {
+        this.#stack[i][SymbolDispose]();
+      }
+      this.#stack = undefined;


Suggested change

for (let i = this.#stack.length - 1; i >= 0; i--) {

this.#stack[i][SymbolDispose]();

}

this.#stack = undefined;

const stack = this.#stack;

this.#stack = undefined;

for (let i = stack.length - 1; i >= 0; i--) {

stack[i][SymbolDispose]();

}

Matches the order of operations of DisposableStack.prototype.dispose(), in case anything touches the stack during disposal.

Can any of the store disposers ever throw? If so, we need error handling here as well.

ayush23chaudhary · 2026-07-02T16:21:30Z

Thanks for the thorough review, @Renegade334 and @anonrig!

I've updated RunStoresScope to perfectly match the suggested error handling and disposal order (NodeAggregateError).
I've added a test in test/parallel/test-diagnostics-channel-no-erm.js to ensure Node does not segfault when run with --no-js-explicit-resource-management.
I squashed everything into a single commit to resolve the commit linter failure.

Renegade334

Thanks for your changes! Some more comments 👍

Renegade334 · 2026-07-02T21:54:24Z

+      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]();


There's no need to nest this, it can just be try ... catch ... finally.

Suggested change

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]();

const result = ReflectApply(fn, thisArg, args);

context.result = result;

return result;

} catch (err) {

context.error = err;

error.publish(context);

throw err;

} finally {

scope[SymbolDispose]();

Renegade334 · 2026-07-02T21:54:57Z

+      try {
+        // TODO: Is there a way to have asyncEnd _after_ the continuation?
+      } finally {
+        scope[SymbolDispose]();
+      }


No need for an empty block here.

Suggested change

try {

// TODO: Is there a way to have asyncEnd _after_ the continuation?

} finally {

scope[SymbolDispose]();

}

// TODO: Is there a way to have asyncEnd _after_ the continuation?

scope[SymbolDispose]();

Renegade334 · 2026-07-02T21:57:16Z

+      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;
      }


Same here.

Suggested change

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;

}

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;

Renegade334 · 2026-07-02T21:57:53Z

+      try {
+        return ReflectApply(fn, thisArg, args);
+      } catch (err) {
+        context.error = err;
+        error.publish(context);
+        throw err;
+      }
+    } finally {
+      scope[SymbolDispose]();


Same here.

Suggested change

try {

return ReflectApply(fn, thisArg, args);

} catch (err) {

context.error = err;

error.publish(context);

throw err;

}

} finally {

scope[SymbolDispose]();

return ReflectApply(fn, thisArg, args);

} catch (err) {

context.error = err;

error.publish(context);

throw err;

} finally {

scope[SymbolDispose]();

Renegade334 · 2026-07-02T22:00:47Z

+const child = spawnSync(process.execPath, [
+  '--no-js-explicit-resource-management',
+  '--eval',
+  'require("diagnostics_channel").tracingChannel("foo").traceSync(() => {})'
+]);


The test runner can handle this abstraction for you. Just place a line like the following at the top of the script:

// Flags: --no-js-explicit-resource-management

and the test script will be run under those runtime flags, without needing to spawn a new process. See https://github.com/nodejs/node/blob/main/test/parallel/test-permission-has.js for an example.

Renegade334 · 2026-07-02T22:07:17Z

+      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);
+      }
+    }


I've had a look, and all of the scope disposers should never throw synchronously (they wrap their code in their own try ... catch blocks and emit errors asynchronously). As such, I don't think we need to implement this, and we can use your original version without the error handling.

Copilot AI review requested due to automatic review settings July 2, 2026 04:13

nodejs-github-bot added diagnostics_channel Issues and PRs related to diagnostics channel needs-ci PRs that need a full CI run. labels Jul 2, 2026

Copilot started reviewing on behalf of ayush23chaudhary July 2, 2026 04:14 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

Comment thread lib/diagnostics_channel.js

Comment thread lib/diagnostics_channel.js

Comment thread lib/diagnostics_channel.js

Renegade334 reviewed Jul 2, 2026

View reviewed changes

anonrig reviewed Jul 2, 2026

View reviewed changes

Renegade334 requested changes Jul 2, 2026

View reviewed changes

diagnostics_channel: replace using with try-finally

1d66ae6

ayush23chaudhary force-pushed the fix-erm-diagnostics branch from 548c1c1 to 1d66ae6 Compare July 2, 2026 16:20

ayush23chaudhary requested a review from Renegade334 July 2, 2026 17:20

Renegade334 reviewed Jul 2, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

ayush23chaudhary commented Jul 2, 2026

Description

Example of Refactor

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Renegade334 Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayush23chaudhary commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Renegade334 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Renegade334 Jul 2, 2026 •

edited

Loading

ayush23chaudhary commented Jul 2, 2026 •

edited

Loading