refactor(js): unify env lifecycle refcounting#53
Conversation
Module lifecycle used a bare atomic while js.io() had a mutex-guarded refcount; the split allowed an init/cleanup race across concurrent N-API envs and skipped cleanup when export registration failed after a successful init. SharedResource serializes hooks under a lock and only increments after init succeeds, so both hold by construction. Refs #31, #52 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@GrapeBaBa The changes in #31 and #52 were using the pattern and logic we already have in See if that seems good to you. |
| /// holder). | ||
| /// | ||
| /// Each instantiation owns its own refcount, mutex, and `T` instance. | ||
| pub fn SharedResource(comptime T: type, comptime hooks: anytype) type { |
There was a problem hiding this comment.
should we add a test that exercises the mutex?
There was a problem hiding this comment.
Yes added the tests for it.
| } | ||
|
|
||
| /// Returns the shared instance, or null when no holder is active. | ||
| pub fn get() ?*T { |
There was a problem hiding this comment.
im assuming the resource has to be retained for this to be threadsafe?
There was a problem hiding this comment.
Yes correct, added to docs.
| /// holder). | ||
| /// | ||
| /// Each instantiation owns its own refcount, mutex, and `T` instance. | ||
| pub fn SharedResource(comptime T: type, comptime hooks: anytype) type { |
There was a problem hiding this comment.
I’m not sure this abstraction is necessary. Different resource types may not share the same lifecycle state machine. If one resource’s lifecycle evolves independently, some transitions or invariants may have to be handled outside SharedResource, splitting a single state machine between the generic helper and its callers. That could make lifecycle correctness harder to reason about.
If we keep this abstraction, I think get() should make its lifetime requirements explicit. It returns *T after releasing the mutex, so SharedResource itself does not guarantee that the instance remains valid during use. Consider returning a lease that holds a retain until released. At minimum, document that the caller must already hold a retain for the entire lifetime of the returned pointer.
There was a problem hiding this comment.
The abstraction is deliberately scoped to the one state machine we already have twice: env-scoped refcounted init/teardown. Having it in two copies is what produced #31 and #52.
It's also internal — not exported from js.zig — so if a future resource's lifecycle doesn't fit, it just doesn't use this helper.
On get() agreed the lifetime requirement should be explicit — I've documented that the returned pointer is only valid while the caller holds a retain.
There was a problem hiding this comment.
i also like that this makes the lifecycle concern in zapi rather than consumer
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
spiral-ladder
left a comment
spiral-ladder
left a comment
There was a problem hiding this comment.
this generally lgtm, but will wait on @GrapeBaBa for second opinion
3 participants
Motivation
export_module.zig(bare atomic) andjs/io.zig(mutex-guarded refcount) duplicate the same retain/release lifecycle. That split is the root cause of both open fixes: the cross-env init race (#31) and the missed cleanup on registration failure (#52).Description
lifecycle.SharedResource(T, hooks): a mutex-guarded refcount that runs fallibleinitunder the lock before incrementing, andcleanupafter decrementing.js/io.zigand the module lifecycle inexport_module.zigonto it. No public API changes.Both fixes hold by construction:
initsucceeds — fixes the fix(js): serialize moduleInit/cleanupHook across N-API envs #31 race, with registration kept outside the lock.release(), which always runscleanupwith the correct count — fixes fix(js): roll back module lifecycle initialization #52.Supersedes #31 and #52.
Testing
New tests in
lifecycle.zigcover hook ordering, init-failure rollback, and instance visibility.zig build test: 54/54 pass.🤖 Generated with Claude Code