Skip to content

fix(ext/napi): defer GC weak-callback finalizers to the event loop#33260

Open
bartlomieju wants to merge 1 commit intomainfrom
fix/napi-defer-gc-finalizers
Open

fix(ext/napi): defer GC weak-callback finalizers to the event loop#33260
bartlomieju wants to merge 1 commit intomainfrom
fix/napi-defer-gc-finalizers

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

Summary

  • Defer napi Reference weak-callback finalizers from V8 GC to the next event loop tick via V8CrossThreadTaskSpawner, matching Node.js's EnqueueFinalizer pattern
  • Implement node_api_post_finalizer (was a no-op)
  • Add regression test verifying finalizers run after GC, not during

Background

Deep comparison of Node-API implementations between Node.js and Deno revealed that Deno was calling user-provided finalizer callbacks directly during V8 GC pauses. This is unsafe — finalizers can re-enter V8, allocate GC'd objects, or trigger cascading weak callbacks, all of which are undefined behavior during a GC pause. Node.js avoids this by deferring finalizers via EnqueueFinalizer to the event loop.

This is the most likely root cause of the intermittent napi_unwrap failures reported in #33137 (Vite 8 / rolldown builds). The timing-dependent nature of the bug matches: it only manifests when GC fires at a particular moment during napi class instance lifecycle.

Test plan

  • New deferred_finalizer_test.js: creates an external with a finalizer, triggers GC, asserts finalizer has NOT run yet, yields to event loop, asserts finalizer HAS run
  • All existing napi tests pass (115 passed, 1 pre-existing failure in cross_env_test.js)
  • cargo check, tools/format.js, tools/lint.js all clean

Ref #33137

🤖 Generated with Claude Code

Previously, napi Reference weak-callback finalizers were called directly
during V8 GC. This is unsafe because user-provided finalizer callbacks
can re-enter V8, allocate GC'd objects, or trigger cascading weak
callbacks — all of which lead to undefined behavior during a GC pause.

Node.js avoids this by deferring finalizers via EnqueueFinalizer so they
run on the next event loop tick. This commit adopts the same pattern
using V8CrossThreadTaskSpawner to schedule deferred finalizer execution.

Also implements node_api_post_finalizer which was previously a no-op.

Ref #33137

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Copy link
Copy Markdown

@miracatbot miracatbot left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Deferring weak-callback finalizers off the V8 GC path is the right fix here. The callback/data/env handling in Reference::weak_callback() looks careful, and removing the finalizer from the ref tracker before scheduling should avoid double-finalization during shutdown.

The regression test also checks the important behavioral contract: not during gc(), yes after yielding back to the event loop.

@bartlomieju bartlomieju requested a review from nathanwhit April 15, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants