-
Notifications
You must be signed in to change notification settings - Fork 80
Improve object safety #500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Vladimir Morozov (vmoroz)
wants to merge
38
commits into
microsoft:main
Choose a base branch
from
vmoroz:PR/improve-object-safety
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
d554115
Improve object safety
vmoroz c1e47c0
Address Copilot review: scope/context correctness + per-env host tear…
vmoroz 3a0345b
Fix formatting
vmoroz e1ed27e
Free runtime context root at teardown
vmoroz 2454f05
Reuse one JSRuntimeContext per env in embedding adapters
vmoroz d217d0d
Dispose IDisposable module instance at environment teardown
vmoroz 85e2611
Document the runtime model and add agent instructions
vmoroz 047ed0b
Harden SetDisposableAnnotation against post-dispose and replacement
vmoroz a4e6e42
Give each loaded module its own module holder
vmoroz 3b84450
Return a context from FromEnv only when it matches the env
vmoroz 65ec52e
Address pre-PR code review: shared-context module disposal
vmoroz ba16dce
Free the env instance-data block at teardown; fix module-disposable d…
vmoroz e5114f4
Pin JSRuntimeContext to its creation thread
vmoroz 5dc04b6
Harden runtime-context construction and scope entry
vmoroz 7ab932b
Run full host disposal from the JS dispose() hook
vmoroz 372ce30
Guard lazy sync-context creation; report a stackless init error
vmoroz 1ca52e2
Dispose exports reference on host dispose; clarify finalizer doc
vmoroz 31e752d
Trim NativeHost.Dispose comments
vmoroz 43db3ae
Contain managed-host scope creation in the failure path; harden stres…
vmoroz 54e0f60
Guard native-host init at the boundary; safe slot-clear order; net472…
vmoroz a6edc79
Document native-host context finalizer ownership in the init catch
vmoroz ca4d8fb
Guard managed-host and generated-AOT entry points at the boundary
vmoroz e3be211
Reject disposing a value scope off-thread or out of order
vmoroz ff248c1
Guard the embedding boundary; close the native scope on failed setup
vmoroz 3de5b98
Clear the context's instance-data slot only if it still owns it
vmoroz cf1b737
Guard teardown cleanup in finally so a failure can't strand state
vmoroz 89fc05b
Run all context teardown phases and guard host cleanup in finally
vmoroz 8c1205b
Associate a napi_env with a runtime context exactly once
vmoroz 3632a47
Fix RuntimeScope creation
vmoroz 61fd815
Make the callback runtime-scope factory non-throwing and env-matched
vmoroz 980ff4d
Contain callback-scope disposal at native boundaries; fail reference …
vmoroz 7344eba
Contain finalizer and teardown exceptions; dispose a context on its o…
vmoroz fad0d60
Clarify why a disposed context never deletes its napi_refs
vmoroz 2541ad7
Defer runtime context disposal until its value scopes close
vmoroz f26d083
Track the target context when deferring runtime-context disposal
vmoroz 2d76ea9
Contain and order disposal at more teardown boundaries
vmoroz 547f8a3
Search the ancestor chain when deferring runtime-context disposal
vmoroz bedf377
Enforce one runtime context per thread scope stack
vmoroz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # GitHub Copilot instructions | ||
|
|
||
| See [AGENTS.md](../AGENTS.md) for how to work in this repository, including the runtime model that | ||
| underlies environments, teardown, threading, and object lifetime, plus the build/format/test steps. | ||
|
|
||
| Key reminder: run `dotnet format --severity info --verbosity detailed` after code changes (PR builds | ||
| fail on formatting violations), and run `dotnet pack` before `dotnet test`. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| # Repository guide for AI agents | ||
|
|
||
| This file orients automated coding agents (and new contributors) working in this repository. It is | ||
| intentionally short; it points at the authoritative docs rather than duplicating them. | ||
|
|
||
| `node-api-dotnet` provides high-performance, in-process interop between .NET and JavaScript, built on | ||
| [Node-API](https://nodejs.org/api/n-api.html). It ships a runtime library, a native + managed host, | ||
| a C# source generator, and a TypeScript type-definitions generator. | ||
|
|
||
| ## Read this first: the runtime model | ||
|
|
||
| Most recurring misunderstandings in this codebase come from the JavaScript environment / .NET | ||
| context lifetime model. **Read [docs/concepts/runtime-model.md](docs/concepts/runtime-model.md) | ||
| before reasoning about environments, teardown, threading, or object lifetime.** The facts that are | ||
| most often gotten wrong: | ||
|
|
||
| - **Node.js creates one `napi_env` per loaded native module.** A Native AOT module is `1 env : 1` | ||
| `JSRuntimeContext`. A managed module runs a native host and a managed host that **share one env** | ||
| (two instance-data slots) — that is the *only* case where two contexts share an env. Two | ||
| independently compiled AOT addons are two separate modules and therefore get **two different | ||
| envs**; they never share one, so their per-environment state cannot collide. | ||
| - **`node::Environment` is not `napi_env`.** There is one `node::Environment` per V8 isolate / worker | ||
| thread, and **zero or more `napi_env` per `node::Environment`** (one per native module). An | ||
| environment cleanup hook is associated with the `node::Environment`; the **instance-data finalizer | ||
| is per `napi_env`.** Per-context teardown keys off the instance-data finalizer, not the cleanup | ||
| hook. | ||
| - **Finalizers run during environment teardown, where calling into JavaScript is forbidden.** Resolve | ||
| the context with `JSRuntimeContext.FromEnv(env)`, never by dereferencing a finalize hint that may be | ||
| freed, and assume no ordering between wrapped-object finalizers and the instance-data finalizer. | ||
| - **`napi_value` / `JSValue` are valid only within their `JSValueScope` and only on the JS thread.** | ||
| To keep a value beyond its scope, hold a `JSReference` (`napi_ref`). There are three scope types — | ||
| runtime-context, handle, and escapable — and a module boundary starts a fresh module holder so each | ||
| loaded module resolves its own module instance. | ||
|
|
||
| ## Build, format, and test | ||
|
|
||
| Full details are in [README-DEV.md](README-DEV.md). The essentials: | ||
|
|
||
| ```bash | ||
| dotnet build | ||
| dotnet format --severity info --verbosity detailed # PR builds FAIL if formatting is non-compliant | ||
| dotnet pack # required before tests (the generator is consumed as a local package) | ||
| dotnet test | ||
| ``` | ||
|
|
||
| - **Run `dotnet format` after code changes and before tests** — formatting is a CI gate. | ||
| - **`dotnet pack` is required before `dotnet test`**, and again after any change to the source | ||
| generator, because tests consume the generator through the locally built NuGet package. Use | ||
| `-c Release` for release-configuration testing. | ||
| - Most test cases run twice: once in hosted CLR mode and once in Native AOT mode. Test cases are | ||
| derived from the `.js` files under `test/TestCases`. | ||
|
|
||
| ## Conventions | ||
|
|
||
| - Follow the existing code style enforced by `.editorconfig` (American English in code, comments, and | ||
| docs). | ||
| - See [docs/contributing.md](docs/contributing.md) for contribution guidelines, and | ||
| [docs/NodeApi-Layers.md](docs/NodeApi-Layers.md) for how the assemblies and namespaces are layered. | ||
| - Keep code comments minimal: add one only to explain a non-obvious "why" that the code cannot show. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Guidance for Claude | ||
|
|
||
| See [AGENTS.md](AGENTS.md) for how to work in this repository, including the runtime model that | ||
| underlies environments, teardown, threading, and object lifetime, plus the build/format/test steps. | ||
|
|
||
| Key reminder: run `dotnet format --severity info --verbosity detailed` after code changes (PR builds | ||
| fail on formatting violations), and run `dotnet pack` before `dotnet test`. |
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| # Runtime model: environments, lifetimes, and threads | ||
|
|
||
| This page describes the foundational runtime model that the rest of the library is built on: | ||
| how JavaScript environments map to .NET runtime contexts, how those contexts are torn down, and | ||
| the rules for safely holding JavaScript values. The per-feature pages | ||
| ([JS value scopes](../features/js-value-scopes), [JS references](../features/js-references), | ||
| [JS threading & async](../features/js-threading-async), | ||
| [Node worker threads](../features/node-workers)) assume the model described here. | ||
|
|
||
| If you are extending this library or reviewing a change to it, read this first — several parts of | ||
| the design only make sense once the environment/module relationship is clear. | ||
|
|
||
| ## Environments and module instances | ||
|
|
||
| **Node.js creates a unique `napi_env` for each native module it loads.** When a module is | ||
| registered, `napi_module_register_by_symbol` (in Node's `src/node_api.cc`) calls `NodeApiEnv::New`, | ||
| which mints a fresh `napi_env` for that specific module. So the mapping is per-module, not | ||
| per-process and not per-isolate. | ||
|
|
||
| That gives three deployment shapes: | ||
|
|
||
| | Shape | `napi_env` : `JSRuntimeContext` | Notes | | ||
| | --- | --- | --- | | ||
| | **Native AOT module** | 1 : 1 | The `.node` file *is* the module, so Node makes one env and the module owns one context. | | ||
| | **Managed module** (`.node` native host + managed host) | 1 : 2 | The native host and the managed host run in **separate .NET runtimes** but share the **same** env. Each registers its own context. | | ||
| | **Embedding** (a .NET app hosting `libnode`) | 1 : 1 per env | The .NET app creates and owns each environment's context. | | ||
|
|
||
| The managed-module case is the only one where two contexts share a single `napi_env`. The native | ||
| host (`NativeHost`, AOT-compiled into the `.node`) initializes first and hands the same env to the | ||
| managed host (`ManagedHost`, loaded into the default .NET runtime); both create a `JSRuntimeContext` | ||
| for that one env. This is deliberate and bounded — there are never more than these two. | ||
|
|
||
| **A consequence worth stating explicitly:** two independently compiled AOT addons are two separate | ||
| native modules, so Node gives them **two different `napi_env` instances**. They never share one | ||
| environment, and their per-environment state never collides. The same is true for an AOT addon | ||
| loaded alongside the managed host: different modules, different envs. | ||
|
|
||
| ## `node::Environment` vs `napi_env` vs isolate/worker | ||
|
|
||
| These three are easy to conflate, but they nest at different granularities: | ||
|
|
||
| - **`node::Environment`** — one per V8 isolate, i.e. one per Node.js **worker thread** (the main | ||
| thread is a worker too). It owns the event loop and the environment-cleanup hook list. | ||
| - **`napi_env`** — **zero or more per `node::Environment`**, one for each native module loaded into | ||
| that worker. Node-API objects, references, and instance data all belong to a specific `napi_env`. | ||
| - **isolate/worker thread** — the JS execution thread. All JS values and value scopes have affinity | ||
| to it. | ||
|
|
||
| Two teardown callbacks live at these different levels, and the difference matters: | ||
|
|
||
| - An **environment cleanup hook** (`napi_add_env_cleanup_hook`, backed by | ||
| `node::AddEnvironmentCleanupHook`) is associated with the **`node::Environment`**. It fires once | ||
| when the whole worker shuts down. | ||
| - The **instance-data finalizer** (registered with `napi_set_instance_data`) is associated with a | ||
| **single `napi_env`**. It fires when that module's environment is torn down. | ||
|
|
||
| Because a `JSRuntimeContext` is scoped to one `napi_env`, this library keys per-context teardown off | ||
| the **instance-data finalizer**, not the environment cleanup hook. Using the cleanup hook would be | ||
| both too coarse (one worker may host several envs) and wrongly timed for per-module lifetime. | ||
|
|
||
| ## Instance-data ownership (`JSRuntimeContext`) | ||
|
|
||
| Each context roots itself with a `GCHandle` stored in its env's instance-data block. Because the | ||
| managed-module case puts two contexts (in two separate .NET runtimes/GC heaps) on one env, the block | ||
| has **two slots**: | ||
|
|
||
| - **slot 0** — the module context: managed host, AOT module, or embedding. | ||
| - **slot 1** — the native host context. | ||
|
|
||
| There are exactly two slots because the native-host + managed-host pair is the only case where two | ||
| contexts share an env. A runtime **reads and writes only its own slot**, so it never dereferences a | ||
| `GCHandle` that belongs to the other runtime's GC heap (which would be undefined behavior). | ||
|
|
||
| `JSRuntimeContext.FromEnv(napi_env)` resolves the calling runtime's context from its slot. This is | ||
| how callback dispatch and finalizers recover the context when no scope is yet current on the thread. | ||
|
|
||
| At environment teardown the instance-data finalizer disposes the owning context, which **clears its | ||
| slot and frees the rooting `GCHandle`**. Disposing a host context cascades synchronously to the | ||
| other slot's context, so once every context on the env is gone the finalizer **frees the block**. | ||
| Freeing it there is no less safe than keeping it: a finalizer that called `FromEnv` after the | ||
| instance-data finalizer would already be reading Node's own freed finalizer record (Node does not | ||
| null its instance-data pointer), so retaining the block never protected that case. The block is not | ||
| nulled out via `napi_set_instance_data` — that would delete the finalizer record Node is running and | ||
| then double-free it. | ||
|
|
||
| ## JavaScript value scopes | ||
|
|
||
| Every `JSValue` belongs to a [`JSValueScope`](../features/js-value-scopes). There are three scope | ||
| types, each created by a static factory: | ||
|
|
||
| - **Runtime-context scope** — `JSValueScope.CreateRuntimeScope(env, context)`. References a | ||
| `JSRuntimeContext` and marks a call/context boundary. It opens no napi handle scope. This is the | ||
| scope opened at a module entry point or a callback into .NET. | ||
| - **Handle scope** — `JSValueScope.CreateHandleScope()`. A nested napi handle scope; JS values | ||
| created within it are released when it is disposed, unless held by a `JSReference`. Use it to | ||
| bound the lifetime of values created in a loop. | ||
| - **Escapable scope** — `JSValueScope.CreateEscapableScope()`. Like a handle scope, but one value | ||
| may be promoted to the parent scope with `Escape`, so it survives the inner scope's disposal. | ||
|
|
||
| A **module boundary** is a runtime-context scope that starts a *fresh module holder* while reusing | ||
| the surrounding context, so each loaded module resolves its own module instance via | ||
| `JSValueScope.Current.Module`. This matters when a single managed host loads several generated | ||
| modules: without a fresh holder per module, the most recently loaded module's instance would be the | ||
| one every module's callbacks resolve. | ||
|
|
||
| Scopes nest on a **thread-static stack, and every scope on that stack shares one runtime context.** | ||
| Each loaded module has its own stack — a native module is compiled with its own copy of this library, | ||
| so even the AOT native host and the CoreCLR managed host that share an env are separate modules whose | ||
| stacks never mix — and there is one context per environment per module. So a thread running one | ||
| module's code always sees exactly one context: a nested runtime-context scope inherits its parent's | ||
| context, and creating one for a *different* context throws. Callback dispatch depends on this, | ||
| inheriting the current scope's context (or `FromEnv` when no scope is open) rather than reconciling | ||
| several. | ||
|
|
||
| ## Lifetime of `napi_value` and `napi_ref` (`JSValue` / `JSReference`) | ||
|
|
||
| - A `napi_value` (wrapped by [`JSValue`](../features/js-value-scopes)) is valid **only within its | ||
| scope**. Using it after the scope closes throws `JSValueScopeClosedException`. Values passed to a | ||
| .NET callback belong to that call's scope and become invalid when it returns. | ||
| - JS values and scopes have **thread affinity**: they may be accessed only from the JS thread that | ||
| owns the environment. Access from another thread throws `JSInvalidThreadAccessException`. To marshal | ||
| work back to the JS thread, use the context's synchronization context (see | ||
| [JS threading & async](../features/js-threading-async)). | ||
| - To keep a value **beyond its scope**, create a [`JSReference`](../features/js-references) (a | ||
| `napi_ref`). A strong reference keeps the value alive; a weak one lets it be collected and resolves | ||
| to nothing afterward. A `JSReference` is itself owned by a context and released with it. | ||
|
|
||
| ### Finalizers and teardown — no JS once the context is disposed | ||
|
|
||
| A finalizer (for a wrapped .NET object, an external, or a reference) may run during normal GC while | ||
| the environment is still alive, or while the environment is being torn down. **Once the context is | ||
| disposed at environment teardown, calling into JavaScript is forbidden.** Finalizer code in this | ||
| library follows two rules: | ||
|
|
||
| 1. **Resolve the context from the env**, via `JSRuntimeContext.FromEnv(env)` — never by dereferencing | ||
| a finalize hint that may already be freed. If `FromEnv` returns no live context (the slot was | ||
| cleared at teardown), the finalizer only frees its own native handle and does no JS work. While the | ||
| context is still live, a finalizer action may run — for example `JSValue.CallFinalizeAction` opens a | ||
| runtime scope to invoke the user action — so this rule is what keeps teardown itself JS-free. | ||
| 2. **Never assume ordering** among the env's finalizers. Node drains wrapped-object finalizers in no | ||
| guaranteed order, so a finalizer must tolerate the context's slot already being cleared (rule 1). | ||
| The instance-data finalizer frees the block only after every context on the env is disposed. | ||
|
|
||
| ## See also | ||
|
|
||
| - [Project layers](../NodeApi-Layers) — how the assemblies and namespaces are organized. | ||
| - [JS value scopes](../features/js-value-scopes), [JS references](../features/js-references) — | ||
| the day-to-day API surface built on this model. | ||
| - [JS threading & async](../features/js-threading-async), | ||
| [Node worker threads](../features/node-workers) — the threading rules in practice. |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.