fix: serialize moduleInit/cleanupHook across N-API envs#31
Open
GrapeBaBa wants to merge 1 commit into
Open
Conversation
`exportModule` atomically incremented `env_refcount`, but the user's `options.init` and the subsequent `registerDecls` / `options.register` calls were not serialized across environments. When two N-API environments (e.g. Node.js Worker threads loading the same addon at roughly the same time) call into `moduleInit` concurrently, the "second" env's `fetchAdd` returns a non-zero `prev_refcount` and skips the user's init, but it then proceeds to `registerDecls` and exposes the module's exports to JS while the "first" env is still inside the user's `init` hook. JS code on the second env can observe partially-initialized shared state. Guard `moduleInit`'s lifecycle path and `cleanupHook` with a per-module spinlock (same pattern already used in `class_runtime.zig`, introduced in ChainSafe#20). The critical section stays small and uncontended in the common case (single env) — one CAS on attach and one store on detach.
wemeetagain
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
moduleInitatomically incrementsState.env_refcountto decide which caller is the "first" env, but the user'soptions.initand the subsequentregisterDecls/options.registerare not serialized between concurrent environments. When two N-API envs attach near-simultaneously (e.g. a Node.js main thread plus a Worker threadrequire()-ing the same addon), the "second" env'sfetchAddreturns a non-zeroprev_refcount, skips the user's init, and proceeds to expose exports to its JS isolate while the first env is still inside the user'sinithook. JS code on the second env can then observe partially-initialized shared state. The same race applies betweencleanupHookand a freshmoduleInit.Description
Guard the lifecycle path in
moduleInitand the body ofcleanupHookwith a per-module spinlock, reusing the same primitive introduced forclass_runtime.zigin #20 (std.atomic.Value(bool)+cmpxchgWeak+spinLoopHint). No newstd.Thread/std.Iodependency. One CAS on attach, one atomic store on detach; uncontended in the single-env case.addEnvCleanupHookis called while the lock is held, but N-API never invokes the hook synchronously from registration, so no self-deadlock.zig build testpasses.