Skip to content

async_hooks: clear context frame for thrown microtasks#64147

Open
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-microtask-async-context-frame
Open

async_hooks: clear context frame for thrown microtasks#64147
mcollina wants to merge 1 commit into
nodejs:mainfrom
mcollina:fix-microtask-async-context-frame

Conversation

@mcollina

@mcollina mcollina commented Jun 26, 2026

Copy link
Copy Markdown
Member

This clears the AsyncContextFrame before rethrowing exceptions from queueMicrotask() callbacks.

V8 restores continuation-preserved embedder data for microtasks. If a microtask callback throws, exception reporting can re-enter JavaScript while the microtask's context frame is still current. Clearing the frame before rethrowing keeps subsequent exception formatting and microtasks from observing the throwing callback's AsyncLocalStorage state.

Includes a regression test covering a non-Error object thrown from a microtask whose string conversion queues another microtask.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jun 26, 2026
@mcollina

Copy link
Copy Markdown
Member Author

cc @nodejs/diagnostics

@Qard Qard 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.

Don't we generally want the context to be available in uncaughtException? We did a bunch of work at one point to ensure it would be there so uncaughtException handlers could attribute the errors to spans they came from in tracers. It'd probably be more correct to cut off the context after those handlers run. 🤔

@mcollina

Copy link
Copy Markdown
Member Author

Don't we generally want the context to be available in uncaughtException?

I thought as well.

The key assertion here is this one: https://github.com/nodejs/node/pull/64147/changes#diff-6c14bb576b8a1eaf12aeb421aec5efd35b58d92c8a02d668a83a2f74f9c6474fR18.

@mcollina

Copy link
Copy Markdown
Member Author

Note that if we think the current behavior is correct, then I'll add a test for it to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants