feat: Node-API (napi) surface for plugin developers - #437
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a V8-backed Node-API runtime for NativeScript. The change adds public Node-API headers, runtime environment management, addon loading, async APIs, thread-safe functions, test addons, coverage tests, documentation, and Xcode wiring. ChangesNode-API runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant JavaScript
participant ModuleInternal
participant NapiModules
participant NapiEnv
participant RuntimeLoop
JavaScript->>ModuleInternal: require registered addon
ModuleInternal->>NapiModules: request context exports
NapiModules->>NapiEnv: initialize or retrieve cached exports
NapiEnv-->>JavaScript: return addon exports
JavaScript->>NapiEnv: queue async work or thread-safe call
NapiEnv->>RuntimeLoop: schedule JavaScript callback
RuntimeLoop-->>JavaScript: deliver callback
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
NativeScript/napi/NodeApiEmbed.mm (1)
85-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
contextparameter is ignored.
GetExportsreceivescontextbut entersenv->context()instead. If a caller ever passes a context other than the environment's main context, the addon'sexportsobject is created in the isolate but the register function runs under a different context than the caller expects. Either use the passedcontextor remove the parameter to make the contract explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/napi/NodeApiEmbed.mm` around lines 85 - 107, Update NapiModules::GetExports to honor its context parameter by scoping execution to the passed context instead of env->context(). Keep the exports creation and module registration flow unchanged, ensuring the register function runs in the caller-provided context.NativeScript/napi/NapiThreadSafeFunction.mm (1)
426-426: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
shared_from_this()throws if the last reference is already gone.Both call sites call
func->shared_from_this()on a raw handle supplied by the addon. If the control block's use count already reached zero,shared_from_this()throwsstd::bad_weak_ptr. These functions areextern "C", so the exception escapes across the C ABI and terminates the process instead of returningnapi_invalid_arg.The registry keeps the object alive while
threadCount > 0, so a correct addon never hits this. Consider looking the handle up inRegistry()and returningnapi_invalid_argwhen it is absent, which converts an addon bug into a status code.Also applies to: 451-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/napi/NapiThreadSafeFunction.mm` at line 426, Update both dispatch call sites around PostDispatch to look up the raw function handle in Registry() before obtaining ownership, and return napi_invalid_arg when no live entry exists. Avoid calling func->shared_from_this() directly on an unvalidated handle so std::bad_weak_ptr cannot escape the extern "C" boundary; preserve normal dispatch behavior for registered handles.NativeScript/napi/shim/util-inl.h (1)
32-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMark
OnScopeLeaveas[[nodiscard]].Upstream Node declares this factory with
MUST_USE_RESULT. If a caller discards the returned guard, the temporary is destroyed at the end of the full expression andfnruns immediately instead of at scope exit. The attribute turns that misuse into a compiler warning.♻️ Proposed attribute addition
// Runs `fn` when the returned guard leaves scope, however it is left. template <typename Fn> -inline OnScopeLeaveImpl<Fn> OnScopeLeave(Fn&& fn) { +[[nodiscard]] inline OnScopeLeaveImpl<Fn> OnScopeLeave(Fn&& fn) { return OnScopeLeaveImpl<Fn>(std::move(fn)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/napi/shim/util-inl.h` around lines 32 - 35, Mark the OnScopeLeave factory function as [[nodiscard]] so callers receive a compiler warning when they discard the returned scope guard. Preserve its existing forwarding and OnScopeLeaveImpl construction behavior.NativeScript/napi/shim/js_native_api_v8_internals.h (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<cstring>for thememcpyused by the vendored sources.
js_native_api_v8.hline 316 callsmemcpyinV8LocalValueFromJsValue. This shim is the only header that supplies standard library includes to the vendored files, and it does not include<cstring>. The build works today only because libc++ pullsmemcpyin transitively through<string>or<memory>. Make the dependency explicit.♻️ Proposed include addition
+#include <cstring> `#include` <memory> `#include` <string> `#include` <string_view> `#include` <unordered_set> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/napi/shim/js_native_api_v8_internals.h` around lines 19 - 23, Add an explicit <cstring> include to js_native_api_v8_internals.h alongside the existing standard-library includes so the vendored V8LocalValueFromJsValue memcpy usage has a direct declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/node-api.md`:
- Around line 10-21: Update Add to validate that argc is at least 2 before
accessing args[0] and args[1], returning an appropriate N-API error for
insufficient arguments. Check and handle the status from each
napi_get_value_double and napi_create_double call, propagating failures instead
of returning an incorrect result.
In `@NativeScript/napi/NapiEnv.mm`:
- Around line 105-120: Set tearingDown_ to true at the start of the teardown
sequence, before NapiRunEnvCleanupHooks and all reference/finalizer callbacks
execute. Preserve the existing cleanup, thread-safe-function abort,
DrainFinalizers, and RefTracker::FinalizeAll ordering while ensuring
can_call_into_js() rejects JavaScript entry throughout teardown.
In `@NativeScript/napi/NapiThreadSafeFunction.mm`:
- Around line 306-308: Enter the relevant V8 context before JS-visible callbacks
at both affected sites: in NativeScript/napi/NapiThreadSafeFunction.mm lines
306-308, add a Context::Scope after the existing isolate/handle scopes and
before FinalizeOnJsThread; in NativeScript/napi/NodeApiEmbed.mm lines 170-175,
add the same scope after HandleScope and before ReportToJsHandlersAndLog.
- Around line 129-146: In the thread-safe function cleanup flow, drain all
entries in undelivered before invoking finalizeCb, passing the existing context
to callJs with null env and null callback data; only call finalizeCb after the
queue is empty. Preserve callbackRef cleanup and the existing undelivered-item
handling while ensuring no callJs uses the context after CallFinalizer.
In `@NativeScript/napi/NodeApiEmbed.mm`:
- Around line 623-644: Bound concurrency for the worker block submitted by
napi_queue_async_work instead of using the unrestricted global concurrent queue.
Add a dedicated shared queue or semaphore with the intended async-work pool
width, acquire capacity before invoking work->execute, and release it on every
completion path while preserving cancellation handling and CompleteAsyncWork
scheduling.
- Around line 71-77: Update node_module_register to handle the incoming Node
node_module with a dedicated adapter rather than casting it to napi_module. Add
explicit handling for the node_module field layout and callback signatures,
while keeping napi_module registration separate through napi_module_register;
ensure NODE_BINDING_CONTEXT_AWARE_CPP internal modules register correctly
without interpreting the payload as another type.
In `@NativeScript/NapiRuntime.mm`:
- Around line 12-15: Update NativeScriptNapiEnv to avoid dereferencing the
potentially stale pointer returned by Runtime::GetCurrentRuntime; use
synchronized runtime ownership or a registry entry invalidated before Runtime
destruction, then read GetNapiEnv only from a confirmed-live Runtime while
preserving the nullptr result after teardown.
---
Nitpick comments:
In `@NativeScript/napi/NapiThreadSafeFunction.mm`:
- Line 426: Update both dispatch call sites around PostDispatch to look up the
raw function handle in Registry() before obtaining ownership, and return
napi_invalid_arg when no live entry exists. Avoid calling
func->shared_from_this() directly on an unvalidated handle so std::bad_weak_ptr
cannot escape the extern "C" boundary; preserve normal dispatch behavior for
registered handles.
In `@NativeScript/napi/NodeApiEmbed.mm`:
- Around line 85-107: Update NapiModules::GetExports to honor its context
parameter by scoping execution to the passed context instead of env->context().
Keep the exports creation and module registration flow unchanged, ensuring the
register function runs in the caller-provided context.
In `@NativeScript/napi/shim/js_native_api_v8_internals.h`:
- Around line 19-23: Add an explicit <cstring> include to
js_native_api_v8_internals.h alongside the existing standard-library includes so
the vendored V8LocalValueFromJsValue memcpy usage has a direct declaration.
In `@NativeScript/napi/shim/util-inl.h`:
- Around line 32-35: Mark the OnScopeLeave factory function as [[nodiscard]] so
callers receive a compiler warning when they discard the returned scope guard.
Preserve its existing forwarding and OnScopeLeaveImpl construction behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecce2acb-1d99-4eee-beb1-9ba0eedf5a6f
📒 Files selected for processing (33)
NativeScript/NapiRuntime.hNativeScript/NapiRuntime.mmNativeScript/napi/NapiEnv.hNativeScript/napi/NapiEnv.mmNativeScript/napi/NapiModules.hNativeScript/napi/NapiThreadSafeFunction.hNativeScript/napi/NapiThreadSafeFunction.mmNativeScript/napi/NodeApiEmbed.mmNativeScript/napi/shim/env-inl.hNativeScript/napi/shim/js_native_api_v8_internals.hNativeScript/napi/shim/util-inl.hNativeScript/napi/vendor/NOTICENativeScript/napi/vendor/README.mdNativeScript/napi/vendor/js_native_api.hNativeScript/napi/vendor/js_native_api_types.hNativeScript/napi/vendor/js_native_api_v8.ccNativeScript/napi/vendor/js_native_api_v8.hNativeScript/napi/vendor/node_api.hNativeScript/napi/vendor/node_api_types.hNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmNativeScript/runtime/js/promise-proxy.jsTestFixtures/NapiCoverageModule.mmTestFixtures/NapiTestModule.mmTestFixtures/NapiTestSupport.hTestRunner/app/tests/ErrorEventsTests.jsTestRunner/app/tests/NapiCoverageTests.jsTestRunner/app/tests/NapiTests.jsTestRunner/app/tests/index.jsdocs/README.mddocs/node-api.mdv8ios.xcodeproj/project.pbxproj
| // The symbol node-gyp addons built against Node's internal registration path | ||
| // emit instead of napi_module_register. | ||
| extern "C" NAPI_MODULE_EXPORT void node_module_register(void* mod); | ||
|
|
||
| extern "C" NAPI_MODULE_EXPORT void node_module_register(void* mod) { | ||
| napi_module_register(static_cast<napi_module*>(mod)); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Node.js struct node_module vs napi_module field layout node_module_register
💡 Result:
In Node.js, node_module and napi_module are distinct structures used for registering different types of native modules [1][2][3]. node_module is the internal structure used by Node.js for traditional C++ addons and built-in modules [1][4]. Its memory layout is defined as follows [1]: struct node_module { int nm_version; unsigned int nm_flags; void* nm_dso_handle; const char* nm_filename; node::addon_register_func nm_register_func; node::addon_context_register_func nm_context_register_func; const char* nm_modname; void* nm_priv; struct node_module* nm_link; }; napi_module is the structure used for Node-API (N-API) addons [2][3]. Its layout is defined as [2]: typedef struct napi_module { int nm_version; unsigned int nm_flags; const char* nm_filename; napi_addon_register_func nm_register_func; const char* nm_modname; void* nm_priv; void* reserved[5]; } napi_module; Key Differences and Registration: 1. Registration Process: Traditional addons use node_module_register to add themselves to the Node.js module registry [1][4]. Node-API addons use napi_module_register [2][3]. 2. Integration: The napi_module_register function acts as a compatibility layer [3]. When called, it internally converts the napi_module into a node_module by setting nm_register_func to a wrapper function (napi_module_register_cb) and subsequently calling node_module_register [3]. 3. Fields: napi_module includes a reserved[5] array to provide binary compatibility and future-proofing, which is absent in the node_module struct [1][2]. Additionally, node_module supports dual registration methods (direct or context-aware) via nm_register_func and nm_context_register_func, whereas napi_module focuses on a single registration function [1][2]. Note: The use of napi_module_register is now considered deprecated in favor of symbol-based registration (using NAPI_MODULE or NAPI_MODULE_INIT macros), which allows Node.js to load the module without requiring an explicit registration function call at the C++ constructor level [2][6].
Citations:
- 1: https://github.com/nodejs/node/blob/main/src/node.h
- 2: https://github.com/nodejs/node/blob/main/src/node_api.h
- 3: https://github.com/nodejs/node/blob/main/src/node_api.cc
- 4: https://www.mo4tech.com/8-27-nodejs-source-code-module-c-core-module-load-source-analysis.html
- 5: https://github.com/nodejs/node/blob/v14.9.0/src/node_api.h
- 6: https://github.com/nodejs/node/blob/d90d9d55/src/node_api.h
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate file ---'
cat -n NativeScript/napi/NodeApiEmbed.mm | sed -n '1,110p'
printf '%s\n' '--- registration symbols and related types ---'
rg -n --hidden -S 'node_module_register|napi_module_register|NAPI_MODULE|NODE_MODULE|struct node_module|typedef struct napi_module|napi_addon_register_func' . \
-g '!node_modules' -g '!build' -g '!dist'
printf '%s\n' '--- repository status and relevant diff summary ---'
git diff --stat -- NativeScript/napi/NodeApiEmbed.mmRepository: NativeScript/ios
Length of output: 9676
🏁 Script executed:
#!/bin/bash
set -eu
cat -n NativeScript/napi/NodeApiEmbed.mm | sed -n '1,110p'
rg -n --hidden -S 'node_module_register|napi_module_register|NAPI_MODULE|NODE_MODULE|struct node_module|typedef struct napi_module|napi_addon_register_func' . \
-g '!node_modules' -g '!build' -g '!dist'Repository: NativeScript/ios
Length of output: 9554
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- ModuleBinding.hpp ---'
cat -n NativeScript/runtime/ModuleBinding.hpp | sed -n '1,90p'
printf '%s\n' '--- vendored N-API declarations ---'
cat -n NativeScript/napi/vendor/node_api.h | sed -n '30,100p'
cat -n NativeScript/napi/vendor/node_api_types.h | sed -n '1,35p'
printf '%s\n' '--- module-related headers ---'
cat -n NativeScript/napi/NapiModules.h | sed -n '1,80p'
rg -n --hidden -S 'node_module|NODE_MODULE_VERSION|addon_register_func|nm_dso_handle|nm_context_register_func' NativeScript TestFixtures docs \
-g '!build' -g '!dist'Repository: NativeScript/ios
Length of output: 11494
🏁 Script executed:
#!/bin/bash
set -eu
cat -n NativeScript/runtime/ModuleBinding.hpp | sed -n '1,90p'
cat -n NativeScript/napi/vendor/node_api.h | sed -n '30,100p'
cat -n NativeScript/napi/vendor/node_api_types.h | sed -n '1,35p'
cat -n NativeScript/napi/NapiModules.h | sed -n '1,80p'
rg -n --hidden -S 'node_module|NODE_MODULE_VERSION|addon_register_func|nm_dso_handle|nm_context_register_func' NativeScript TestFixtures docs \
-g '!build' -g '!dist'Repository: NativeScript/ios
Length of output: 11401
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- complete ns_module definition ---'
cat -n NativeScript/runtime/ModuleBinding.hpp | sed -n '80,115p'
printf '%s\n' '--- declarations and calls of node_module_register ---'
rg -n -C 4 --hidden -S 'node_module_register\s*\(' NativeScript TestFixtures docs \
-g '!build' -g '!dist'
printf '%s\n' '--- standalone layout probe ---'
python3 - <<'PY'
import ctypes
class NapiModule(ctypes.Structure):
_fields_ = [
("nm_version", ctypes.c_int),
("nm_flags", ctypes.c_uint),
("nm_filename", ctypes.c_void_p),
("nm_register_func", ctypes.c_void_p),
("nm_modname", ctypes.c_void_p),
("nm_priv", ctypes.c_void_p),
("reserved", ctypes.c_void_p * 4),
]
class NodeModule(ctypes.Structure):
_fields_ = [
("nm_version", ctypes.c_int),
("nm_flags", ctypes.c_uint),
("nm_dso_handle", ctypes.c_void_p),
("nm_filename", ctypes.c_void_p),
("nm_register_func", ctypes.c_void_p),
("nm_context_register_func", ctypes.c_void_p),
("nm_modname", ctypes.c_void_p),
("nm_priv", ctypes.c_void_p),
("nm_link", ctypes.c_void_p),
]
print("pointer_size:", ctypes.sizeof(ctypes.c_void_p))
for cls in (NapiModule, NodeModule):
print(cls.__name__, "size:", ctypes.sizeof(cls))
for name, _ in cls._fields_:
print(f" {name}: {getattr(cls, name).offset}")
print("node_module fields read as napi_module:")
for name, _ in NapiModule._fields_:
print(f" napi.{name} <- node.{name if name in ('nm_version','nm_flags','nm_priv') else 'offset ' + str(getattr(NapiModule, name).offset)}")
PYRepository: NativeScript/ios
Length of output: 3467
🏁 Script executed:
#!/bin/bash
set -eu
cat -n NativeScript/runtime/ModuleBinding.hpp | sed -n '80,115p'
rg -n -C 4 --hidden -S 'node_module_register\s*\(' NativeScript TestFixtures docs \
-g '!build' -g '!dist'
python3 - <<'PY'
import ctypes
class NapiModule(ctypes.Structure):
_fields_ = [
("nm_version", ctypes.c_int),
("nm_flags", ctypes.c_uint),
("nm_filename", ctypes.c_void_p),
("nm_register_func", ctypes.c_void_p),
("nm_modname", ctypes.c_void_p),
("nm_priv", ctypes.c_void_p),
("reserved", ctypes.c_void_p * 4),
]
class NodeModule(ctypes.Structure):
_fields_ = [
("nm_version", ctypes.c_int),
("nm_flags", ctypes.c_uint),
("nm_dso_handle", ctypes.c_void_p),
("nm_filename", ctypes.c_void_p),
("nm_register_func", ctypes.c_void_p),
("nm_context_register_func", ctypes.c_void_p),
("nm_modname", ctypes.c_void_p),
("nm_priv", ctypes.c_void_p),
("nm_link", ctypes.c_void_p),
]
print("pointer_size:", ctypes.sizeof(ctypes.c_void_p))
for cls in (NapiModule, NodeModule):
print(cls.__name__, "size:", ctypes.sizeof(cls))
for name, _ in cls._fields_:
print(f" {name}: {getattr(cls, name).offset}")
PYRepository: NativeScript/ios
Length of output: 3053
Handle node_module and napi_module with separate adapters.
node_module_register receives ns_module/Node node_module objects, but this code treats them as napi_module. Their field layouts and callback signatures differ. The existing NODE_BINDING_CONTEXT_AWARE_CPP path therefore fails to register internal modules or reads invalid fields and can crash. Implement a separate node_module registration path with explicit callback handling. Do not cast the payload.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/napi/NodeApiEmbed.mm` around lines 71 - 77, Update
node_module_register to handle the incoming Node node_module with a dedicated
adapter rather than casting it to napi_module. Add explicit handling for the
node_module field layout and callback signatures, while keeping napi_module
registration separate through napi_module_register; ensure
NODE_BINDING_CONTEXT_AWARE_CPP internal modules register correctly without
interpreting the payload as another type.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
NativeScript/napi/NodeApiEmbed.mm (1)
835-865: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep claimed async cleanup handles alive until invocation completes.
napi_remove_async_cleanup_hookcan delete a handle after teardown removes it frombyEnvbut beforeNapiRunEnvCleanupHooksinvokes it. Mark the handle as running or retain it underregistry.mutex, then defer deletion until invocation completes. Removal must suppress only hooks that have not started.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/napi/NodeApiEmbed.mm` around lines 835 - 865, Update NapiRunEnvCleanupHooks and the async cleanup handle lifecycle so a handle claimed from registry.byEnv remains alive through entry.asyncHandle->hook invocation. Under registry.mutex, mark claimed handles as running or otherwise retain them, and make napi_remove_async_cleanup_hook defer deletion while running; removal should suppress only unstarted hooks. Complete the invocation state and perform deferred deletion after the hook returns.docs/node-api.md (3)
144-144: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a teardown-safe release path for external buffers.
NapiEnvis destroyed before V8 releases the backing store, and the current deleter skips the finalizer when the isolate is no longer alive. Add ownership or release handling that frees the allocation without a disposednapi_env, and add a worker-teardown regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/node-api.md` at line 144, Update the external-buffer ownership and backing-store deleter path used by napi_create_external_buffer and napi_create_external_arraybuffer so allocations are released safely even after NapiEnv teardown, without invoking a finalizer with a disposed napi_env. Add a worker-teardown regression test covering V8 releasing the backing store after the environment is destroyed.
127-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSeparate dropped async work from teardown callbacks.
Drop queued thread-safe-function JS calls and async-work completions when a
Workerterminates. Cleanup hooks run during teardown, and pending finalizers are drained synchronously. State that these teardown callbacks can release env-bound resources but must not enter JavaScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/node-api.md` at line 127, Update the documentation sentence describing Worker termination so queued thread-safe-function callbacks and async-work completions are identified as dropped, while cleanup hooks and pending finalizers are described separately as teardown callbacks that run during teardown or are drained synchronously. State that teardown callbacks may release environment-bound resources but must not enter JavaScript.
118-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLimit the owner-thread rule to isolate-bound calls.
Calls that access the isolate or
napi_envmust run on the environment owner thread. Documentnapi_call_threadsafe_function,napi_acquire_threadsafe_function, andnapi_release_threadsafe_functionas cross-thread producer operations. The async-workexecutecallback runs off that thread and must not access the isolate ornapi_env; thecompletecallback runs on the owner thread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/node-api.md` around lines 118 - 125, Revise the Threading section to limit the owner-thread requirement to calls accessing the isolate or napi_env. Explicitly identify napi_call_threadsafe_function, napi_acquire_threadsafe_function, and napi_release_threadsafe_function as supported cross-thread producer operations, while preserving that async-work execute runs off-thread without isolate or napi_env access and complete runs on the owner thread.
♻️ Duplicate comments (1)
NativeScript/NapiRuntime.mm (1)
17-21: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAcquire a runtime and environment lifetime instead of checking liveness.
Runtime::IsAlivereleasesisolatesMutex_before its caller dereferences the pointer. A teardown thread can unregister and destroy the runtime, isolate, or environment in that interval. The current checks therefore leave a use-after-free race.
NativeScript/NapiRuntime.mm#L17-L21: acquire a live runtime/environment reference before readingGetNapiEnv().NativeScript/napi/NodeApiEmbed.mm#L247-L252: acquire the live environment before comparing it and callingCallFinalizer.NativeScript/napi/NodeApiEmbed.mm#L520-L523: acquire the live environment before dispatching async-work completion.Add a synchronized acquisition or pin API that remains valid through the caller's use. Do not use
IsAliveas a lifetime guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/NapiRuntime.mm` around lines 17 - 21, Replace the unsafe Runtime::IsAlive checks with a synchronized runtime/environment acquisition or pin that remains valid through each use. In NativeScript/NapiRuntime.mm lines 17-21, hold the acquired reference before GetNapiEnv(); in NativeScript/napi/NodeApiEmbed.mm lines 247-252, hold it through the environment comparison and CallFinalizer; and in NativeScript/napi/NodeApiEmbed.mm lines 520-523, hold it through async-work completion dispatch. Do not use IsAlive as a lifetime guarantee.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/node-api.md`:
- Line 59: Change the “Including the headers” Markdown heading from level three
to level two so it correctly follows the document title and preserves the
heading hierarchy.
- Line 69: Update the configuration code fence in the documentation to include a
language identifier, using text or xcconfig, so the snippet renders consistently
and satisfies MD040.
---
Outside diff comments:
In `@docs/node-api.md`:
- Line 144: Update the external-buffer ownership and backing-store deleter path
used by napi_create_external_buffer and napi_create_external_arraybuffer so
allocations are released safely even after NapiEnv teardown, without invoking a
finalizer with a disposed napi_env. Add a worker-teardown regression test
covering V8 releasing the backing store after the environment is destroyed.
- Line 127: Update the documentation sentence describing Worker termination so
queued thread-safe-function callbacks and async-work completions are identified
as dropped, while cleanup hooks and pending finalizers are described separately
as teardown callbacks that run during teardown or are drained synchronously.
State that teardown callbacks may release environment-bound resources but must
not enter JavaScript.
- Around line 118-125: Revise the Threading section to limit the owner-thread
requirement to calls accessing the isolate or napi_env. Explicitly identify
napi_call_threadsafe_function, napi_acquire_threadsafe_function, and
napi_release_threadsafe_function as supported cross-thread producer operations,
while preserving that async-work execute runs off-thread without isolate or
napi_env access and complete runs on the owner thread.
In `@NativeScript/napi/NodeApiEmbed.mm`:
- Around line 835-865: Update NapiRunEnvCleanupHooks and the async cleanup
handle lifecycle so a handle claimed from registry.byEnv remains alive through
entry.asyncHandle->hook invocation. Under registry.mutex, mark claimed handles
as running or otherwise retain them, and make napi_remove_async_cleanup_hook
defer deletion while running; removal should suppress only unstarted hooks.
Complete the invocation state and perform deferred deletion after the hook
returns.
---
Duplicate comments:
In `@NativeScript/NapiRuntime.mm`:
- Around line 17-21: Replace the unsafe Runtime::IsAlive checks with a
synchronized runtime/environment acquisition or pin that remains valid through
each use. In NativeScript/NapiRuntime.mm lines 17-21, hold the acquired
reference before GetNapiEnv(); in NativeScript/napi/NodeApiEmbed.mm lines
247-252, hold it through the environment comparison and CallFinalizer; and in
NativeScript/napi/NodeApiEmbed.mm lines 520-523, hold it through async-work
completion dispatch. Do not use IsAlive as a lifetime guarantee.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bdf78d0d-2948-4bd7-a509-0092e65cf440
📒 Files selected for processing (10)
NativeScript/NapiRuntime.mmNativeScript/js_native_api.hNativeScript/napi/NapiEnv.mmNativeScript/napi/NapiModules.hNativeScript/napi/NapiThreadSafeFunction.mmNativeScript/napi/NodeApiEmbed.mmNativeScript/node_api.hNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmdocs/node-api.md
🚧 Files skipped from review as they are similar to previous changes (3)
- NativeScript/napi/NapiModules.h
- NativeScript/napi/NapiEnv.mm
- NativeScript/napi/NapiThreadSafeFunction.mm
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/runtime/Runtime.mm`:
- Around line 611-613: Update the thread-local runtime tracking used by
GetRuntime so each entry stores an immutable runtime generation alongside the
Runtime pointer. While isolatesMutex_ is held, require both the pointer and
generation to match before returning candidate->GetNapiEnv(); otherwise return
nullptr. Add coverage for destroying a runtime on another thread and then
registering a new runtime at the reused address.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 25447dcf-c412-4fff-b614-4d8343aa8ad0
📒 Files selected for processing (5)
NativeScript/NapiRuntime.mmNativeScript/napi/NodeApiEmbed.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmdocs/node-api.md
🚧 Files skipped from review as they are similar to previous changes (3)
- NativeScript/NapiRuntime.mm
- docs/node-api.md
- NativeScript/napi/NodeApiEmbed.mm
| Runtime* candidate = GetRuntime(isolate); | ||
| if (candidate == runtime) { | ||
| return candidate->GetNapiEnv(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Prevent stale Runtime pointer ABA matches.
If allocation reuses a destroyed Runtime address, candidate == runtime succeeds for the stale thread-local pointer. This function then returns the new runtime's napi_env instead of nullptr. NativeScriptNapiEnv can expose that environment to code on the old runtime thread.
Store an immutable runtime generation with the thread-local runtime entry. Compare both the pointer and generation while isolatesMutex_ is held. Add coverage for destruction on another thread followed by a new runtime registration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/Runtime.mm` around lines 611 - 613, Update the
thread-local runtime tracking used by GetRuntime so each entry stores an
immutable runtime generation alongside the Runtime pointer. While isolatesMutex_
is held, require both the pointer and generation to match before returning
candidate->GetNapiEnv(); otherwise return nullptr. Add coverage for destroying a
runtime on another thread and then registering a new runtime at the reused
address.
Six files copied byte-identical from nodejs/node tag v26.7.0 (b4f23d3619c98bed09af93a21192f6080197a8c6): the engine-independent Node-API headers, the V8 implementation, and its impl header. node_api.cc is intentionally not vendored; its role is filled by the runtime-side embed layer added separately. vendor/README.md records provenance, build accommodations, and the re-sync procedure; NOTICE carries the Node.js license.
One napi_env per runtime (main and each Worker), created at the end of Runtime::Init and destroyed under the teardown Locker before DisposeAllRegistered, since its reference lists hold v8::Globals. The shim supplies the idioms js_native_api_v8.cc expects from Node's internal headers, keeping the vendored files byte-identical. NodeApiEmbed covers the node_api.h surface Node implements in node_api.cc: constructor-based module registration (with a node_module_register alias for napi-ios addon compatibility), version queries, fatal errors, and buffers over Uint8Array; async and threadsafe-function entry points are stubs returning napi_generic_failure until the async layer lands. Plugins reach the env through NativeScriptNapiEnv() (NapiRuntime.h, mirroring JSIRuntime), and registered addons load through require(name): bare specifiers only, consulted after the builtin fast path, with exports cached per env. Finalizers queued from V8 weak callbacks drain on the next runloop turn, matching Node's SetImmediate placement.
Exercises the napitestmodule fixture: value round-trips, property definition, error propagation with code, napi_wrap/unwrap, references, per-env exports caching, and the finalizer draining on the runloop turn after collection.
Replaces the Phase-1 stubs with the full async surface. Threadsafe functions follow Node's queue semantics (bounded queue, blocking and nonblocking call modes, acquire/release, abort -> napi_closing) with every JS-side step posted to the owning runtime's runloop — producer threads never take the isolate's Locker, which is the same invariant the Worker class-initialization deadlock taught us. napi_async_work executes on a GCD global queue and completes on the owning runloop, with napi_cancelled reported when cancel wins the race. Env teardown now enters the isolate (the destructor holds the Locker but no scopes), then runs cleanup hooks (one LIFO list, drained against the live registry so hooks may add and remove hooks), aborts surviving threadsafe functions — releasing blocked producers and handing undelivered items to the callback with a null env — and only then finalizes references, since each TSFN holds a ref to its JS callback. Known divergences, documented inline: ref/unref of a TSFN gates nothing (the app runloop never exits because of NAPI), a JS-thread call into its own full blocking queue returns napi_would_deadlock rather than blocking, and napi_delete_async_work refuses while the work is in flight.
Ports a high-value subset of Node's test/js-native-api assertions (strings with exact truncation and NUL semantics, number conversions including the int32/int64 sentinel tables, symbols, typed arrays and dataviews with bounds and detach, promises, errors with codes, references across the weak/strong boundary, exceptions, coercions, property definition) into a second fixture module and 70+ specs. Shared fixture helpers move to NapiTestSupport.h, which also fixes error reporting: the pending-exception probe clears the last error code, so the real message has to be captured before it. One ported expectation was wrong for this runtime and is now asserted as the actual behavior and documented: the global Promise is replaced by a Proxy (promise-proxy.js), so napi_is_promise reports false for promises constructed through it, while napi_create_promise and async-function promises report true. docs/node-api.md is the plugin-author guide: quickstart addon, registration and require() loading, threading contract, finalizer timing, version story, and the divergences-from-Node table.
The cross-thread resolution marshaling lives entirely in the wrapped executor; the per-instance Proxy only rebound then/catch/finally to the underlying promise, so removing it changes no scheduling behavior while making constructed promises real V8 promises again. Engine-level checks now hold for them — napi_is_promise reports true (divergence entry removed from docs/node-api.md), and the promise user code holds is the same object the unhandledrejection/rejectionhandled events carry, which the late-handler test now asserts directly.
…nded async pool Threadsafe-function teardown now hands undelivered items back (null env) before the finalize callback runs: finalize is where addons free the context those calls receive, so the old order handed a freed pointer to callJs. Matches Node's ordering. tearingDown_ is set at the top of env teardown, so can_call_into_js() is false while cleanup hooks and finalizers run — native work proceeds, JS entry is refused, per Node's contract. Context scopes added around the TSFN abort path and napi_fatal_exception. Async work runs on a shared NSOperationQueue capped at 4 concurrent operations: addons write execute callbacks assuming libuv's bounded pool, and the unbounded global GCD queue would let N blocking callbacks spawn N threads. The node_module_register alias is removed rather than fixed: Node's symbol of that name takes a node_module* with a different layout and napi-ios's takes (name, init), so a single-pointer cast misreads both. napi_module_register is the one registration entry point. NativeScriptNapiEnv() no longer trusts the thread-local runtime pointer blindly — a Runtime destroyed on a foreign thread leaves it stale — and instead confirms liveness via a registry scan that never dereferences the candidate (new Runtime::IsAlive overload). Also adds <NativeScript/node_api.h> / <NativeScript/js_native_api.h> alias headers and documents the portable include story: bare #include <node_api.h> with Headers/napi/vendor on the search path, source-compatible with Node and napi-ios addons.
Self-created NSOperationQueues default to Background QoS, which is CPU/IO-throttled — pin the async-work pool to Default, matching the GCD global queue it replaced. NativeScriptNapiEnv() resolved the env after releasing the registry lock, leaving a narrow use-after-free window against a Runtime destroyed on a foreign thread; Runtime::GetNapiEnvIfAlive now resolves it while the lock is held. The returned env's validity remains governed by the Node-API threading contract. The threading doc still described the execute callback as running on a GCD global queue; it now states the bounded-pool contract, including the Node-matching deadlock behavior for >4 interdependent blocking executes.
Finalizer drains, threadsafe-function dispatches, and async-work completions post to the event loop's internal lane instead of raw CFRunLoopPerformBlock. Entries run under the loop's Locker/scopes and end with a microtask checkpoint, so a promise resolved from a complete or TSFN callback settles without waiting for unrelated JS — previously a native-only resolution could stall under kAuto. Shutdown drops queued entries before ~Runtime destroys the env, which retires the per-entry liveness re-checks and the isolate ceremony the posted blocks carried; the TSFN keeps its retained CFRunLoopRef solely for the napi_would_deadlock same-thread check. The internal lane runs one entry per pass and shares it with V8's own tasks, so finalizer-vs-timer ordering is no longer implied: the wrap finalizer spec polls, and the docs state the contract.
2d9b103 to
be00b4e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/node-api.md`:
- Around line 135-140: Update the __collect() example in the finalizer
documentation to avoid implying that a single setTimeout guarantees finalizer
execution. Replace the one-turn timer with polling, following the established
approach in NapiTests.js, while preserving the documented unspecified ordering
relative to timers.
- Around line 144-145: Update the external buffer and external ArrayBuffer
teardown flow for napi_create_external_buffer and
napi_create_external_arraybuffer so external_data is always released when the
backing store deleter runs, including after napi_env disposal. Separate
unconditional native-data cleanup from the env-dependent finalizer callback,
skipping only the callback when the environment is unavailable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe669a5f-ddab-4f3c-acd3-8cd462269ab8
📒 Files selected for processing (11)
NativeScript/napi/NapiEnv.hNativeScript/napi/NapiEnv.mmNativeScript/napi/NapiThreadSafeFunction.mmNativeScript/napi/NodeApiEmbed.mmNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmTestRunner/app/tests/NapiTests.jsTestRunner/app/tests/index.jsdocs/node-api.mdv8ios.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (8)
- TestRunner/app/tests/index.js
- NativeScript/runtime/Runtime.mm
- NativeScript/runtime/ModuleInternal.mm
- NativeScript/napi/NapiEnv.mm
- v8ios.xcodeproj/project.pbxproj
- NativeScript/napi/NodeApiEmbed.mm
- NativeScript/runtime/Runtime.h
- NativeScript/napi/NapiThreadSafeFunction.mm
External-buffer finalizers must run exactly once, on the env's thread, while the env is alive — but V8's backing-store deleter fires on arbitrary threads, including during isolate disposal after the env died. The finalizer is now registered with the env: the deleter only posts it to the event loop, the teardown sweep runs whatever is still unclaimed while the callback can still be made, and a claim flag flipped exclusively on the env thread arbitrates. This retires the documented skip-and-leak behavior, and the previous deleter's worse defect of invoking the callback on whatever thread V8 released the store from. Runtime::GetNapiEnvIfAlive additionally requires the matched runtime to be homed on the calling thread's runloop, closing the allocator-reuse hole: a stale thread-local can only exist on the dead runtime's home thread, and a recycled same-address Runtime homed on this thread would have overwritten that thread-local, so an address match with a foreign home loop is provably a recycled pointer. Docs: portable-include section heading level and fence language, the finalizer example polls instead of assuming one timer turn, and the teardown paragraphs describe the Shutdown-drop mechanism rather than the per-entry re-checks the event-loop migration removed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/napi/NodeApiEmbed.mm`:
- Around line 263-268: Preserve the basic external-buffer finalizer type across
the custom finalizer registry: in NativeScript/napi/NodeApiEmbed.mm:263-268,
store the callback received by napi_create_external_buffer as
node_api_basic_finalize; update NapiEnv.h:33-38 to represent this callback type;
and update NapiEnv.mm:92-99 to invoke it through a dedicated basic-finalizer
path passing node_api_basic_env, while retaining the existing napi_finalize path
for regular finalizers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 22241a9f-011a-4145-9606-346e24946b69
📒 Files selected for processing (5)
NativeScript/napi/NapiEnv.hNativeScript/napi/NapiEnv.mmNativeScript/napi/NodeApiEmbed.mmNativeScript/runtime/Runtime.mmdocs/node-api.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/node-api.md
| auto finalizer = std::make_shared<tns::NapiExternalFinalizer>(); | ||
| finalizer->cb = reinterpret_cast<napi_finalize>(finalize_cb); | ||
| finalizer->data = data; | ||
| finalizer->hint = finalize_hint; | ||
| tnsEnv->RegisterExternalFinalizer(finalizer); | ||
| deleter_data = new DeleterState{tnsEnv, std::move(finalizer), tnsEnv->GetEventLoop()}; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'node_api_basic_env|node_api_basic_finalize|napi_finalize' \
NativeScript/napi/vendor/js_native_api_types.h
rg -n -C 3 \
'NapiExternalFinalizer|RunExternalFinalizer|reinterpret_cast<napi_finalize>' \
NativeScript/napiRepository: NativeScript/ios
Length of output: 9643
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- type definitions and feature guards ---'
sed -n '1,215p' NativeScript/napi/vendor/js_native_api_types.h
printf '%s\n' '--- affected declarations and calls ---'
sed -n '20,45p' NativeScript/napi/NapiEnv.h
sed -n '80,105p' NativeScript/napi/NapiEnv.mm
sed -n '225,280p' NativeScript/napi/NodeApiEmbed.mm
printf '%s\n' '--- callback declarations and call sites ---'
rg -n -C 4 \
'node_api_create_external_arraybuffer|node_api_create_external_string|node_api_wrap|node_api_unwrap|basic_finalize_callback|basic_finalize_cb|CallFinalizer' \
NativeScript/napiRepository: NativeScript/ios
Length of output: 27737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("NativeScript/napi/vendor/js_native_api_types.h")
text = p.read_text()
for name in ("node_api_nogc_env", "node_api_basic_env", "napi_env", "napi_finalize", "node_api_nogc_finalize", "node_api_basic_finalize"):
print(f"{name}:")
for i, line in enumerate(text.splitlines(), 1):
if name in line:
print(f" {i}: {line}")
PY
printf '%s\n' '--- build/configuration references ---'
rg -n -C 3 \
'NAPI_EXPERIMENTAL|NODE_API_EXPERIMENTAL_(NOGC_ENV_OPT_OUT|BASIC_ENV_OPT_OUT)|js_native_api_types.h' \
NativeScript .github CMakeLists.txt Makefile 2>/dev/null || trueRepository: NativeScript/ios
Length of output: 18928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '35,62p' NativeScript/napi/NapiEnv.mm
sed -n '90,110p' NativeScript/napi/vendor/js_native_api_v8.h
sed -n '3600,3685p' NativeScript/napi/vendor/js_native_api_v8.cc
sed -n '415,445p' NativeScript/napi/vendor/js_native_api.h
printf '%s\n' '--- all external-finalizer type flow ---'
rg -n -C 5 \
'napi_create_external_buffer|NapiExternalFinalizer|CallFinalizer\(|node_api_basic_finalize' \
NativeScript/napi/NodeApiEmbed.mm NativeScript/napi/NapiEnv.h NativeScript/napi/NapiEnv.mmRepository: NativeScript/ios
Length of output: 15467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
types = Path("NativeScript/napi/vendor/js_native_api_types.h").read_text()
env = Path("NativeScript/napi/NapiEnv.h").read_text()
embed = Path("NativeScript/napi/NodeApiEmbed.mm").read_text()
assert "`#define` NAPI_EXPERIMENTAL" in env or "`#define` NAPI_EXPERIMENTAL" in embed
assert "typedef const struct napi_env__* node_api_nogc_env;" in types
assert "typedef node_api_nogc_finalize node_api_basic_finalize;" in types
assert "napi_finalize cb = nullptr;" in env
assert "reinterpret_cast<napi_finalize>(finalize_cb)" in embed
print("NAPI_EXPERIMENTAL is enabled by the affected translation units.")
print("node_api_basic_finalize is const-qualified in the active configuration.")
print("NapiExternalFinalizer stores napi_finalize and NodeApiEmbed.mm casts to that type.")
print("The reported type flow exists exactly as described.")
PYRepository: NativeScript/ios
Length of output: 425
🌐 Web query:
Node-API node_api_basic_finalize napi_finalize reinterpret_cast const node_api_basic_env upstream implementation
💡 Result:
In Node-API, the node_api_basic_finalize function pointer type and the associated node_api_basic_env type are designed to provide a safe, compile-time enforced mechanism for handling native resource cleanup during garbage collection [1][2][3]. Key technical aspects include: 1. node_api_basic_env: This is defined as a const-qualified variant of the environment pointer (typedef const struct napi_env* node_api_basic_env) [2]. This const qualification is the mechanism used to enforce compile-time safety [2]. Because Node-API functions that interact with the JavaScript engine heap require a non-const napi_env, passing a node_api_basic_env to them results in a compiler error or warning [1][2]. Conversely, Node-API functions that are safe to call during garbage collection (i.e., those that do not access the JavaScript engine state) are designed to accept node_api_basic_env as their first parameter [1][4]. 2. node_api_basic_finalize: This is the callback type used for finalizers that run synchronously during garbage collection [1][5]. Because these finalizers execute while the JavaScript engine is in a state where it cannot execute JavaScript code, they are restricted to calling only those Node-APIs that accept node_api_basic_env [1][6]. 3. reinterpret_cast and Implementation: In the upstream Node.js implementation (e.g., src/node_api.cc), when a developer provides a function that matches the node_api_basic_finalize signature, it is sometimes reinterpret_cast to a napi_finalize when passed to underlying structures that expect the more generic napi_finalize type [7]. This allows the API to maintain a uniform internal representation while using the C type system's const qualifier to prevent illegal API usage during the restricted finalization phase [2]. If a developer requires access to the JavaScript engine state for cleanup, they should not use a basic finalizer or must use node_api_post_finalizer to schedule the work to run after the garbage collection cycle completes [1][6]. This design ensures that native add-ons remain stable by preventing dangerous operations during GC [1][3].
Citations:
- 1: https://nodejs.org/api/n-api.html
- 2: https://github.com/nodejs/node/blob/main/src/js_native_api_types.h
- 3: https://github.com/nodejs/node/blob/master/doc/api/n-api.md
- 4: https://github.com/nodejs/node/blob/main/src/node_api.h
- 5: https://beta.docs.nodejs.org/n-api.html
- 6: https://github.com/nodejs/node-addon-api/blob/main/doc/finalization.md
- 7: https://github.com/nodejs/node/blob/main/src/node_api.cc
Preserve the external-buffer finalizer type.
napi_create_external_buffer receives node_api_basic_finalize, but the custom registry stores it as napi_finalize and invokes it through the incompatible function-pointer type. Store node_api_basic_finalize and call it through a dedicated basic-finalizer path that passes node_api_basic_env.
📍 Affects 3 files
NativeScript/napi/NodeApiEmbed.mm#L263-L268(this comment)NativeScript/napi/NapiEnv.h#L33-L38NativeScript/napi/NapiEnv.mm#L92-L99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/napi/NodeApiEmbed.mm` around lines 263 - 268, Preserve the basic
external-buffer finalizer type across the custom finalizer registry: in
NativeScript/napi/NodeApiEmbed.mm:263-268, store the callback received by
napi_create_external_buffer as node_api_basic_finalize; update NapiEnv.h:33-38
to represent this callback type; and update NapiEnv.mm:92-99 to invoke it
through a dedicated basic-finalizer path passing node_api_basic_env, while
retaining the existing napi_finalize path for regular finalizers.
2 participants
What
Exposes a standard Node-API (
napi_*) surface to plugin authors, alongside the existing JSI facade — plugins can be written against the Node-API C ABI instead of raw V8.js_native_api_v8.cc+ public headers) from nodejs/node v26.7.0 (b4f23d36), byte-identical; provenance, license (MIT/OpenJS NOTICE), and a 3-line re-sync procedure live inNativeScript/napi/vendor/README.md. A small shim (NativeScript/napi/shim/) supplies the handful of idioms upstream expects from Node internals.napi_envper runtime (main + each Worker), created at the end ofRuntime::Init, destroyed under the teardown Locker beforeObjectManager::DisposeAllRegistered(env ref lists holdv8::Globals). NAPI refs/finalizers stay on stockv8::Global+SetWeak, byte-compatible with upstream — deliberately independent of the planned cppgc migration of runtime wrappers.napi_closing) where every JS-side step is posted to the owning runtime's runloop — producer threads never take the isolate'sLocker;napi_async_workexecutes on a GCD global queue and completes on the owning runloop; async contexts,napi_make_callback, callback scopes, and env/async cleanup hooks (LIFO, hooks may add/remove hooks while running).#include <node_api.h>withHeaders/napi/vendoron the search path (same source compiles against Node, napi-ios, and this runtime; required fornode-addon-api); zero-config alternatives are<NativeScript/node_api.h>(alias) and#include <NativeScript/NapiRuntime.h>→NativeScriptNapiEnv()(mirrorsJSIRuntime). Headers ship in the framework automatically.napi_module_register(deliberately nonode_module_registeralias: Node’s symbol takes a differently-laid-outnode_module*, napi-ios’s takes(name, init)— a cast would misread both) and load from JS withrequire("name")— bare specifiers only, consulted after the builtin fast path, exports cached per env (Workers get their own instance).napi_is_promiseand other engine-level checks hold for promises made via the globalPromise, and rejection events carry the same object user code holds.napi_resolve_deferredin acompletecallback) settles without waiting for unrelated JS.EventLoop::Shutdown()dropping queued entries before env teardown retires the per-entry liveness guards the posted blocks previously carried.napi_get_version); envs use module API version 8 (Node's default), with the implications documented.docs/node-api.md— plugin-author quickstart (working addon + JS caller, registration, loading, threading contract, finalizer timing, version story) plus the full divergence reference.Divergences from Node
Everything in
js_native_api.his upstream, compiled unmodified. The differences are confined to thenode_api.hsurface, where Node's implementation depends on libuv,node::Buffer, or its module loader — All are written up indocs/node-api.md.napi_get_uv_event_loopandnode_api_get_module_file_namereturnnapi_generic_failure. There is nouv_loop_t— the runtime drives aCFRunLoop— and addons are linked into the app binary rather than loaded from a file, so nothing identifies the calling module.Uint8Arrays. There is nonode::Buffer, sonapi_is_bufferis exactly "is this aUint8Array".napi_create_external_bufferis still zero-copy; its finalizer runs from V8's backing-store deleter, and is skipped (leaking the data) if the isolate is already disposing.ref/unrefare no-ops. The runloop belongs to the app or the worker; it does not exit because an addon released its last reference. The flag is tracked so calls pair up, and gates nothing.napi_call_threadsafe_functionreturnsnapi_would_deadlockinstead of blocking, when a blocking call is made from the env's own thread with a full queue — the thread that would drain the queue is the caller. Node blocks regardless and never returns this status; wedging the runloop is worse than a status an addon may not expect.napi_delete_async_workrefuses queued or executing work (napi_generic_failure) rather than deleting it and leaving the queue holding a dangling pointer.napi_remove_async_cleanup_hookis safe.napi_fatal_exceptionreports and continues through the runtime's error handlers;napi_fatal_errorstill aborts, as upstream.NAPI_MODULEmacro is not the entry point. It only emits the symbols adlopenloader would scan for, and there is no such loader here; addons register from a constructor callingnapi_module_register.node_api.his the whole native surface — noprocess, nofs, no libuv handles, nonode.h/v8.h/uv.haccess.Testing
NapiTests.jsandNapiCoverageTests.js).test/js-native-apisuites: string encodings/truncation/NUL semantics, number conversion sentinel tables, symbols, typed arrays + dataviews (bounds, alignment, detach), promises, errors with codes, references (weak/strong transitions, GC-driven), exceptions, coercions, property definition — plus TSFN ordering/backpressure/reentry/abort, async work + cancel, cleanup hooks, and Worker isolation.nmon the built framework (Debug simulator + Release device): 145napi_*symbols +NativeScriptNapiEnvexported.Follow-ups (not in this PR)
require("<path>.node")dylib loading (dlopen+dlsym("napi_register_module_v1"), as napi-ios does).-fmodulesconsumers:NapiRuntime.hincludes non-modularnapi/vendor/*.hheaders; likely moot since the framework setsDEFINES_MODULE = NO, but worth confirming with a real plugin build.napi_create_external_buffer's backing-store deleter holds a rawnapi_env; guarded against teardown races (leaks instead of dangling), a full fix would mirror Node'sv8impl::Referenceownership.Summary by CodeRabbit