fix: serialize moduleInit/cleanupHook across N-API envs#31
Open
GrapeBaBa wants to merge 4 commits 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
previously approved these changes
May 13, 2026
spiral-ladder
pushed a commit
to ChainSafe/lodestar-z
that referenced
this pull request
May 25, 2026
## Motivation `ThreadPool` worker hot loops use `.acquire` on `err_flag.load()` where `.monotonic` suffices — the flag is a pure early-exit signal with no data dependency on the setter's other writes. BLS verification is CPU-intensive, so relaxing this in the inner loop avoids unnecessary memory-fence cost. Matches the pattern already used in `src/state_transition/cache/pubkey_cache.zig`. The earlier NAPI init-mutex changes from this branch have been dropped after merging main, because main moved to zapi-managed lifecycle (`js.exportModule` with `init`/`cleanup` hooks). The concurrent-register race they were guarding against has been filed against zapi upstream: ChainSafe/zapi#31. ## Description `src/bls/ThreadPool.zig`: - `err_flag.load(.acquire)` → `.monotonic` in `VerifyMultiWorkItem.exec` and `AggVerifyWorkItem.exec` worker loops. - Setter side (`err_flag.store(true, .release)` on pairing failure) is unchanged — release semantics on the producer side carry no obligation on the consumer to also be `.acquire` when the consumer doesn't depend on the producer's other writes.
markolazic01
pushed a commit
to markolazic01/lodestar-z
that referenced
this pull request
Jun 17, 2026
## Motivation `ThreadPool` worker hot loops use `.acquire` on `err_flag.load()` where `.monotonic` suffices — the flag is a pure early-exit signal with no data dependency on the setter's other writes. BLS verification is CPU-intensive, so relaxing this in the inner loop avoids unnecessary memory-fence cost. Matches the pattern already used in `src/state_transition/cache/pubkey_cache.zig`. The earlier NAPI init-mutex changes from this branch have been dropped after merging main, because main moved to zapi-managed lifecycle (`js.exportModule` with `init`/`cleanup` hooks). The concurrent-register race they were guarding against has been filed against zapi upstream: ChainSafe/zapi#31. ## Description `src/bls/ThreadPool.zig`: - `err_flag.load(.acquire)` → `.monotonic` in `VerifyMultiWorkItem.exec` and `AggVerifyWorkItem.exec` worker loops. - Setter side (`err_flag.store(true, .release)` on pairing failure) is unchanged — release semantics on the producer side carry no obligation on the consumer to also be `.acquire` when the consumer doesn't depend on the producer's other writes.
…ifecycle # Conflicts: # src/js/export_module.zig
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 environment, butoptions.initand the subsequentregisterDecls/options.registercalls are not serialized between concurrent environments. When two N-API environments attach near-simultaneously, the second environment can expose exports while the first is still initializing shared state. The same race applies betweencleanupHookand a freshmoduleInit.Description
Guard the lifecycle path in
moduleInitand the lifecycle portion ofcleanupHookwith a per-modulestd.Io.Mutex. Each attach first retains the shared IO context, then obtains itsstd.Iointerface and callslockUncancelable; this keeps the IO implementation alive while a contending attach waits. Unlocking uses the same retainedstd.Iointerface, without depending directly on thestd.Io.Threadedbackend.The initialization critical section covers the lifecycle refcount update,
options.init, automatic and custom export registration, and cleanup-hook registration. Error rollback remains inside the lock. Cleanup keepsjs.io()retained whileoptions.cleanupruns, then releases the lifecycle lock before releasing the shared IO context.