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

diagnostics_channel: replace using with try-finally#64251

Open
ayush23chaudhary wants to merge 1 commit into
nodejs:mainfrom
ayush23chaudhary:fix-erm-diagnostics
Open

diagnostics_channel: replace using with try-finally#64251
ayush23chaudhary wants to merge 1 commit into
nodejs:mainfrom
ayush23chaudhary:fix-erm-diagnostics

Conversation

@ayush23chaudhary

@ayush23chaudhary ayush23chaudhary commented Jul 2, 2026

Copy link
Copy Markdown

Fixes: #64230

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 AI review requested due to automatic review settings July 2, 2026 04:13
@nodejs-github-bot 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 AI reviewed Jul 2, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread lib/diagnostics_channel.js Outdated
Comment thread lib/diagnostics_channel.js Outdated
Comment thread lib/diagnostics_channel.js Outdated
Renegade334
Renegade334 reviewed Jul 2, 2026
View reviewed changes
Comment thread lib/diagnostics_channel.js Outdated
anonrig
anonrig reviewed Jul 2, 2026
View reviewed changes

@anonrig anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add at least a test

Renegade334
Renegade334 requested changes Jul 2, 2026
View reviewed changes
Comment thread lib/diagnostics_channel.js Outdated
Comment thread lib/diagnostics_channel.js
Comment thread lib/diagnostics_channel.js Outdated
@ayush23chaudhary

ayush23chaudhary commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Renegade334
Renegade334 reviewed Jul 2, 2026
View reviewed changes

@Renegade334 Renegade334 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your changes! Some more comments 👍

Comment thread lib/diagnostics_channel.js Outdated
Comment thread lib/diagnostics_channel.js Outdated
Comment thread lib/diagnostics_channel.js Outdated
Comment thread lib/diagnostics_channel.js Outdated
Comment thread test/parallel/test-diagnostics-channel-no-erm.js Outdated
Comment thread lib/diagnostics_channel.js Outdated
@ayush23chaudhary

ayush23chaudhary commented Jul 3, 2026

Copy link
Copy Markdown
Author

@Renegade334!

  1. I completely flattened the nested try...finally blocks in traceSync, tracePromise, and traceCallback.

  2. I removed the empty try...finally blocks around the continuation window disposals.

  3. I updated the test script to use the // Flags: directive, completely removing the spawnSync boilerplate

  4. As noted, I also reverted RunStoresScope back to the simpler reverse-loop without the synchronous error logic

@Renegade334

Renegade334 commented Jul 3, 2026

Copy link
Copy Markdown
Member

LGTM, but please run git commit --amend --no-edit --signoff to indicate acceptance of the Developer's Certificate of Origin, and make lint-js-fix to satisfy the linter.

@ayush23chaudhary

ayush23chaudhary commented Jul 3, 2026

Copy link
Copy Markdown
Author

@ayush23chaudhary
diagnostics_channel: replace using with try-finally
0be0ec6 
Signed-off-by: Ayush Chaudhary <ayush23chaudhary@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonrig anonrig anonrig left review comments
Copilot code review Copilot Copilot left review comments
@Renegade334 Renegade334 Awaiting requested review from Renegade334

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

diagnostics_channel Issues and PRs related to diagnostics channel needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Segfaults with [await] using within node core

5 participants

@ayush23chaudhary @Renegade334 @anonrig @nodejs-github-bot