refactor(napi_wrap): make user provide Ref#10
Merged
Conversation
We still want the option of the user providing the `Ref` if they want to hold the reference to the created napi object. This is a fix for bls integration on lodestar suffering from a memory leak - because our usage of our bls bindings in lodestar-z involves us doing a `_ = Env.wrap`, we drop the reference to the napi object while increasing the refcount, which means our finalize calls from the v8 GC is never called.
wemeetagain
approved these changes
Mar 24, 2026
wemeetagain
changed the title
RefRef
lodekeeper-z
added a commit
to lodekeeper-z/zapi
that referenced
this pull request
Mar 25, 2026
…r callbacks) Updates examples to match new wrap() signature from ChainSafe#10 (added ref parameter). 🤖 Generated with AI assistance
nazarhussain
pushed a commit
that referenced
this pull request
Apr 28, 2026
* chore: upgrade to Zig 0.16.0-dev (master) - Remove usingnamespace (removed in 0.15): c.zig now exports cImport as .c field - All imports updated: @import("c.zig") → @import("c.zig").c - callconv(.C) → callconv(.c) (lowercase) - napi module requires link_libc = true for @cImport - Example disabled (TODO: std.time.Timer/sleep moved to std.Io) 🤖 Generated with AI assistance * chore: merge upstream main (Ref refactor, raw variant functions, safer callbacks) Updates examples to match new wrap() signature from #10 (added ref parameter). 🤖 Generated with AI assistance * fix: restore hello_world example with Zig 0.16 Timer migration - Replace std.time.Timer with local Timer using std.c.clock_gettime - Replace std.time.sleep with std.c.nanosleep - Restore hello_world module, library, and test in build.zig - Fix b.modules.put to use b.allocator (Zig 0.16 API change) - Update minimum_zig_version to 0.16.0 - Update CI setup-env to use Zig 0.16.0 - Remove duplicate .zig-cache/ in .gitignore * refactor(examples): use std.Io instead of libc for timer and sleep Replace std.c.clock_gettime/nanosleep with std.Io.Timestamp and std.Io.sleep, using Io.Threaded.init_single_threaded for the Io instance in the NAPI callback context. * refactor(examples): use Io.Threaded.global_single_threaded Remove module-level var and getIo() helper. Use the stdlib-provided global_single_threaded instance directly. * refactor(examples): use explicit Io.Threaded init instead of global Per Zig author recommendation, prefer explicit one-time initialization over global_single_threaded for dynamically-loaded NAPI libraries. * refactor(examples): remove Timer wrapper, use Io.Timestamp directly The custom Timer struct was an unnecessary wrapper around Io.Timestamp. Use Io.Timestamp directly in the NAPI callbacks. * fix(build): exclude example tests from global test step Example modules reference NAPI symbols that are only available at runtime via Node.js. Running them as standalone zig tests causes linker errors. Keep them as individual test steps (test:example_hello_world, test:example_type_tag) but exclude from the global `zig build test`. * fix: address PR review comments - Remove stale "generated by zbuild" comment from build.zig.zon - Update zbuild.zon minimum_zig_version to 0.16.0 - Add link_libc to zapi module in zbuild.zon for consistency with build.zig * fix: use threadlocal for Io.Threaded to avoid cross-thread sharing Each thread (main, libuv worker, spawned) gets its own init_single_threaded instance, which is correct since init_single_threaded has no synchronization. * fix: restore zbuild generated-file headers build.zig and build.zig.zon are generated by zbuild — keep the header so manual edits don't get silently overwritten on regeneration. * fix: remove unnecessary link_libc from zapi module zapi only uses @cImport for NAPI C headers (stddef.h, stdint.h) which Zig provides natively. link_libc is only needed by the example dynamic libraries that load into Node.js at runtime. * example: use stdlib global_single_threaded Io in hello_world The threadlocal `init_single_threaded` instance was defensively scoped per thread on the theory that cross-thread sharing of a single-threaded Threaded could race. In practice stdlib already provides `std.Io.Threaded.global_single_threaded` for exactly this scenario: > In some cases such as debugging, it is desirable to hardcode a > reference to this `Io` implementation. It's a single process-wide instance, and — importantly — the only ops this example uses (`Timestamp.now`, `random`, etc.) bottom out in OS calls that don't touch the `Threaded` struct's internal mutable state, so one shared instance is safe for this use case. This removes the `threadlocal var` declaration entirely, matching the stdlib-recommended pattern for examples. * chore: port main's newly-merged code to Zig 0.16 The merge with main brought in the js_dsl feature (#11) and a refactor of `src/root.zig` (napi symbols moved to `src/napi.zig`, re-exported via `pub usingnamespace`). All of that was written for 0.14.1 and hits several 0.16 breaking changes: * `b.modules.put(key, value)` → `b.modules.put(allocator, key, value)`. Two call sites in `build.zig` were missed by the previous update pass (the `napi` module and the `example_js_dsl` module). * `src/napi.zig` exported `pub const c = @import("c.zig")`, but every internal file accesses napi types through the inner `.c` namespace. Restore the `.c` on the import. * `pub usingnamespace` was removed in Zig 0.16. Replace it in `src/root.zig` with explicit `pub const foo = napi.foo;` re-exports. * `callconv(.C)` → `callconv(.c)` (6 NAPI callback shims in `src/js/wrap_class.zig` and `src/js/wrap_function.zig`). * `std.Thread.Mutex` → `std.Io.Mutex`. The DSL's per-class registry in `src/js/class_runtime.zig` needs a 0.16-compatible mutex, which requires an `Io`. The `Io` is threaded in through the DSL's entry point rather than letting zapi carry global state: - `js.exportModule` gains an optional `.io = fn () std.Io` field in `options`. The generated `moduleInit` calls it after the user's `options.init` hook and passes the result down into `registerDecls` → `class_runtime.registerClass`. If `.io` is omitted, a DSL-local `init_single_threaded` fallback keeps zero-config modules compiling. - `registerClass` caches the resolved `Io` in per-`T` `state(T).io`; `getConstructor`, `materializeClassInstance`, and `cleanupHook` all read from this cache because they execute inside napi C callbacks that can't receive user-provided `Io`. - Normal paths (`registerClass`, `getConstructor`) use cancelable `try mutex.lock(io)` so caller cancellation can propagate. - `cleanupHook` (a napi C callback with a fixed `(*Entry)` signature that cannot return errors) uses `lockUncancelable` deliberately — unwinding on cancel would leak the entry and hand napi a dangling pointer. * `Env` gains a `fromRaw(raw_env)` constructor so the 19 napi C callback sites don't all hand-roll the `Env{ .env = ... }` literal. `Env` itself stays a pure `napi_env` wrapper — no `io` field. After this commit `zig build` succeeds, and `zig build test` runs the 42 regular tests green. `example_js_dsl` has a separate, pre-existing link issue in `zig build test` (undefined `_napi_wrap` and friends, resolved by Node at runtime when loading the `.node` file) — orthogonal to the 0.16 migration. * chore: migrate to zbuild library mode Replace the auto-generated 175-line build.zig + the deleted zbuild.zon CLI mode with the new zbuild library-mode pattern: a 6-line wrapper invoking `zbuild.configureBuild` over a manifest-driven build.zig.zon. build.zig: - shrunk from 175 lines to a wrapper that delegates to `zbuild.configureBuild(b, @import("build.zig.zon"), .{})`. - Post-processes the 3 example test artifacts to set `linker_allow_shlib_undefined = true` (zbuild exposes this option only for libraries, so we apply it post-hoc to tests that link against napi C symbols Node provides at dlopen time). build.zig.zon: - Adds zbuild as a dependency (pinned to refactor/comptime-library-rewrite @ 17c389b — same pin as lodestar-z `main`). - Declares all modules / libraries / tests / options_modules in the manifest; zbuild materializes them at build time. - All build steps from the previous build.zig are preserved (`build-lib:*`, `test:*`, `build-test:*`, `test`). Verified: `zig build` clean, `zig build test` passes 42/42, all 3 `.node` files install correctly, `zig fmt --check` clean. * chore: declare test linker_allow_shlib_undefined in manifest Bump zbuild to 7ec852f, which adds `linker_allow_shlib_undefined` support for `tests` entries (previously only allowed in `libraries`). This lets the example tests declare the flag directly in the manifest instead of post-processing the test artifacts in build.zig: build.zig: shrinks back to a 6-line wrapper (no post-hoc fixup loop). build.zig.zon: 3 example test entries each set `linker_allow_shlib_undefined = true`. * fix: remove obsolete zbuild manifest --------- Co-authored-by: lodekeeper-z <lodekeeper-z@users.noreply.github.com> Co-authored-by: Chen Kai <281165273grape@gmail.com> Co-authored-by: Chen Kai <grapebabamarch@outlook.com>
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.
We still want the option of the user providing the
Refif they want tohold the reference to the created napi object.
This is a fix for bls integration on lodestar suffering from a memory leak - because our usage of our bls bindings in lodestar-z involves us doing a
_ = Env.wrap,we drop the reference to the napi object while increasing the refcount,
which means our finalize calls from the v8 GC is never called.