From d554115109f0c60217b72e708afc9f4043bf1434 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 11:27:59 -0700 Subject: [PATCH 01/38] Improve object safety --- Directory.Packages.props | 2 +- bench/Benchmarks.cs | 2 +- docs/features/js-value-scopes.md | 9 +- examples/hermes-engine/HermesRuntime.cs | 5 +- src/NodeApi.DotNetHost/JSMarshaller.cs | 18 +- src/NodeApi.DotNetHost/ManagedHost.cs | 89 ++++- .../ManagedHostRegistration.cs | 35 ++ src/NodeApi.Generator/ModuleGenerator.cs | 32 +- src/NodeApi/DotNetHost/NativeHost.cs | 106 ++++- src/NodeApi/Interop/JSCallbackDescriptor.cs | 17 +- src/NodeApi/Interop/JSModuleBuilderOfT.cs | 10 +- src/NodeApi/Interop/JSModuleContext.cs | 42 -- src/NodeApi/Interop/JSRuntimeContext.cs | 218 +++++++++- .../Interop/JSSynchronizationContext.cs | 36 ++ src/NodeApi/Interop/JSThreadSafeFunction.cs | 6 +- src/NodeApi/JSError.cs | 50 +-- src/NodeApi/JSPropertyDescriptor.cs | 10 +- src/NodeApi/JSReference.cs | 88 ++-- src/NodeApi/JSValue.cs | 169 +------- src/NodeApi/JSValueScope.cs | 333 ++++++---------- src/NodeApi/NodeApi.csproj | 7 + src/NodeApi/Runtime/NodeEmbedding.cs | 16 +- .../Runtime/NodeEmbeddingNodeApiScope.cs | 5 +- src/NodeApi/Runtime/TracingJSRuntime.cs | 19 +- test/GCTests.cs | 4 +- test/JSReferenceTests.cs | 80 ++-- test/JSValueScopeTests.cs | 376 ++++++++---------- test/MockJSRuntime.cs | 24 ++ test/TestBuilder.cs | 37 +- .../napi-dotnet/worker_teardown_stress.js | 49 +++ 30 files changed, 1013 insertions(+), 881 deletions(-) create mode 100644 src/NodeApi.DotNetHost/ManagedHostRegistration.cs delete mode 100644 src/NodeApi/Interop/JSModuleContext.cs create mode 100644 test/TestCases/napi-dotnet/worker_teardown_stress.js diff --git a/Directory.Packages.props b/Directory.Packages.props index 973b56fb..7c5e7614 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -11,7 +11,7 @@ - + diff --git a/bench/Benchmarks.cs b/bench/Benchmarks.cs index c3461ab5..937ff8f6 100644 --- a/bench/Benchmarks.cs +++ b/bench/Benchmarks.cs @@ -144,7 +144,7 @@ protected void Setup() _reference = new JSReference(_jsFunction); } - private static JSValueScope NewJSScope() => new(JSValueScopeType.Callback); + private static JSValueScope NewJSScope() => JSValueScope.CreateRuntimeScope(); // Benchmarks in the base class run in both CLR and AOT environments. diff --git a/docs/features/js-value-scopes.md b/docs/features/js-value-scopes.md index de332411..0a25fc59 100644 --- a/docs/features/js-value-scopes.md +++ b/docs/features/js-value-scopes.md @@ -7,9 +7,8 @@ A value is only valid within its scope; if the scope is closed (disposed), then access or use the value will throw [`JSValueScopeClosedException`](../reference/dotnet/Microsoft.JavaScript.NodeApi/JSValueScopeClosedException). -Values received by a .NET method that is a JS callback are associated with a `Callback` -[scope type](../reference/dotnet/Microsoft.JavaScript.NodeApi/JSValueScopeType). When the method -returns, the callback scope is closed and any values in that scope become invalid. +Values received by a .NET method that is a JS callback belong to the current scope for that call. +When the method returns, that scope is closed and any values in it become invalid. ## Nesting and escaping scopes @@ -23,7 +22,7 @@ JSFunction jsFunction = … foreach (string item in array) { - using (var nestedScope = new JSValueScope()) + using (var nestedScope = JSValueScope.CreateHandleScope()) { // Passing a .NET string to JS requires converting it to JSValue. // The conversion is implicit; the explicit cast is for illustration. @@ -44,7 +43,7 @@ public JSValue EscapableScopeExample(JSCallbackArgs args) foreach (string item in array) { - using (var escapableScope = new JSValueScope(JSValueScopeType.Escapable)) + using (var escapableScope = JSValueScope.CreateEscapableScope()) { JSValue result = jsFunction.Call(thisArg: default, (JSValue)item); if (!result.IsUndefined()) diff --git a/examples/hermes-engine/HermesRuntime.cs b/examples/hermes-engine/HermesRuntime.cs index 1e862c99..37df0ede 100644 --- a/examples/hermes-engine/HermesRuntime.cs +++ b/examples/hermes-engine/HermesRuntime.cs @@ -30,7 +30,8 @@ private HermesRuntime(JSDispatcherQueue dispatcherQueue) JSRuntime runtime = HermesApi.Load("hermes.dll"); using HermesConfig tempConfig = new(); hermes_create_runtime((hermes_config)tempConfig, out _runtime).ThrowIfFailed(); - _rootScope = new JSValueScope(JSValueScopeType.Root, (napi_env)this, runtime); + JSRuntimeContext context = JSRuntimeContext.Create((napi_env)this, runtime); + _rootScope = JSValueScope.CreateRuntimeScope((napi_env)this, context); CreatePolyfills(); } @@ -98,7 +99,7 @@ public static explicit operator napi_env(HermesRuntime value) private void CreatePolyfills() { VerifyElseThrow(JSDispatcherQueue.GetForCurrentThread() == _dispatcherQueue); - using var scope = new JSValueScope(); + using var scope = JSValueScope.CreateHandleScope(); // Add global JSValue global = JSValue.Global; diff --git a/src/NodeApi.DotNetHost/JSMarshaller.cs b/src/NodeApi.DotNetHost/JSMarshaller.cs index 25c164b2..8f8e00b8 100644 --- a/src/NodeApi.DotNetHost/JSMarshaller.cs +++ b/src/NodeApi.DotNetHost/JSMarshaller.cs @@ -89,9 +89,9 @@ public JSMarshaller() typeof(JSRuntimeContext).GetStaticProperty(nameof(JSRuntimeContext.Current)) ?? throw new NotImplementedException("JSRuntimeContext.Current"); - private static readonly PropertyInfo s_moduleContext = - typeof(JSModuleContext).GetStaticProperty(nameof(JSModuleContext.Current)) - ?? throw new NotImplementedException("JSModuleContext.Current"); + private static readonly PropertyInfo s_currentScope = + typeof(JSValueScope).GetStaticProperty(nameof(JSValueScope.Current)) + ?? throw new NotImplementedException("JSValueScope.Current"); private static readonly PropertyInfo s_valueItem = typeof(JSValue).GetIndexer(typeof(string)) @@ -1878,22 +1878,22 @@ private IEnumerable BuildThisArgumentExpressions( if (type.GetCustomAttributes().Any()) { - // For a method on a module class, the .NET object is stored in the module context. + // For a method on a module class, the .NET object is the current module instance. // `ThisArg` is ignored for module-level methods. /* - * ObjectType? __this = JSRuntimeContext.Current.Module as ObjectType; + * ObjectType? __this = JSValueScope.Current.Module as ObjectType; * if (__this == null) return JSValue.Undefined; */ - PropertyInfo moduleProperty = typeof(JSModuleContext).GetProperty( - nameof(JSModuleContext.Module)) - ?? throw new NotImplementedException("JSModuleContext.Module"); + PropertyInfo moduleProperty = typeof(JSValueScope).GetProperty( + nameof(JSValueScope.Module)) + ?? throw new NotImplementedException("JSValueScope.Module"); yield return Expression.Assign( thisVariable, Expression.TypeAs( Expression.Property( - Expression.Property(null, s_moduleContext), + Expression.Property(null, s_currentScope), moduleProperty), type)); yield return Expression.IfThen( diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index 402befe6..98450c4c 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -40,7 +40,7 @@ public sealed class ManagedHost : JSEventEmitter, IDisposable private readonly AssemblyLoadContext _loadContext = new(name: default); #endif - private JSValueScope? _rootScope; + private JSRuntimeContext? _context; /// /// Component that dynamically exports types from loaded assemblies. @@ -177,10 +177,14 @@ public static unsafe int InitializeModule(string argument) napi_env env = new((nint)ulong.Parse(args[0], NumberStyles.HexNumber)); napi_value exports = new((nint)ulong.Parse(args[1], NumberStyles.HexNumber)); napi_value* pResult = (napi_value*)(nint)ulong.Parse(args[2], NumberStyles.HexNumber); + ManagedHostRegistration* registration = args.Length > 3 ? + (ManagedHostRegistration*)(nint)ulong.Parse(args[3], NumberStyles.HexNumber) : null; #else [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] - public static napi_value InitializeModule(napi_env env, napi_value exports) + public static unsafe napi_value InitializeModule( + napi_env env, napi_value exports, nint registrationPtr) { + ManagedHostRegistration* registration = (ManagedHostRegistration*)registrationPtr; Trace($"> ManagedHost.InitializeModule({env.Handle:X8})"); Trace($" .NET Runtime version: {Environment.Version}"); #endif @@ -198,7 +202,13 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) runtime = new TracingJSRuntime(runtime, trace); } - JSValueScope scope = new(JSValueScopeType.Root, env, runtime); + // The managed host registers its context in the environment instance-data block (at the + // module slot). When hosted, the native host owns that block and its finalizer signals + // environment teardown, so the managed context is a non-owner: it writes its own slot but + // does not claim the finalizer, and is disposed via the registration notification below. + bool hosted = registration != null; + JSRuntimeContext context = new(env, runtime); + using JSValueScope scope = JSValueScope.CreateRuntimeScope(env, context); try { @@ -219,9 +229,20 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) ManagedHost host = new(exportsObject) { - _rootScope = scope + _context = context }; + if (hosted) + { + // Root the managed host for the environment lifetime and give the native host a + // native callback to invoke at teardown (never a JS call -- see OnEnvironmentFinalize). + registration->AddonGCHandle = (nint)GCHandle.Alloc(host); +#if !(NETFRAMEWORK || NETSTANDARD) + registration->OnEnvFinalize = + (nint)(delegate* unmanaged[Cdecl])&OnEnvironmentFinalize; +#endif + } + Trace("< ManagedHost.InitializeModule()"); } catch (Exception ex) @@ -238,6 +259,62 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) #endif } +#if !(NETFRAMEWORK || NETSTANDARD) + /// + /// Called natively by the native host when the environment is being torn down. Runs during + /// environment finalization where calling into JavaScript is forbidden, so it touches only + /// managed state. + /// + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] + private static void OnEnvironmentFinalize(nint addon) => OnEnvironmentFinalizeCore(addon); +#else + /// + /// Called by the native host (through the default AppDomain) when the environment is being + /// torn down. Runs during environment finalization where calling into JavaScript is forbidden, + /// so it touches only managed state. + /// + public static int OnEnvironmentFinalize(string argument) + { + OnEnvironmentFinalizeCore((nint)ulong.Parse(argument, NumberStyles.HexNumber)); + return 0; + } +#endif + + private static void OnEnvironmentFinalizeCore(nint addon) + { + if (addon == default) + { + return; + } + + GCHandle handle = GCHandle.FromIntPtr(addon); + try + { + (handle.Target as ManagedHost)?.DisposeOnEnvironmentFinalize(); + } + catch (Exception ex) + { + Trace($"Failed to dispose managed host on environment finalize: {ex}"); + } + finally + { + handle.Free(); + } + } + + /// + /// Disposes the runtime context in response to environment teardown. No JavaScript may be + /// called here; disposing the context marks it disposed (so any late cross-thread post becomes + /// a no-op) and frees its GC handles. The context's references are reclaimed by Node as the + /// environment is torn down. + /// + private void DisposeOnEnvironmentFinalize() + { + JSRuntimeContext? context = _context; + _context = null; + context?.Dispose(); + } + /// /// Resolve references to Node API and other assemblies that loaded assemblies depend on. /// @@ -592,8 +669,8 @@ protected override void Dispose(bool disposing) { if (disposing) { - _rootScope?.Dispose(); - _rootScope = null; + _context?.Dispose(); + _context = null; #if NETFRAMEWORK || NETSTANDARD AppDomain.CurrentDomain.AssemblyResolve -= OnResolvingAssembly; diff --git a/src/NodeApi.DotNetHost/ManagedHostRegistration.cs b/src/NodeApi.DotNetHost/ManagedHostRegistration.cs new file mode 100644 index 00000000..00869630 --- /dev/null +++ b/src/NodeApi.DotNetHost/ManagedHostRegistration.cs @@ -0,0 +1,35 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Runtime.InteropServices; + +namespace Microsoft.JavaScript.NodeApi.DotNetHost; + +/// +/// Native handshake structure the managed host fills in at initialization, so the native host can +/// keep the managed host alive for the environment lifetime and notify it when the environment is +/// torn down. +/// +/// +/// The layout must exactly match the native host's own copy of this structure (in the NodeApi +/// assembly). Both are two pointer-sized fields, passed by pointer across the native/managed +/// boundary. The native host and managed host run in separate .NET runtimes, so the structure is +/// defined independently in each and only its binary layout is shared. +/// +[StructLayout(LayoutKind.Sequential)] +internal struct ManagedHostRegistration +{ + /// + /// A strong to the managed host, allocated and freed only by managed + /// code. The native host treats it as an opaque pointer. + /// + public nint AddonGCHandle; + + /// + /// A native callback pointer (delegate* unmanaged<nint, void>) the native host + /// invokes at environment teardown, or default when the native host uses another channel + /// (the .NET Framework host invokes the finalize method through the default AppDomain instead). + /// + public nint OnEnvFinalize; +} diff --git a/src/NodeApi.Generator/ModuleGenerator.cs b/src/NodeApi.Generator/ModuleGenerator.cs index d4c3bd87..d9e8c3cf 100644 --- a/src/NodeApi.Generator/ModuleGenerator.cs +++ b/src/NodeApi.Generator/ModuleGenerator.cs @@ -24,6 +24,7 @@ public class ModuleGenerator : SourceGenerator, ISourceGenerator { private const string ModuleInitializerClassName = "Module"; private const string ModuleInitializeMethodName = "Initialize"; + private const string ModuleExportsMethodName = "InitializeExports"; private const string ModuleRegisterFunctionName = "napi_register_module_v1"; private readonly JSMarshaller _marshaller = new() @@ -287,26 +288,36 @@ private SourceBuilder GenerateModuleInitializer( s += $"public static class {ModuleInitializerClassName}"; s += "{"; - // The module scope is not disposed after a successful initialization. It becomes - // the parent of callback scopes, allowing the JS runtime instance to be inherited. - s += "private static JSValueScope _moduleScope;"; - - // The unmanaged entrypoint is used only when the AOT-compiled module is loaded. + // The unmanaged entrypoint is used only when the AOT-compiled module is loaded. As the + // root it creates the runtime context; there is no host to resolve it from. s += "#if !NETFRAMEWORK"; s += $"[UnmanagedCallersOnly(EntryPoint = \"{ModuleRegisterFunctionName}\")]"; s += $"public static napi_value _{ModuleInitializeMethodName}(napi_env env, napi_value exports)"; - s += $"{s.Indent}=> {ModuleInitializeMethodName}(env, exports);"; + s += "{"; + s += "JSRuntimeContext context = JSRuntimeContext.Create(env);"; + s += "using var moduleScope = JSValueScope.CreateRuntimeScope(env, context);"; + s += $"return {ModuleExportsMethodName}(moduleScope, exports);"; + s += "}"; s += "#endif"; s++; - // The main initialization entrypoint is called by the `ManagedHost`, and by the unmanaged entrypoint. + // The main initialization entrypoint is called by the `ManagedHost` that loaded this + // module; the scope resolves the runtime context from that host. s += $"public static napi_value {ModuleInitializeMethodName}(napi_env env, napi_value exports)"; s += "{"; - s += "_moduleScope = new JSValueScope(JSValueScopeType.Module, env, runtime: default);"; + s += "using var moduleScope = JSValueScope.CreateRuntimeScope(env);"; + s += $"return {ModuleExportsMethodName}(moduleScope, exports);"; + s += "}"; + s++; + + // The shared body builds the exports within the module scope opened by an entrypoint + // above; the scope stays alive through the catch so it can build the JS error. + s += $"private static napi_value {ModuleExportsMethodName}(JSValueScope moduleScope, napi_value exports)"; + s += "{"; s += "try"; s += "{"; - s += "JSRuntimeContext context = _moduleScope.RuntimeContext;"; - s += "JSValue exportsValue = new(exports, _moduleScope);"; + s += "JSRuntimeContext context = moduleScope.RuntimeContext;"; + s += "JSValue exportsValue = new(exports, moduleScope);"; s++; if (moduleInitializer is IMethodSymbol moduleInitializerMethod) @@ -340,7 +351,6 @@ private SourceBuilder GenerateModuleInitializer( s += "{"; s += "System.Console.Error.WriteLine($\"Failed to export module: {ex}\");"; s += "JSError.ThrowError(ex);"; - s += "_moduleScope.Dispose();"; s += "return exports;"; s += "}"; s += "}"; diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index e925cb19..a8d41fdb 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -7,6 +7,7 @@ using System.IO; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; +using Microsoft.JavaScript.NodeApi.Interop; using Microsoft.JavaScript.NodeApi.Runtime; using static Microsoft.JavaScript.NodeApi.DotNetHost.HostFxr; using static Microsoft.JavaScript.NodeApi.DotNetHost.MSCorEE; @@ -28,9 +29,14 @@ internal unsafe partial class NativeHost : IDisposable private string? _managedHostPath; private ICLRRuntimeHost* _runtimeHost; private hostfxr_handle _hostContextHandle; - private readonly JSValueScope _hostScope; private JSReference? _exports; + // Filled in by the managed host during initialization via the registration struct: a GCHandle + // (owned by the managed runtime) that roots the managed host, and a native callback the native + // host invokes at environment teardown. Both are default until a managed host is initialized. + private nint _addonGCHandle; + private nint _onEnvFinalize; + public static bool IsTracingEnabled { get; } = Environment.GetEnvironmentVariable("NODE_API_TRACE_HOST") == "1"; @@ -194,15 +200,21 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) s_jsRuntime ??= new NodejsRuntime(); - // The native host JSValueScope is not disposed after a successful initialization. It - // becomes the parent of callback scopes, allowing the JS runtime instance to be inherited. - JSValueScope hostScope = new(JSValueScopeType.NoContext, env, s_jsRuntime); + // The native host's context occupies the host instance-data slot, so the initialize()/ + // dispose() callbacks (dispatched later with no parent scope) recover it via FromEnv. + JSRuntimeContext.UseHostContextSlot(); + + // The host owns its context (inline, non-TSFN sync context); the transient scope only + // references it and is opened before the try so the catch can still build a JSValue error. + // The context outlives the scope -- rooted by its instance-data slot, disposed by that + // slot's finalizer (which disposes the NativeHost). + JSRuntimeContext context = new(env, s_jsRuntime, new JSInlineSynchronizationContext()); + using JSValueScope hostScope = JSValueScope.CreateRuntimeScope(env, context); try { - NativeHost host = new(hostScope); + NativeHost host = new(); + context.SetDisposableAnnotation(host); - // Do not use JSModuleBuilder here because it relies on having a current context. - // But the context will be set by the managed host. new JSValue(exports, hostScope).DefineProperties( // The package index.js will invoke the initialize method with the path to // the managed host assembly. @@ -213,7 +225,6 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) string message = $"Failed to load CLR native host module: {ex}"; Trace(message); s_jsRuntime.Throw(env, (napi_value)JSValue.CreateError(null, (JSValue)message)); - hostScope.Dispose(); } Trace("< NativeHost.InitializeModule()"); @@ -221,9 +232,37 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) return exports; } - private NativeHost(JSValueScope hostScope) + [StructLayout(LayoutKind.Sequential)] + private struct ManagedHostRegistration { - _hostScope = hostScope; + public nint AddonGCHandle; + public nint OnEnvFinalize; + } + + private void NotifyManagedHostEnvironmentFinalize() + { + if (_onEnvFinalize != default) + { + // hostfxr (.NET 5+): the managed host provided a native callback pointer. + ((delegate* unmanaged[Cdecl])_onEnvFinalize)(_addonGCHandle); + } + else if (_runtimeHost is not null && _addonGCHandle != default && _managedHostPath is not null) + { + // .NET Framework: invoke the managed finalize through the default AppDomain. This is a + // native call into the (still-loaded) CLR, never a JavaScript call. + try + { + _runtimeHost->ExecuteInDefaultAppDomain( + _managedHostPath, + s_managedHostTypeName, + "OnEnvironmentFinalize", + ((ulong)_addonGCHandle).ToString("X8")); + } + catch (Exception ex) + { + Trace("Failed to notify managed host on environment finalize: " + ex); + } + } } /// @@ -350,8 +389,10 @@ private JSValue InitializeFrameworkHost( napi_value exports = (napi_value)exportsValue; // The method to be executed must take a single string argument and return a uint. - // So, encode the parameters and retval pointer in the argument string. - string argument = $"{(ulong)env.Handle:X8},{(ulong)exports.Handle:X8},{(ulong)&exports:X8}"; + // So, encode the parameters, retval pointer, and registration pointer in the argument. + ManagedHostRegistration registration = default; + string argument = $"{(ulong)env.Handle:X8},{(ulong)exports.Handle:X8}," + + $"{(ulong)&exports:X8},{(ulong)®istration:X8}"; Trace($" Calling {s_managedHostTypeName}.{nameof(InitializeModule)}({argument})"); _runtimeHost->ExecuteInDefaultAppDomain( @@ -360,6 +401,9 @@ private JSValue InitializeFrameworkHost( nameof(InitializeModule), argument); + _addonGCHandle = registration.AddonGCHandle; + _onEnvFinalize = registration.OnEnvFinalize; + exportsValue = exports; return exportsValue; } @@ -446,12 +490,6 @@ private JSValue InitializeDotNetHost( Trace(" Invoking managed host method: " + nameof(InitializeModule)); - // Invoke the managed host initialize method. - // (It will define some properties on the exports object passed in.) - napi_register_module_v1 initializeModule = - Marshal.GetDelegateForFunctionPointer( - initializeModulePointer); - // Create an "exports" object for the managed host module initialization. var exports = JSValue.CreateObject(); exports.SetProperty("require", require); @@ -460,9 +498,19 @@ private JSValue InitializeDotNetHost( // Define a dispose method implemented by the native host that closes the CLR context. // The managed host proxy will pass through dispose calls to this callback. exports.DefineProperties(new JSPropertyDescriptor( - "dispose", (_) => { Dispose(); return default; })); - - exports = initializeModule((napi_env)exports.Scope, (napi_value)exports); + "dispose", (_) => { CloseRuntimeHost(); return default; })); + + // Invoke the managed host initialize method. It defines properties on the exports object + // and fills in the registration so the native host can keep the managed host alive and + // notify it when the environment is torn down. + ManagedHostRegistration registration = default; + var initializeModule = + (delegate* unmanaged[Cdecl]) + initializeModulePointer; + exports = initializeModule((napi_env)exports.Scope, (napi_value)exports, (nint)(®istration)); + + _addonGCHandle = registration.AddonGCHandle; + _onEnvFinalize = registration.OnEnvFinalize; return exports; } @@ -498,6 +546,22 @@ private hostfxr_handle InitializeManagedRuntime( public void Dispose() { + // Called by the host context when the environment is torn down (the NativeHost is a + // disposable annotation on that context). Runs during environment finalization, where + // calling into JS is forbidden, so it only notifies the managed host (a native call) and + // drops the exports reference; the exports napi_ref is reclaimed by Node as the env dies. + NotifyManagedHostEnvironmentFinalize(); + _addonGCHandle = default; + _onEnvFinalize = default; + _exports = null; + } + + private void CloseRuntimeHost() + { + // Closes the CLR host context handle / releases the .NET Framework runtime host. This is + // process/runtime-level teardown, separate from environment teardown, invoked only by the + // optional JS dispose() hook. + // Close the CLR host context handle, if it's still open. if (_hostContextHandle != default) { diff --git a/src/NodeApi/Interop/JSCallbackDescriptor.cs b/src/NodeApi/Interop/JSCallbackDescriptor.cs index 682f0752..e865acf5 100644 --- a/src/NodeApi/Interop/JSCallbackDescriptor.cs +++ b/src/NodeApi/Interop/JSCallbackDescriptor.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using System.Runtime.CompilerServices; namespace Microsoft.JavaScript.NodeApi.Interop; @@ -15,10 +16,10 @@ namespace Microsoft.JavaScript.NodeApi.Interop; public readonly struct JSCallbackDescriptor { /// - /// Saves the module context under which the callback was defined, so that multiple .NET + /// Saves the module instance holder under which the callback was defined, so that multiple .NET /// modules in the same process can register callbacks for module-level functions. /// - internal JSModuleContext? ModuleContext { get; } + internal StrongBox? ModuleHolder { get; } /// /// Gets the name of the callback, for debugging purposes. @@ -37,27 +38,27 @@ public readonly struct JSCallbackDescriptor public object? Data { get; } public JSCallbackDescriptor(JSCallback callback, object? data = null) - : this(null, callback, data, JSValueScope.Current.ModuleContext) + : this(null, callback, data, JSValueScope.Current.ModuleHolder) { } public JSCallbackDescriptor(string? name, JSCallback callback, object? data = null) - : this(name, callback, data, JSValueScope.Current.ModuleContext) + : this(name, callback, data, JSValueScope.Current.ModuleHolder) { } - internal JSCallbackDescriptor(JSCallback callback, object? data, JSModuleContext? moduleContext) - : this(null, callback, data, moduleContext) + internal JSCallbackDescriptor(JSCallback callback, object? data, StrongBox? moduleHolder) + : this(null, callback, data, moduleHolder) { } internal JSCallbackDescriptor( - string? name, JSCallback callback, object? data, JSModuleContext? moduleContext) + string? name, JSCallback callback, object? data, StrongBox? moduleHolder) { Name = name; Callback = callback ?? throw new ArgumentNullException(nameof(callback)); Data = data; - ModuleContext = moduleContext; + ModuleHolder = moduleHolder; } public static implicit operator JSCallbackDescriptor(JSCallback callback) => new(callback); diff --git a/src/NodeApi/Interop/JSModuleBuilderOfT.cs b/src/NodeApi/Interop/JSModuleBuilderOfT.cs index 072c55af..b45f1b70 100644 --- a/src/NodeApi/Interop/JSModuleBuilderOfT.cs +++ b/src/NodeApi/Interop/JSModuleBuilderOfT.cs @@ -19,21 +19,21 @@ public JSModuleBuilder() : base(Unwrap) private static new T? Unwrap(JSCallbackArgs _) { - return (T?)JSModuleContext.Current.Module; + return (T?)JSValueScope.Current.Module; } /// /// Exports the built properties to the module exports object. /// /// An object that represents the module instance and is - /// used as the 'this' argument for any non-static methods on the module. If the object - /// implements then it is also registered for disposal when - /// the module is unloaded. + /// used as the 'this' argument for any non-static methods on the module. /// Object to be returned from the module initializer. /// The module exports. public JSValue ExportModule(T module, JSObject exports) { - JSModuleContext.Current.Module = module; + // Write through the holder the descriptors captured, so callbacks bound before the module + // instance existed observe it. + JSValueScope.Current.ModuleHolder!.Value = module; exports.DefineProperties(Properties.ToArray()); return exports; } diff --git a/src/NodeApi/Interop/JSModuleContext.cs b/src/NodeApi/Interop/JSModuleContext.cs deleted file mode 100644 index 371e56a4..00000000 --- a/src/NodeApi/Interop/JSModuleContext.cs +++ /dev/null @@ -1,42 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using System; - -namespace Microsoft.JavaScript.NodeApi.Interop; - -/// -/// Manages JavaScript interop context for the lifetime of a .NET module. -/// -/// -/// A instance is constructed when the module is loaded and disposed -/// when the module is unloaded. -/// -public sealed class JSModuleContext : IDisposable -{ - /// - /// Gets the current module context. - /// - public static JSModuleContext Current => JSValueScope.Current.ModuleContext - ?? throw new InvalidCastException("No current module context."); - - /// - /// Gets an instance of the class that represents the module, or null if there is no module - /// class. - /// - public object? Module { get; internal set; } - - public bool IsDisposed { get; private set; } - - public void Dispose() - { - if (IsDisposed) return; - - IsDisposed = true; - - if (Module is IDisposable module) - { - module.Dispose(); - } - } -} diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 2c2388c1..4d27ec39 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -37,8 +37,6 @@ public sealed class JSRuntimeContext : IDisposable /// public const string GlobalObjectName = "node_api_dotnet"; - private readonly napi_env _env; - // Track JS constructors and instance JS wrappers for exported classes, enabling // .NET objects to be automatically wrapped when returned to JS, and re-wrapped as needed // if the (weakly-referenced) JS wrapper has been released. @@ -113,6 +111,30 @@ public sealed class JSRuntimeContext : IDisposable private readonly ConcurrentDictionary _collectionProxyHandlerMap = new(); + // Two buckets so ownership is explicit: DisposableAnnotations are disposed at context teardown; + // Annotations are not. Both are lazy and touched only on the JS thread. + private Dictionary? _annotations; + private Dictionary? _disposableAnnotations; + + // Env instance-data layout: one GCHandle slot per runtime sharing the napi_env. Slot 0 is the + // module context (managed host / AOT module / embedding); slot 1 is the native host context. + // A runtime reads and writes only its own slot, so it never dereferences the other runtime's + // GCHandle (which belongs to a separate GC heap). + private const int ModuleContextSlot = 0; + private const int HostContextSlot = 1; + private const int InstanceDataSlotCount = 2; + + // This runtime's slot in the instance-data block: the module slot by default, or the host slot + // once the native host calls UseHostContextSlot() at startup. + private static int s_instanceDataSlot = ModuleContextSlot; + + // The runtime used to read env instance data in FromEnv, captured when a context registers. + private static JSRuntime? s_instanceDataRuntime; + + // A GCHandle rooting this context, used both as its env instance-data slot value and as the + // finalize hint for pooled GC handles. It is intentionally never freed: pooled-handle + // finalizers dereference it during env teardown, after this context is already disposed. + internal napi_env EnvironmentHandle { get @@ -122,19 +144,60 @@ internal napi_env EnvironmentHandle throw new ObjectDisposedException(nameof(JSRuntimeContext)); } - return _env; + return UncheckedEnvironmentHandle; } } + /// + /// Gets the environment handle without checking whether the context is disposed. For use + /// only where a checked access is unnecessary, such as capturing the env to release a + /// reference on the JS thread (where a disposed context makes the release a safe no-op). + /// + internal napi_env UncheckedEnvironmentHandle { get; } + + /// + /// Gets the GCHandle that roots this context, for use as a finalize hint by scopes that adopt + /// this context. + /// + internal nint ContextHandle { get; } + public static explicit operator napi_env(JSRuntimeContext context) { if (context is null) throw new ArgumentNullException(nameof(context)); return context.EnvironmentHandle; } - public static explicit operator JSRuntimeContext(napi_env env) - => JSValue.GetInstanceData(env) as JSRuntimeContext - ?? throw new InvalidCastException("Context is not found in napi_env instance data."); + /// + /// Resolves the for the calling runtime from a napi_env, via the + /// env instance-data block, or null if none is registered. Unlike this + /// does not require a current scope, so callback dispatch can recover the context when no scope + /// is on the thread-static stack yet. + /// + public static unsafe JSRuntimeContext? FromEnv(napi_env env) + { + JSRuntime? runtime = s_instanceDataRuntime; + if (runtime is null) + { + return null; + } + + runtime.GetInstanceData(env, out nint instanceData).ThrowIfFailed(); + if (instanceData == default) + { + return null; + } + + nint slotHandle = ((nint*)instanceData)[s_instanceDataSlot]; + return slotHandle == default + ? null + : GCHandle.FromIntPtr(slotHandle).Target as JSRuntimeContext; + } + + /// + /// Configures the calling runtime to use the native host's instance-data slot. Called once by + /// the native host at startup; every other runtime keeps the default module slot. + /// + internal static void UseHostContextSlot() => s_instanceDataSlot = HostContextSlot; public bool IsDisposed { get; private set; } @@ -147,7 +210,31 @@ public static explicit operator JSRuntimeContext(napi_env env) public JSRuntime Runtime { get; } - public JSSynchronizationContext SynchronizationContext { get; } + private JSSynchronizationContext? _synchronizationContext; + + /// + /// Gets the synchronization context that marshals callbacks and continuations to the JS thread. + /// A default one is created on first access, which happens while a scope for this context is + /// current, because creating it requires the current scope's runtime and environment. + /// + public JSSynchronizationContext SynchronizationContext + => _synchronizationContext ??= JSSynchronizationContext.Create(); + + /// + /// Creates a runtime context for a JS environment. Used by AOT module entry points and other + /// embedders that own the environment and therefore create the context rather than resolving + /// it from a host. + /// + /// The JS environment handle. + /// The JS runtime interface; defaults to a . + /// + /// The synchronization context owned by this context; a + /// default one is created when omitted. + public static JSRuntimeContext Create( + napi_env env, + JSRuntime? runtime = null, + JSSynchronizationContext? synchronizationContext = null) + => new(env, runtime ?? new NodejsRuntime(), synchronizationContext); internal JSRuntimeContext( napi_env env, @@ -156,10 +243,70 @@ internal JSRuntimeContext( { if (env.IsNull) throw new ArgumentNullException(nameof(env)); - _env = env; + UncheckedEnvironmentHandle = env; Runtime = runtime; - JSValue.SetInstanceData(env, this); - SynchronizationContext = synchronizationContext ?? JSSynchronizationContext.Create(); + ContextHandle = (nint)GCHandle.Alloc(this); + RegisterInstanceData(env, runtime); + + _synchronizationContext = synchronizationContext; + } + + /// + /// Registers this context in the env instance-data block at this runtime's slot, allocating the + /// block and attaching the teardown finalizer if this runtime is the first to claim the slot. + /// + private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) + { + s_instanceDataRuntime = runtime; + + runtime.GetInstanceData(env, out nint instanceData).ThrowIfFailed(); + if (instanceData == default) + { + instanceData = Marshal.AllocHGlobal(IntPtr.Size * InstanceDataSlotCount); + for (int i = 0; i < InstanceDataSlotCount; i++) + { + ((nint*)instanceData)[i] = default; + } + + runtime.SetInstanceData( + env, + instanceData, + new napi_finalize(s_finalizeInstanceData), + finalizeHint: default).ThrowIfFailed(); + } + + ((nint*)instanceData)[s_instanceDataSlot] = ContextHandle; + } + +#if !UNMANAGED_DELEGATES + private static readonly napi_finalize.Delegate s_finalizeInstanceData = FinalizeInstanceData; +#else + private static readonly unsafe delegate* unmanaged[Cdecl] + s_finalizeInstanceData = &FinalizeInstanceData; +#endif + +#if UNMANAGED_DELEGATES + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] +#endif + private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hint) + { + // Runs during env teardown, where calling into JS is forbidden. Dispose the owning + // runtime's context and free the shared block. Only this runtime's slot is read (never the + // other runtime's); the slot GCHandles are intentionally left rooted (see _contextHandle). + nint slotHandle = ((nint*)data)[s_instanceDataSlot]; + if (slotHandle != default) + { + try + { + (GCHandle.FromIntPtr(slotHandle).Target as JSRuntimeContext)?.Dispose(); + } + catch + { + // A finalizer must never throw; teardown continues regardless. + } + } + + Marshal.FreeHGlobal(data); } /// @@ -695,6 +842,41 @@ public async Task ImportAsync( return value; } + /// + /// Gets a non-owning annotation associated with this context by its type, or null if none. + /// + public T? GetAnnotation() where T : class + => _annotations != null && _annotations.TryGetValue(typeof(T), out object? value) + ? (T)value : null; + + /// + /// Associates a non-owning annotation with this context, keyed by its type. The context never + /// disposes it. + /// + public void SetAnnotation(T value) where T : class + { + if (value is null) throw new ArgumentNullException(nameof(value)); + (_annotations ??= new())[typeof(T)] = value; + } + + /// + /// Gets an owning annotation associated with this context by its type, or null if none. + /// + public T? GetDisposableAnnotation() where T : class, IDisposable + => _disposableAnnotations != null && + _disposableAnnotations.TryGetValue(typeof(T), out IDisposable? value) + ? (T)value : null; + + /// + /// Associates an owning annotation with this context, keyed by its type. The context disposes + /// it when the context itself is disposed (at environment teardown). + /// + public void SetDisposableAnnotation(T value) where T : class, IDisposable + { + if (value is null) throw new ArgumentNullException(nameof(value)); + (_disposableAnnotations ??= new())[typeof(T)] = value; + } + public void Dispose() { if (IsDisposed) return; @@ -711,6 +893,22 @@ public void Dispose() DisposeReferences(_classMap.Values); DisposeReferences(_staticClassMap.Values); DisposeReferences(_structMap.Values); + + // Disposed after IsDisposed is set, so a late cross-thread post is already a no-op. + if (_disposableAnnotations != null) + { + foreach (IDisposable annotation in _disposableAnnotations.Values) + { + try + { + annotation.Dispose(); + } + catch + { + // A failing annotation must not prevent the rest of teardown. + } + } + } } private static void DisposeReferences( diff --git a/src/NodeApi/Interop/JSSynchronizationContext.cs b/src/NodeApi/Interop/JSSynchronizationContext.cs index 48e2463b..e260ba7d 100644 --- a/src/NodeApi/Interop/JSSynchronizationContext.cs +++ b/src/NodeApi/Interop/JSSynchronizationContext.cs @@ -416,3 +416,39 @@ public override void OpenAsyncScope() { } public override void CloseAsyncScope() { } } + +/// +/// A synchronization context that runs work inline when already on the JS thread and drops it +/// otherwise, without a thread-safe function. Used by the native host, which only ever operates +/// on the JS thread and must not stand up a TSFN (which would ref the environment and require an +/// env cleanup hook). +/// +/// +/// Because there is no TSFN to marshal to, work posted from another thread (such as a +/// finalizer running on the GC thread) is dropped rather than +/// scheduled. That is safe for the native host: its references are env-lifetime and reclaimed by +/// Node at teardown, so a dropped off-thread delete never leaves a live reference behind and never +/// touches a dead environment. +/// +internal sealed class JSInlineSynchronizationContext : JSSynchronizationContext +{ + public override void OpenAsyncScope() { } + + public override void CloseAsyncScope() { } + + public override void Post(SendOrPostCallback callback, object? state) + { + if (!IsDisposed && Current == this) + { + callback(state); + } + } + + public override void Send(SendOrPostCallback callback, object? state) + { + if (!IsDisposed && Current == this) + { + callback(state); + } + } +} diff --git a/src/NodeApi/Interop/JSThreadSafeFunction.cs b/src/NodeApi/Interop/JSThreadSafeFunction.cs index a7dbef68..ca7bbb17 100644 --- a/src/NodeApi/Interop/JSThreadSafeFunction.cs +++ b/src/NodeApi/Interop/JSThreadSafeFunction.cs @@ -231,7 +231,7 @@ private static unsafe void CustomCallJS(napi_env env, napi_value jsCallback, nin try { - using JSValueScope scope = new(JSValueScopeType.Callback, env, runtime: null); + using JSValueScope scope = JSValueScope.CreateRuntimeScope(env); object? callbackData = null; if (data != default) @@ -265,7 +265,9 @@ private static unsafe void DefaultCallJS(napi_env env, napi_value jsCallback, ni try { - using JSValueScope scope = new(JSValueScopeType.Callback, env, runtime: null); + // Dispatched on the JS thread; the scope references the context inherited from the + // parent scope, or recovered from env instance data when there is none. + using JSValueScope scope = JSValueScope.CreateRuntimeScope(env); if (data != default) { diff --git a/src/NodeApi/JSError.cs b/src/NodeApi/JSError.cs index 33437ae9..942c4199 100644 --- a/src/NodeApi/JSError.cs +++ b/src/NodeApi/JSError.cs @@ -200,34 +200,28 @@ private static JSValue CreateErrorValueForException(Exception exception, out str JSValue error = (exception as JSException)?.Error?.Value ?? JSValue.CreateError(code: null, (JSValue)message); - // A no-context scope is used when initializing the host. In that case, do not attempt - // to override the stack property, because if initialization fails the scope may not - // be available for the stack callback. - if (JSValueScope.Current.ScopeType != JSValueScopeType.NoContext) + // When running on V8, the `Error.captureStackTrace()` function and `Error.stack` + // property can be used to add the .NET stack info to the JS error stack. + JSValue captureStackTrace = JSValue.Global["Error"]["captureStackTrace"]; + if (captureStackTrace.IsFunction()) { - // When running on V8, the `Error.captureStackTrace()` function and `Error.stack` - // property can be used to add the .NET stack info to the JS error stack. - JSValue captureStackTrace = JSValue.Global["Error"]["captureStackTrace"]; - if (captureStackTrace.IsFunction()) - { - // Capture the stack trace of the .NET exception, which will be combined with - // the JS stack trace when requested. - JSValue dotnetStack = exception.StackTrace?.Replace("\r", string.Empty) - ?? string.Empty; - - // Capture the current JS stack trace as an object. - // Defer formatting the stack as a string until requested. - JSObject jsStack = new(); - captureStackTrace.Call(default, jsStack); - - // Override the `stack` property of the JS Error object, and add private - // properties that the overridden property getter uses to construct the stack. - error.DefineProperties( - JSPropertyDescriptor.AccessorProperty( - "stack", GetErrorStack, setter: null, JSPropertyAttributes.DefaultProperty), - JSPropertyDescriptor.DataProperty("__dotnetStack", dotnetStack), - JSPropertyDescriptor.DataProperty("__jsStack", jsStack)); - } + // Capture the stack trace of the .NET exception, which will be combined with + // the JS stack trace when requested. + JSValue dotnetStack = exception.StackTrace?.Replace("\r", string.Empty) + ?? string.Empty; + + // Capture the current JS stack trace as an object. + // Defer formatting the stack as a string until requested. + JSObject jsStack = new(); + captureStackTrace.Call(default, jsStack); + + // Override the `stack` property of the JS Error object, and add private + // properties that the overridden property getter uses to construct the stack. + error.DefineProperties( + JSPropertyDescriptor.AccessorProperty( + "stack", GetErrorStack, setter: null, JSPropertyAttributes.DefaultProperty), + JSPropertyDescriptor.DataProperty("__dotnetStack", dotnetStack), + JSPropertyDescriptor.DataProperty("__jsStack", jsStack)); } return error; @@ -238,7 +232,7 @@ public readonly void ThrowError() if (_errorRef is null) return; - using var scope = new JSValueScope(JSValueScopeType.Handle); + using var scope = JSValueScope.CreateHandleScope(); if (IsExceptionPending()) throw new JSException(new JSError()); diff --git a/src/NodeApi/JSPropertyDescriptor.cs b/src/NodeApi/JSPropertyDescriptor.cs index a2e34289..3b5e1c20 100644 --- a/src/NodeApi/JSPropertyDescriptor.cs +++ b/src/NodeApi/JSPropertyDescriptor.cs @@ -3,7 +3,7 @@ using System; using System.Diagnostics; -using Microsoft.JavaScript.NodeApi.Interop; +using System.Runtime.CompilerServices; namespace Microsoft.JavaScript.NodeApi; @@ -16,10 +16,10 @@ namespace Microsoft.JavaScript.NodeApi; public readonly struct JSPropertyDescriptor { /// - /// Saves the module context under which the callback was defined, so that multiple .NET + /// Saves the module instance holder under which the callback was defined, so that multiple .NET /// modules in the same process can register callbacks for module-level functions. /// - internal JSModuleContext? ModuleContext { get; init; } + internal StrongBox? ModuleHolder { get; init; } // Either Name or NameValue should be non-null. // NameValue supports non-string property names like symbols. @@ -49,7 +49,7 @@ public JSPropertyDescriptor( JSPropertyAttributes attributes = JSPropertyAttributes.Default, object? data = null) { - ModuleContext = JSValueScope.Current.ModuleContext; + ModuleHolder = JSValueScope.Current.ModuleHolder; Name = name; Method = method; @@ -72,7 +72,7 @@ public JSPropertyDescriptor( JSPropertyAttributes attributes = JSPropertyAttributes.Default, object? data = null) { - ModuleContext = JSValueScope.Current.ModuleContext; + ModuleHolder = JSValueScope.Current.ModuleHolder; NameValue = name; Method = method; diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 250b0286..40456de4 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -5,6 +5,7 @@ using System.Diagnostics.CodeAnalysis; using System.Threading; using Microsoft.JavaScript.NodeApi.Interop; +using Microsoft.JavaScript.NodeApi.Runtime; using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; namespace Microsoft.JavaScript.NodeApi; @@ -29,8 +30,7 @@ namespace Microsoft.JavaScript.NodeApi; public class JSReference : IDisposable { private readonly napi_ref _handle; - private readonly napi_env _env; - private readonly JSRuntimeContext? _context; + private readonly JSRuntimeContext _context; /// /// Creates a new instance of a that holds a strong or weak @@ -64,7 +64,6 @@ public JSReference(napi_ref handle, bool isWeak = false) JSValueScope currentScope = JSValueScope.Current; // Thread access to the env will be checked on reference handle use. - _env = currentScope.UncheckedEnvironmentHandle; _handle = handle; _context = currentScope.RuntimeContext; IsWeak = isWeak; @@ -134,7 +133,7 @@ public static bool TryCreateReference( /// accesses the referenced value, if there is a possibility that the current execution /// context is not already on the correct thread. /// - public JSSynchronizationContext? SynchronizationContext => _context?.SynchronizationContext; + public JSSynchronizationContext? SynchronizationContext => _context.SynchronizationContext; private napi_env Env { @@ -142,7 +141,7 @@ private napi_env Env { ThrowIfDisposed(); ThrowIfInvalidThreadAccess(); - return _env; + return _context.UncheckedEnvironmentHandle; } } @@ -321,7 +320,7 @@ private void ThrowIfDisposed() private void ThrowIfInvalidThreadAccess() { JSValueScope currentScope = JSValueScope.Current; - if ((napi_env)currentScope != _env) + if ((napi_env)currentScope != _context.UncheckedEnvironmentHandle) { int threadId = Environment.CurrentManagedThreadId; string? threadName = Thread.CurrentThread.Name; @@ -351,65 +350,44 @@ protected virtual void Dispose(bool disposing) return; } + // Once the context is disposed its napi_env was torn down and Node already reclaimed every + // napi_ref, so there is nothing to delete and touching the env would be unsafe. This single + // flag invalidates all references at once, for both the explicit and finalizer paths. + if (_context.IsDisposed) + { + IsDisposed = true; + return; + } + IsDisposed = true; + // The guard above handles an already-disposed context; if it is disposed concurrently after + // that check, the posted delete is still a safe no-op (the napi_ref went with the env). + napi_env env = _context.UncheckedEnvironmentHandle; + napi_ref handle = _handle; + JSRuntime runtime = _context.Runtime; + if (disposing) { - // Explicit disposal preserves the documented behavior, including asserting that a - // no-context reference is disposed from the JS thread. - if (_context == null) - { - ThrowIfInvalidThreadAccess(); - JSValueScope.CurrentRuntime.DeleteReference(_env, _handle).ThrowIfFailed(); - } - else - { - _context.SynchronizationContext.Post( - () => _context.Runtime.DeleteReference( - _env, _handle).ThrowIfFailed(), allowSync: true); - } + // Delete the reference on the JS thread (inline if already there). + _context.SynchronizationContext.Post( + () => runtime.DeleteReference(env, handle).ThrowIfFailed(), allowSync: true); } else { // The finalizer runs on the GC finalizer thread and MUST NOT throw: an exception // escaping a finalizer terminates the process (observed as a fatal - // JSInvalidThreadAccessException / SIGSEGV during worker-thread teardown). Release the - // native reference only if it can be done without switching threads or asserting an - // active JS scope, and never let an exception propagate. - DisposeFromFinalizer(); - } - } - - private void DisposeFromFinalizer() - { - try - { - if (_context == null) + // JSInvalidThreadAccessException / SIGSEGV during worker-thread teardown). Post the + // delete to the JS thread; the synchronization context is a safe no-op once it (and + // the environment) are gone. + try { - // A no-context reference (for example one created from the native host scope) can - // only be deleted on the JS thread. CurrentOrNull is thread-static, so on the real - // GC finalizer thread it is null and this delete is skipped; the napi_ref is then - // reclaimed when the JS environment is destroyed. The guarded delete still runs if - // Dispose(disposing: false) is ever invoked on the owning JS thread. A no-context - // scope has no synchronization context, so the finalizer cannot marshal the delete - // to the JS thread; doing so would require an env-scoped cleanup queue in the - // native host (tracked as a follow-up). - JSValueScope? scope = JSValueScope.CurrentOrNull; - if (scope != null && scope.UncheckedEnvironmentHandle == _env) - { - scope.Runtime.DeleteReference(_env, _handle); - } - } - else - { - // Post the delete to the JS thread. The synchronization context is a safe no-op - // once it has been disposed (that is, after the worker has been torn down). - _context.SynchronizationContext?.Post( + _context.SynchronizationContext.Post( () => { try { - _context.Runtime.DeleteReference(_env, _handle); + runtime.DeleteReference(env, handle); } catch { @@ -418,10 +396,10 @@ private void DisposeFromFinalizer() }, allowSync: false); } - } - catch - { - // Never allow an exception to escape the finalizer. + catch + { + // Never allow an exception to escape the finalizer. + } } } diff --git a/src/NodeApi/JSValue.cs b/src/NodeApi/JSValue.cs index 52f30e3b..76f45e96 100644 --- a/src/NodeApi/JSValue.cs +++ b/src/NodeApi/JSValue.cs @@ -194,9 +194,7 @@ public static unsafe JSValue CreateFunction( new JSCallbackDescriptor(name, callback, callbackData)); JSValue func = CreateFunction( name, - new napi_callback( - JSValueScope.Current?.ScopeType == JSValueScopeType.NoContext ? - s_invokeJSCallbackNC : s_invokeJSCallback), + new napi_callback(s_invokeJSCallback), (nint)descriptorHandle); func.AddGCHandleFinalizer((nint)descriptorHandle); return func; @@ -801,9 +799,7 @@ public static unsafe JSValue DefineClass( { GCHandle descriptorHandle = JSRuntimeContext.Current.AllocGCHandle(constructorDescriptor); JSValue? func = null; - napi_callback callback = new( - Current?.ScopeType == JSValueScopeType.NoContext - ? s_invokeJSCallbackNC : s_invokeJSCallback); + napi_callback callback = new(s_invokeJSCallback); nint[] handles = ToUnmanagedPropertyDescriptors( name, propertyDescriptors, (name, descriptorsPtr) => @@ -1163,36 +1159,6 @@ public JSValue GetAllPropertyNames( (napi_key_conversion)conversion, out napi_value result).ThrowIfFailed(result); - //TODO: (vmoroz) What env parameter does here? - //TODO: (vmoroz) Move instance data to somewhere else. It must be not in the public API - internal static unsafe void SetInstanceData(napi_env env, object? data) - { - JSRuntime runtime = CurrentRuntime; - runtime.GetInstanceData(env, out nint handlePtr).ThrowIfFailed(); - if (handlePtr != default) - { - // Current napi_set_instance_data implementation does not call finalizer when we replace existing instance data. - // It means that we only remove the GC root, but do not call Dispose. - GCHandle.FromIntPtr(handlePtr).Free(); - } - - if (data != null) - { - GCHandle handle = GCHandle.Alloc(data); - runtime.SetInstanceData( - env, - (nint)handle, - new napi_finalize(s_finalizeGCHandleToDisposable), - finalizeHint: default).ThrowIfFailed(); - } - } - - internal static object? GetInstanceData(napi_env env) - { - CurrentRuntime.GetInstanceData(env, out nint data).ThrowIfFailed(); - return (data != default) ? GCHandle.FromIntPtr(data).Target : null; - } - public void DetachArrayBuffer() => GetRuntime(out napi_env env, out napi_value handle) .DetachArrayBuffer(env, handle).ThrowIfFailed(); @@ -1218,13 +1184,8 @@ public void Seal() => GetRuntime(out napi_env env, out napi_value handle) internal static readonly napi_callback.Delegate s_invokeJSMethod = InvokeJSMethod; internal static readonly napi_callback.Delegate s_invokeJSGetter = InvokeJSGetter; internal static readonly napi_callback.Delegate s_invokeJSSetter = InvokeJSSetter; - internal static readonly napi_callback.Delegate s_invokeJSCallbackNC = InvokeJSCallbackNoContext; - internal static readonly napi_callback.Delegate s_invokeJSMethodNC = InvokeJSMethodNoContext; - internal static readonly napi_callback.Delegate s_invokeJSGetterNC = InvokeJSGetterNoContext; - internal static readonly napi_callback.Delegate s_invokeJSSetterNC = InvokeJSSetterNoContext; internal static readonly napi_finalize.Delegate s_finalizeGCHandle = FinalizeGCHandle; - internal static readonly napi_finalize.Delegate s_finalizeGCHandleToDisposable = FinalizeGCHandleToDisposable; internal static readonly napi_finalize.Delegate s_finalizeGCHandleToPinnedMemory = FinalizeGCHandleToPinnedMemory; internal static readonly napi_finalize.Delegate s_callFinalizeAction = CallFinalizeAction; #else @@ -1236,19 +1197,9 @@ internal static readonly unsafe delegate* unmanaged[Cdecl] s_invokeJSGetter = &InvokeJSGetter; internal static readonly unsafe delegate* unmanaged[Cdecl] s_invokeJSSetter = &InvokeJSSetter; - internal static readonly unsafe delegate* unmanaged[Cdecl] - s_invokeJSCallbackNC = &InvokeJSCallbackNoContext; - internal static readonly unsafe delegate* unmanaged[Cdecl] - s_invokeJSMethodNC = &InvokeJSMethodNoContext; - internal static readonly unsafe delegate* unmanaged[Cdecl] - s_invokeJSGetterNC = &InvokeJSGetterNoContext; - internal static readonly unsafe delegate* unmanaged[Cdecl] - s_invokeJSSetterNC = &InvokeJSSetterNoContext; internal static readonly unsafe delegate* unmanaged[Cdecl] s_finalizeGCHandle = &FinalizeGCHandle; - internal static readonly unsafe delegate* unmanaged[Cdecl] - s_finalizeGCHandleToDisposable = &FinalizeGCHandleToDisposable; internal static readonly unsafe delegate* unmanaged[Cdecl] s_finalizeGCHandleToPinnedMemory = &FinalizeGCHandleToPinnedMemory; internal static readonly unsafe delegate* unmanaged[Cdecl] @@ -1262,7 +1213,7 @@ internal static unsafe napi_value InvokeJSCallback( napi_env env, napi_callback_info callbackInfo) { return InvokeCallback( - env, callbackInfo, JSValueScopeType.Callback, (descriptor) => descriptor); + env, callbackInfo, (descriptor) => descriptor); } #if UNMANAGED_DELEGATES @@ -1271,11 +1222,11 @@ internal static unsafe napi_value InvokeJSCallback( private static unsafe napi_value InvokeJSMethod(napi_env env, napi_callback_info callbackInfo) { return InvokeCallback( - env, callbackInfo, JSValueScopeType.Callback, (propertyDescriptor) => new( + env, callbackInfo, (propertyDescriptor) => new( propertyDescriptor.Name, propertyDescriptor.Method!, propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); + propertyDescriptor.ModuleHolder)); } #if UNMANAGED_DELEGATES @@ -1284,11 +1235,11 @@ private static unsafe napi_value InvokeJSMethod(napi_env env, napi_callback_info private static unsafe napi_value InvokeJSGetter(napi_env env, napi_callback_info callbackInfo) { return InvokeCallback( - env, callbackInfo, JSValueScopeType.Callback, (propertyDescriptor) => new( + env, callbackInfo, (propertyDescriptor) => new( propertyDescriptor.Name, propertyDescriptor.Getter!, propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); + propertyDescriptor.ModuleHolder)); } #if UNMANAGED_DELEGATES @@ -1297,75 +1248,27 @@ private static unsafe napi_value InvokeJSGetter(napi_env env, napi_callback_info private static napi_value InvokeJSSetter(napi_env env, napi_callback_info callbackInfo) { return InvokeCallback( - env, callbackInfo, JSValueScopeType.Callback, (propertyDescriptor) => new( + env, callbackInfo, (propertyDescriptor) => new( propertyDescriptor.Name, propertyDescriptor.Setter!, propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); - } - -#if UNMANAGED_DELEGATES - [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] -#endif - internal static unsafe napi_value InvokeJSCallbackNoContext( - napi_env env, napi_callback_info callbackInfo) - { - return InvokeCallback( - env, callbackInfo, JSValueScopeType.NoContext, (descriptor) => descriptor); - } - -#if UNMANAGED_DELEGATES - [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] -#endif - private static unsafe napi_value InvokeJSMethodNoContext(napi_env env, napi_callback_info callbackInfo) - { - return InvokeCallback( - env, callbackInfo, JSValueScopeType.NoContext, (propertyDescriptor) => new( - propertyDescriptor.Name, - propertyDescriptor.Method!, - propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); - } - -#if UNMANAGED_DELEGATES - [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] -#endif - private static unsafe napi_value InvokeJSGetterNoContext(napi_env env, napi_callback_info callbackInfo) - { - return InvokeCallback( - env, callbackInfo, JSValueScopeType.NoContext, (propertyDescriptor) => new( - propertyDescriptor.Name, - propertyDescriptor.Getter!, - propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); - } - -#if UNMANAGED_DELEGATES - [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] -#endif - private static napi_value InvokeJSSetterNoContext(napi_env env, napi_callback_info callbackInfo) - { - return InvokeCallback( - env, callbackInfo, JSValueScopeType.NoContext, (propertyDescriptor) => new( - propertyDescriptor.Name, - propertyDescriptor.Setter!, - propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); + propertyDescriptor.ModuleHolder)); } private static unsafe napi_value InvokeCallback( napi_env env, napi_callback_info callbackInfo, - JSValueScopeType scopeType, Func getCallbackDescriptor) { - using var scope = new JSValueScope(scopeType, env, runtime: default); + // The scope references the context inherited from the parent scope, or -- when the native + // host dispatches a callback with no scope on the thread -- recovered from env instance data. + using var scope = JSValueScope.CreateRuntimeScope(env); try { JSCallbackArgs.GetDataAndLength(scope, callbackInfo, out object? data, out int length); Span args = stackalloc napi_value[length]; JSCallbackDescriptor descriptor = getCallbackDescriptor((TDescriptor)data!); - scope.ModuleContext = descriptor.ModuleContext; + scope.ModuleHolder = descriptor.ModuleHolder; return (napi_value)descriptor.Callback( new JSCallbackArgs(scope, callbackInfo, args, descriptor.Data)); } @@ -1394,31 +1297,6 @@ internal static unsafe void FinalizeGCHandle(napi_env env, nint data, nint hint) } } -#if UNMANAGED_DELEGATES - [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] -#endif - internal static unsafe void FinalizeGCHandleToDisposable(napi_env env, nint data, nint hint) - { - GCHandle handle = GCHandle.FromIntPtr(data); - try - { - (handle.Target as IDisposable)?.Dispose(); - } - finally - { - if (hint != default) - { - GCHandle contextHandle = GCHandle.FromIntPtr(hint); - JSRuntimeContext context = (JSRuntimeContext)contextHandle.Target!; - context.FreeGCHandle(handle); - } - else - { - handle.Free(); - } - } - } - #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif @@ -1450,7 +1328,7 @@ private static unsafe void CallFinalizeAction(napi_env env, nint data, nint hint { // TODO: [vmoroz] In future we will be not allowed to run JS in finalizers. // We must remove creation of the scope. - using var scope = new JSValueScope(JSValueScopeType.Callback); + using var scope = JSValueScope.CreateRuntimeScope(env, context); ((Action)gcHandle.Target!)(); } finally @@ -1654,22 +1532,9 @@ private static unsafe nint[] ToUnmanagedPropertyDescriptors( IReadOnlyCollection descriptors, UseUnmanagedDescriptors action) { - napi_callback methodCallback; - napi_callback getterCallback; - napi_callback setterCallback; - if (JSValueScope.Current?.ScopeType == JSValueScopeType.NoContext) - { - // The NativeHost and ManagedHost set up callbacks without a current module context. - methodCallback = new napi_callback(s_invokeJSMethodNC); - getterCallback = new napi_callback(s_invokeJSGetterNC); - setterCallback = new napi_callback(s_invokeJSSetterNC); - } - else - { - methodCallback = new napi_callback(s_invokeJSMethod); - getterCallback = new napi_callback(s_invokeJSGetter); - setterCallback = new napi_callback(s_invokeJSSetter); - } + napi_callback methodCallback = new(s_invokeJSMethod); + napi_callback getterCallback = new(s_invokeJSGetter); + napi_callback setterCallback = new(s_invokeJSSetter); nint[] handlesToFinalize = new nint[descriptors.Count]; int count = descriptors.Count; diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 2aabc8e2..779aa057 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Threading; using Microsoft.JavaScript.NodeApi.Interop; @@ -13,47 +14,23 @@ namespace Microsoft.JavaScript.NodeApi; /// /// Indicates the type of within the hierarchy of scopes. /// -public enum JSValueScopeType +internal enum JSValueScopeType { /// - /// A limited scope without any or . - /// Used by the Node API .NET native host to set up callbacks before the managed host is - /// initialized. + /// References a and marks the call/context boundary for a JS + /// environment. Opens no napi handle scope; it is the scope a falls back + /// to for validity when no handle scope is open. /// - NoContext, + RuntimeContext, /// - /// A parent scope shared by all (non-AOT) .NET modules loaded in the same process. It has - /// a but no . - /// - /// - /// AOT modules do not have any root scope, so each module scope has a separate - /// . - /// - Root, - - /// - /// A scope specific to each module. It inherits the from the root - /// scope, and has a unique . - /// - /// - /// AOT modules do not have any root scope, so each module also has a separate - /// . - /// - Module, - - /// - /// Callback scope within a module; inherits context from the module. - /// - Callback, - - /// - /// Handle scope within a callback; inherits context from the module. + /// Opens a napi handle scope nested within a parent scope, from which it inherits the context. /// Handle, /// - /// Escapable handle scope within a callback; inherits context from the module. + /// Opens an escapable napi handle scope nested within a parent scope, and can escape one value + /// to the parent scope. /// Escapable, } @@ -77,7 +54,7 @@ public sealed class JSValueScope : IDisposable private readonly SynchronizationContext? _previousSyncContext; private readonly nint _scopeHandle; - public JSValueScopeType ScopeType { get; } + internal JSValueScopeType ScopeType { get; } /// /// Gets the current JS value scope. @@ -147,171 +124,113 @@ public static explicit operator napi_env(JSValueScope scope) internal static JSRuntime CurrentRuntime => Current.Runtime; internal static JSRuntimeContext? CurrentRuntimeContext => CurrentOrNull?.RuntimeContext; - public JSModuleContext? ModuleContext { get; internal set; } + /// + /// Holds the instance of the module class for the current module. It is a shared mutable cell + /// so callback descriptors can capture it during initialization, before the module instance + /// exists, and observe the instance once it is assigned. + /// + internal StrongBox? ModuleHolder { get; set; } /// - /// Creates a new instance of a with a specified scope type. + /// Gets the instance of the module class for the current module, used as the 'this' argument + /// for module-level instance members, or null if there is no module class. /// - /// The type of scope to create; default is - /// . - public JSValueScope(JSValueScopeType scopeType = JSValueScopeType.Handle) - : this(scopeType, env: default, runtime: default) - { - } + public object? Module => ModuleHolder?.Value; /// - /// Creates a new instance of a , which may be a parentless scope - /// with initial environment handle and JS runtime. + /// Creates a scope that references a and marks the call/context + /// boundary for a JS environment. It opens no napi handle scope. /// - /// The type of scope to create. - /// JS environment handle, required only for creating a scope - /// without a parent, otherwise the environment is inherited from the parent scope. - /// JS runtime interface, required only for creating a scope - /// without a parent, otherwise the JS runtime is inherited from the parent scope. - /// Optional synchronization context to use for async - /// operations; if omitted then a default synchronization context is used. - public JSValueScope( - JSValueScopeType scopeType, - napi_env env, - JSRuntime? runtime, - JSSynchronizationContext? synchronizationContext = null) - { - ScopeType = scopeType; + /// The JS environment handle. + /// The runtime context to reference. When null it is inherited from the + /// parent scope, or recovered from the environment instance data. + public static JSValueScope CreateRuntimeScope( + napi_env env = default, JSRuntimeContext? context = null) + => new(env, context); - if (scopeType == JSValueScopeType.NoContext) - { - // A NoContext scope can inherit the env from a parent NoContext scope. - _parentScope = CurrentOrNull; - if (_parentScope != null && _parentScope.ScopeType != JSValueScopeType.NoContext) - { - throw new InvalidOperationException( - "A NoContext scope cannot be created within another type of scope."); - } + /// + /// Creates a napi handle scope nested within the current scope. JS values created within it + /// are released when it is disposed, unless held by a . + /// + public static JSValueScope CreateHandleScope() => new(JSValueScopeType.Handle); - if (env.IsNull) - { - env = _parentScope?._env ?? - throw new ArgumentNullException(nameof(env), "An environment is required."); - } + /// + /// Creates an escapable napi handle scope nested within the current scope. One value may be + /// promoted to the parent scope with . + /// + public static JSValueScope CreateEscapableScope() => new(JSValueScopeType.Escapable); - runtime ??= _parentScope?.Runtime ?? - throw new ArgumentNullException(nameof(runtime), "A runtime is required."); + /// + /// Creates a scope that references an existing + /// (it never creates one). + /// + private JSValueScope(napi_env env, JSRuntimeContext? context) + { + ScopeType = JSValueScopeType.RuntimeContext; + _parentScope = CurrentOrNull; - _env = env; - ThreadId = Environment.CurrentManagedThreadId; - Runtime = runtime; - } - else if (scopeType == JSValueScopeType.Root) + if (context != null) { - _parentScope = CurrentOrNull; - if (_parentScope != null) - { - if (_parentScope.ScopeType == JSValueScopeType.Root) - { - // When there are multiple instances of the managed host in a process - // (created by separate workers), they do not inherit scope. - _parentScope = null; - } - else - { - throw new InvalidOperationException( - "A Root scope cannot be created within another scope."); - } - } - - if (env.IsNull) - { - throw new ArgumentNullException( - nameof(env), "An environment is required for a root scope."); - } - else if (runtime == null) + // An explicit context is a root boundary (host, AOT module, or embedding). + if (!env.IsNull && env != context.UncheckedEnvironmentHandle) { - throw new ArgumentNullException( - nameof(runtime), "A runtime is required for a root scope."); + throw new ArgumentException( + "Environment does not match the runtime context.", nameof(env)); } - - _env = env; - ThreadId = Environment.CurrentManagedThreadId; - Runtime = runtime; } else { - _parentScope = CurrentOrNull; + // Inherit the parent scope's context, else recover it from the env instance data. + context = _parentScope?.RuntimeContext + ?? JSRuntimeContext.FromEnv(env) + ?? throw new InvalidOperationException( + "A runtime context could not be resolved for the scope."); + } - if (scopeType == JSValueScopeType.Module && - _parentScope != null && _parentScope.ScopeType == JSValueScopeType.Module) - { - // When there are multiple AOT modules in a process, they do not inherit scope. - _parentScope = null; - } + _env = context.UncheckedEnvironmentHandle; + ThreadId = Environment.CurrentManagedThreadId; + Runtime = context.Runtime; + ModuleHolder = new StrongBox(); - if (_parentScope == null) - { - // Module scopes may be created without a parent scope (for AOT modules). - if (scopeType != JSValueScopeType.Module) - { - throw new InvalidOperationException( - $"A {scopeType} scope cannot be created without a parent scope."); - } - - // AOT module scopes are constructed with an env parameter - // but without a pre-initialized runtime. - _env = env.IsNull ? throw new ArgumentNullException(nameof(env)) : env; - ThreadId = Environment.CurrentManagedThreadId; - Runtime = runtime ?? new NodejsRuntime(); - } - else if (_parentScope.IsDisposed) - { - // This should never happen because disposing a scope removes it from - // s_currentScope (which is used to initialize _parentScope above). - throw new InvalidOperationException("Parent scope is disposed."); - } - else if (scopeType == JSValueScopeType.Callback && - _parentScope.ScopeType != JSValueScopeType.Callback && - _parentScope.ScopeType != JSValueScopeType.Module && - _parentScope.ScopeType != JSValueScopeType.Root && - _parentScope.ScopeType != JSValueScopeType.NoContext) - { - throw new InvalidOperationException( - $"A Callback scope must be created within a Root, Module, or Callback scope. " + - $"Current scope: {scopeType}"); - } - else if (!env.IsNull && env != _parentScope._env) - { - throw new ArgumentException( - "Environment must not be provided for a non-root scope.", - nameof(env)); - } - else if (runtime != null && runtime != _parentScope.Runtime) - { - throw new ArgumentException( - "Runtime must not be provided for a non-root scope.", - nameof(runtime)); - } - else - { - _parentScope.ThrowIfInvalidThreadAccess(); - _env = _parentScope._env; - ThreadId = _parentScope.ThreadId; - Runtime = _parentScope.Runtime; - } + JSValueScope? previousScope = CurrentOrNull; + try + { + CurrentOrNull = this; + RuntimeContext = context; + RuntimeContextHandle = context.ContextHandle; - if (scopeType == JSValueScopeType.Module) - { - if (_parentScope?.ModuleContext != null) - { - throw new InvalidOperationException("Module scope cannot be nested."); - } + _previousSyncContext = SynchronizationContext.Current; + SynchronizationContext.SetSynchronizationContext(context.SynchronizationContext); + } + catch (Exception) + { + CurrentOrNull = previousScope; + throw; + } + } - ModuleContext = new JSModuleContext(); - } - else - { - ModuleContext = _parentScope!.ModuleContext; - } + /// + /// Creates a or + /// scope that opens a napi handle scope nested within the current scope. + /// + private JSValueScope(JSValueScopeType scopeType) + { + ScopeType = scopeType; + _parentScope = CurrentOrNull ?? throw new InvalidOperationException( + $"A {scopeType} scope cannot be created without a parent scope."); + + if (_parentScope.IsDisposed) + { + throw new InvalidOperationException("Parent scope is disposed."); } - _scopeHandle = ScopeType switch + _parentScope.ThrowIfInvalidThreadAccess(); + _env = _parentScope._env; + ThreadId = _parentScope.ThreadId; + Runtime = _parentScope.Runtime; + ModuleHolder = _parentScope.ModuleHolder; + + _scopeHandle = scopeType switch { JSValueScopeType.Handle => Runtime.OpenHandleScope(_env, out napi_handle_scope handleScope) @@ -320,39 +239,16 @@ public JSValueScope( => Runtime.OpenEscapableHandleScope( _env, out napi_escapable_handle_scope handleScope) .ThrowIfFailed(handleScope).Handle, - _ => default, + _ => throw new ArgumentException( + $"Invalid handle scope type: {scopeType}", nameof(scopeType)), }; JSValueScope? previousScope = CurrentOrNull; try { CurrentOrNull = this; - - if (scopeType == JSValueScopeType.NoContext) - { - // NoContext scopes do not have a runtime context. - RuntimeContext = null!; - RuntimeContextHandle = default; - } - else if (_parentScope?.RuntimeContext != null) - { - // Nested scopes inherit the runtime context from the parent scope. - RuntimeContext = _parentScope.RuntimeContext; - RuntimeContextHandle = _parentScope.RuntimeContextHandle; - } - else - { - // Unparented scopes initialize a new runtime context. - RuntimeContext = new JSRuntimeContext(env, Runtime, synchronizationContext); - RuntimeContextHandle = (nint)GCHandle.Alloc(RuntimeContext); - } - - if (scopeType == JSValueScopeType.Root || scopeType == JSValueScopeType.Callback) - { - _previousSyncContext = SynchronizationContext.Current; - SynchronizationContext.SetSynchronizationContext( - RuntimeContext.SynchronizationContext); - } + RuntimeContext = _parentScope.RuntimeContext; + RuntimeContextHandle = _parentScope.RuntimeContextHandle; } catch (Exception) { @@ -366,24 +262,21 @@ public void Dispose() if (IsDisposed) return; IsDisposed = true; - if (ScopeType != JSValueScopeType.NoContext) - { - napi_env env = RuntimeContext.EnvironmentHandle; + napi_env env = RuntimeContext.EnvironmentHandle; - switch (ScopeType) - { - case JSValueScopeType.Handle: - Runtime.CloseHandleScope( - env, new napi_handle_scope(_scopeHandle)).ThrowIfFailed(); - break; - case JSValueScopeType.Escapable: - Runtime.CloseEscapableHandleScope( - env, new napi_escapable_handle_scope(_scopeHandle)).ThrowIfFailed(); - break; - default: - SynchronizationContext.SetSynchronizationContext(_previousSyncContext); - break; - } + switch (ScopeType) + { + case JSValueScopeType.Handle: + Runtime.CloseHandleScope( + env, new napi_handle_scope(_scopeHandle)).ThrowIfFailed(); + break; + case JSValueScopeType.Escapable: + Runtime.CloseEscapableHandleScope( + env, new napi_escapable_handle_scope(_scopeHandle)).ThrowIfFailed(); + break; + default: + SynchronizationContext.SetSynchronizationContext(_previousSyncContext); + break; } CurrentOrNull = _parentScope; diff --git a/src/NodeApi/NodeApi.csproj b/src/NodeApi/NodeApi.csproj index b6aa76a6..c07735e1 100644 --- a/src/NodeApi/NodeApi.csproj +++ b/src/NodeApi/NodeApi.csproj @@ -23,6 +23,13 @@ true + + + + + + + diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index 7c9b74d1..69a53e1b 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -9,6 +9,7 @@ namespace Microsoft.JavaScript.NodeApi.Runtime; using System.Runtime.CompilerServices; #endif using System.Runtime.InteropServices; +using Microsoft.JavaScript.NodeApi.Interop; using static JSRuntime; using static NodejsRuntime; @@ -362,7 +363,8 @@ internal static unsafe void RuntimePreloadCallbackAdapter( napi_value process, napi_value require) { - using var jsValueScope = new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + JSRuntimeContext context = new(env, JSRuntime); + using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { var callback = (PreloadCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -386,7 +388,8 @@ internal static unsafe napi_value RuntimeLoadingCallbackAdapter( napi_value require, napi_value run_cjs) { - using var jsValueScope = new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + JSRuntimeContext context = new(env, JSRuntime); + using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { var callback = (LoadingCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -410,7 +413,8 @@ internal static unsafe void RuntimeLoadedCallbackAdapter( napi_env env, napi_value loading_result) { - using var jsValueScope = new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + JSRuntimeContext context = new(env, JSRuntime); + using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { var callback = (LoadedCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -433,7 +437,8 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( nint module_name, napi_value exports) { - using var jsValueScope = new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + JSRuntimeContext context = new(env, JSRuntime); + using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { var callback = (InitializeModuleCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -501,7 +506,8 @@ internal static unsafe NodeEmbeddingStatus TaskPostCallbackAdapter( #endif internal static unsafe void NodeApiRunCallbackAdapter(nint cb_data, napi_env env) { - using var jsValueScope = new JSValueScope(JSValueScopeType.Root, env, JSRuntime); + JSRuntimeContext context = new(env, JSRuntime); + using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { var callback = (RunNodeApiCallback)GCHandle.FromIntPtr(cb_data).Target!; diff --git a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs index 6513384b..eca34635 100644 --- a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs +++ b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs @@ -4,6 +4,7 @@ namespace Microsoft.JavaScript.NodeApi.Runtime; using System; +using Microsoft.JavaScript.NodeApi.Interop; using static JSRuntime; using static NodejsRuntime; @@ -19,8 +20,8 @@ public NodeEmbeddingNodeApiScope(NodeEmbeddingRuntime runtime) NodeEmbedding.JSRuntime.EmbeddingRuntimeOpenNodeApiScope( runtime.Handle, out _nodeApiScope, out napi_env env) .ThrowIfFailed(); - _valueScope = new JSValueScope( - JSValueScopeType.Root, env, NodeEmbedding.JSRuntime); + JSRuntimeContext context = new(env, NodeEmbedding.JSRuntime); + _valueScope = JSValueScope.CreateRuntimeScope(env, context); } /// diff --git a/src/NodeApi/Runtime/TracingJSRuntime.cs b/src/NodeApi/Runtime/TracingJSRuntime.cs index e78de8df..4a6f0762 100644 --- a/src/NodeApi/Runtime/TracingJSRuntime.cs +++ b/src/NodeApi/Runtime/TracingJSRuntime.cs @@ -377,12 +377,17 @@ private static readonly unsafe delegate* unmanaged[Cdecl] s_traceSetterCallback = &TraceSetterCallback; #endif + // Like InvokeCallback (which these replace when tracing is on), the scope references the context + // inherited from the parent scope, or recovered from env instance data when there is none. + private static JSValueScope CreateCallbackScope(napi_env env) + => JSValueScope.CreateRuntimeScope(env); + #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif private static unsafe napi_value TraceFunctionCallback(napi_env env, napi_callback_info cbinfo) { - using var scope = new JSValueScope(JSValueScopeType.Callback); + using JSValueScope scope = CreateCallbackScope(env); return ((TracingJSRuntime)scope.Runtime).TraceCallback( scope, cbinfo, (descriptor) => descriptor); } @@ -392,13 +397,13 @@ private static unsafe napi_value TraceFunctionCallback(napi_env env, napi_callba #endif private static unsafe napi_value TraceMethodCallback(napi_env env, napi_callback_info cbinfo) { - using var scope = new JSValueScope(JSValueScopeType.Callback); + using JSValueScope scope = CreateCallbackScope(env); return ((TracingJSRuntime)scope.Runtime).TraceCallback( scope, cbinfo, (propertyDescriptor) => new( propertyDescriptor.Name, propertyDescriptor.Method!, propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); + propertyDescriptor.ModuleHolder)); } #if UNMANAGED_DELEGATES @@ -406,13 +411,13 @@ private static unsafe napi_value TraceMethodCallback(napi_env env, napi_callback #endif private static unsafe napi_value TraceGetterCallback(napi_env env, napi_callback_info cbinfo) { - using var scope = new JSValueScope(JSValueScopeType.Callback); + using JSValueScope scope = CreateCallbackScope(env); return ((TracingJSRuntime)scope.Runtime).TraceCallback( scope, cbinfo, (propertyDescriptor) => new( propertyDescriptor.Name, propertyDescriptor.Getter!, propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); + propertyDescriptor.ModuleHolder)); } #if UNMANAGED_DELEGATES @@ -420,13 +425,13 @@ private static unsafe napi_value TraceGetterCallback(napi_env env, napi_callback #endif private static unsafe napi_value TraceSetterCallback(napi_env env, napi_callback_info cbinfo) { - using var scope = new JSValueScope(JSValueScopeType.Callback); + using JSValueScope scope = CreateCallbackScope(env); return ((TracingJSRuntime)scope.Runtime).TraceCallback( scope, cbinfo, (propertyDescriptor) => new( propertyDescriptor.Name, propertyDescriptor.Setter!, propertyDescriptor.Data, - propertyDescriptor.ModuleContext)); + propertyDescriptor.ModuleHolder)); } /// diff --git a/test/GCTests.cs b/test/GCTests.cs index dc1b4f04..b6a0765f 100644 --- a/test/GCTests.cs +++ b/test/GCTests.cs @@ -44,7 +44,7 @@ public void GCHandles() // - JSPropertyDescriptor: DotnetClass.toString Assert.Equal(3 + 5, JSRuntimeContext.Current.GCHandleCount); - using JSValueScope innerScope = new(JSValueScopeType.Callback); + using JSValueScope innerScope = JSValueScope.CreateRuntimeScope(); jsCreateInstanceFunction.CallAsStatic(dotnetClass); // Two more handles should have been allocated by the JS create-instance function call. @@ -93,7 +93,7 @@ public void GCObjects() Assert.Equal(8, JSRuntimeContext.Current.GCHandleCount); - using (JSValueScope innerScope = new(JSValueScopeType.Callback)) + using (JSValueScope innerScope = JSValueScope.CreateRuntimeScope()) { jsCreateInstanceFunction.CallAsStatic(dotnetClass); } diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index ed994865..49cb89a8 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -13,20 +13,20 @@ public class JSReferenceTests { private readonly MockJSRuntime _mockRuntime = new(); - private JSValueScope TestScope(JSValueScopeType scopeType) - => TestScope(scopeType, new MockJSRuntime.SynchronizationContext()); + private JSValueScope TestScope() + => TestScope(new MockJSRuntime.SynchronizationContext()); - private JSValueScope TestScope( - JSValueScopeType scopeType, JSSynchronizationContext synchronizationContext) + private JSValueScope TestScope(JSSynchronizationContext synchronizationContext) { napi_env env = new(Environment.CurrentManagedThreadId); - return new(scopeType, env, _mockRuntime, synchronizationContext); + var context = new JSRuntimeContext(env, _mockRuntime, synchronizationContext); + return JSValueScope.CreateRuntimeScope(env, context); } [Fact] public void GetReferenceFromSameScope() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); JSValue value = JSValue.CreateObject(); JSReference reference = new(value); @@ -36,10 +36,10 @@ public void GetReferenceFromSameScope() [Fact] public void GetReferenceFromParentScope() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); JSReference reference; - using (JSValueScope handleScope = new(JSValueScopeType.Handle)) + using (JSValueScope handleScope = JSValueScope.CreateHandleScope()) { JSValue value = JSValue.CreateObject(); reference = new JSReference(value); @@ -51,7 +51,7 @@ public void GetReferenceFromParentScope() [Fact] public void GetReferenceFromDifferentThread() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); JSValue value = JSValue.CreateObject(); JSReference reference = new(value); @@ -66,7 +66,7 @@ public void GetReferenceFromDifferentThread() [Fact] public void GetReferenceFromDifferentRootScope() { - using JSValueScope rootScope1 = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope1 = TestScope(); JSValue value = JSValue.CreateObject(); JSReference reference = new(value); @@ -74,7 +74,7 @@ public void GetReferenceFromDifferentRootScope() // Run in a new thread and establish another root scope there. TestUtils.RunInThread(() => { - using JSValueScope rootScope2 = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope2 = TestScope(); Assert.Throws(() => reference.GetValue()); }).Wait(); } @@ -82,7 +82,7 @@ public void GetReferenceFromDifferentRootScope() [Fact] public void GetWeakReferenceUnavailable() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); JSValue value = JSValue.CreateObject(); var reference = new JSReference(value, isWeak: true); @@ -94,7 +94,7 @@ public void GetWeakReferenceUnavailable() [Fact] public void TryGetWeakReferenceValue() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); JSValue value = JSValue.CreateObject(); JSReference reference = new(value); @@ -105,7 +105,7 @@ public void TryGetWeakReferenceValue() [Fact] public void TryGetWeakReferenceUnavailable() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); JSValue value = JSValue.CreateObject(); var reference = new JSReference(value, isWeak: true); @@ -114,25 +114,6 @@ public void TryGetWeakReferenceUnavailable() Assert.False(reference.TryGetValue(out _)); } - // A reference created from a NoContext scope (as the native host does) has a null runtime - // context, so its finalizer takes the branch that previously asserted thread access. The GC - // finalizer runs on a thread with no JS scope, so that assertion threw - // JSInvalidThreadAccessException out of the finalizer, which terminates the process (the - // reported worker-teardown crash). The finalizer must instead complete without throwing. - [Fact] - public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow() - { - using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); - - JSValue value = JSValue.CreateObject(); - var reference = new FinalizerTestReference(value); - - // Run on a new thread that has no current scope, simulating the GC finalizer thread. - TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); - - Assert.True(reference.IsDisposed); - } - // A reference with a runtime context posts its cleanup to the JS thread instead of deleting it // inline. The finalizer must never throw when it runs on a thread with no current scope, and // the posted delete must actually release the native reference once the JS thread pumps it. @@ -140,7 +121,7 @@ public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow() public void FinalizeContextReferenceFromDifferentThreadDoesNotThrow() { var syncContext = new MockJSRuntime.RecordingSynchronizationContext(); - using JSValueScope rootScope = TestScope(JSValueScopeType.Root, syncContext); + using JSValueScope rootScope = TestScope(syncContext); JSValue value = JSValue.CreateObject(); var reference = new FinalizerTestReference(value); @@ -160,20 +141,31 @@ public void FinalizeContextReferenceFromDifferentThreadDoesNotThrow() Assert.False(_mockRuntime.HasReference(handle)); } - // Explicit disposal (disposing: true) preserves the documented behavior of asserting thread - // access for a no-context reference; only the finalizer path is made non-throwing. + // Explicit Dispose() from a thread with no current scope must not throw. The pre-refactor + // no-context path asserted thread access and threw JSInvalidThreadAccessException here; every + // reference is now context-backed, so the delete is posted to the JS thread instead. [Fact] - public void DisposeNoContextReferenceFromDifferentThreadThrows() + public void DisposeReferenceFromDifferentThreadPostsDelete() { - using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); + var syncContext = new MockJSRuntime.RecordingSynchronizationContext(); + using JSValueScope rootScope = TestScope(syncContext); JSValue value = JSValue.CreateObject(); - JSReference reference = new(value); + var reference = new JSReference(value); + napi_ref handle = reference.Handle; + Assert.True(_mockRuntime.HasReference(handle)); - TestUtils.RunInThread(() => - { - Assert.Throws(() => reference.Dispose()); - }).Wait(); + TestUtils.RunInThread(() => reference.Dispose()).Wait(); + + Assert.True(reference.IsDisposed); + + // The delete is deferred to the JS thread, not run inline on the disposing thread. + Assert.True(_mockRuntime.HasReference(handle)); + Assert.Equal(1, syncContext.PendingCount); + + // Pumping the sync context runs the posted delete, releasing the native reference. + Assert.Equal(1, syncContext.RunPendingCallbacks()); + Assert.False(_mockRuntime.HasReference(handle)); } // The finalizer invokes the virtual Dispose(bool), so a derived override can throw before or @@ -184,7 +176,7 @@ public void DisposeNoContextReferenceFromDifferentThreadThrows() [Fact] public void FinalizerSwallowsExceptionsFromDerivedDisposeOverride() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestScope(); CreateAndAbandonThrowingReference(); diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 201d2608..8a2e180b 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -2,6 +2,8 @@ // Licensed under the MIT License. using System; +using System.Runtime.CompilerServices; +using Microsoft.JavaScript.NodeApi.Interop; using Xunit; using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; @@ -16,272 +18,97 @@ public class JSValueScopeTests { private readonly MockJSRuntime _mockRuntime = new(); - private JSValueScope TestScope(JSValueScopeType scopeType) + private JSValueScope TestRuntimeScope() { napi_env env = new(Environment.CurrentManagedThreadId); - return new(scopeType, env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + return JSValueScope.CreateRuntimeScope(env, context); } [Fact] - public void CreateNoContextScope() + public void CreateRuntimeScope() { - using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); - Assert.Null(noContextScope.RuntimeContext); - Assert.Equal(JSValueScopeType.NoContext, JSValueScope.Current.ScopeType); + using JSValueScope runtimeScope = TestRuntimeScope(); + Assert.NotNull(runtimeScope.RuntimeContext); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } [Fact] - public void CreateRootScope() + public void CreateNestedRuntimeScope() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); - Assert.NotNull(rootScope.RuntimeContext); - Assert.Equal(JSValueScopeType.Root, JSValueScope.Current.ScopeType); - } - - [Fact] - public void CreateModuleScopeWithinNoContextScope() - { - using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); - - using (JSValueScope moduleScope = TestScope(JSValueScopeType.Module)) - { - Assert.NotNull(moduleScope.RuntimeContext); - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - } - - Assert.Equal(JSValueScopeType.NoContext, JSValueScope.Current.ScopeType); - } - - [Fact] - public void CreateModuleScopeWithinRootScope() - { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); - - using (JSValueScope moduleScope = new(JSValueScopeType.Module)) - { - Assert.NotNull(moduleScope.RuntimeContext); - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - } - - Assert.Equal(JSValueScopeType.Root, JSValueScope.Current.ScopeType); - } - - [Fact] - public void CreateModuleScopeWithoutRoot() - { - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); - Assert.NotNull(moduleScope.RuntimeContext); - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - } - - [Fact] - public void CreateCallbackScope() - { - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); - - using (JSValueScope callbackScope = new(JSValueScopeType.Callback)) - { - Assert.NotNull(moduleScope.RuntimeContext); - Assert.Equal(JSValueScopeType.Callback, JSValueScope.Current.ScopeType); - } - - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - } - - [Fact] - public void CreateHandleScopeWithinRoot() - { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope runtimeScope = TestRuntimeScope(); - using (JSValueScope handleScope = new(JSValueScopeType.Handle)) + using (JSValueScope nestedScope = JSValueScope.CreateRuntimeScope()) { - Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); + Assert.Same(runtimeScope.RuntimeContext, nestedScope.RuntimeContext); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } - Assert.Equal(JSValueScopeType.Root, JSValueScope.Current.ScopeType); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } [Fact] - public void CreateHandleScopeWithinModule() + public void CreateHandleScopeWithinRuntimeScope() { - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); + using JSValueScope runtimeScope = TestRuntimeScope(); - using (JSValueScope handleScope = new(JSValueScopeType.Handle)) + using (JSValueScope handleScope = JSValueScope.CreateHandleScope()) { Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); } - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } [Fact] - public void CreateHandleScopeWithinCallback() + public void CreateHandleScopeWithinNestedRuntimeScope() { - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); + using JSValueScope runtimeScope = TestRuntimeScope(); - using (JSValueScope callbackScope = new(JSValueScopeType.Callback)) + using (JSValueScope nestedScope = JSValueScope.CreateRuntimeScope()) { - using (JSValueScope handleScope = new(JSValueScopeType.Handle)) + using (JSValueScope handleScope = JSValueScope.CreateHandleScope()) { Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); } - Assert.Equal(JSValueScopeType.Callback, JSValueScope.Current.ScopeType); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } [Fact] - public void CreateEscapableScopeWithinCallback() + public void CreateEscapableScopeWithinRuntimeScope() { - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); + using JSValueScope runtimeScope = TestRuntimeScope(); - using (JSValueScope callbackScope = new(JSValueScopeType.Callback)) + using (JSValueScope escapableScope = JSValueScope.CreateEscapableScope()) { - using (JSValueScope escapableScope = new(JSValueScopeType.Escapable)) - { - Assert.Equal(JSValueScopeType.Escapable, JSValueScope.Current.ScopeType); - } - - Assert.Equal(JSValueScopeType.Callback, JSValueScope.Current.ScopeType); + Assert.Equal(JSValueScopeType.Escapable, JSValueScope.Current.ScopeType); } - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - } - - [Fact] - public void InvalidNoContextScopeNesting() - { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); - Assert.Throws(() => - { - using JSValueScope noContextScope = new(JSValueScopeType.NoContext); - }); - Assert.Equal(JSValueScopeType.Root, JSValueScope.Current.ScopeType); - - using JSValueScope moduleScope = new(JSValueScopeType.Module); - Assert.Throws(() => - { - using JSValueScope noContextScope = new(JSValueScopeType.NoContext); - }); - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - - using JSValueScope callbackScope = new(JSValueScopeType.Callback); - Assert.Throws(() => - { - using JSValueScope noContextScope = new(JSValueScopeType.NoContext); - }); - Assert.Equal(JSValueScopeType.Callback, JSValueScope.Current.ScopeType); - - using JSValueScope handleScope = new(JSValueScopeType.Handle); - Assert.Throws(() => - { - using JSValueScope noContextScope = new(JSValueScopeType.NoContext); - }); - Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); - - using JSValueScope escapableScope = new(JSValueScopeType.Escapable); - Assert.Throws(() => - { - using JSValueScope noContextScope = new(JSValueScopeType.NoContext); - }); - Assert.Equal(JSValueScopeType.Escapable, JSValueScope.Current.ScopeType); - } - - [Fact] - public void InvalidRootContextScopeNesting() - { - using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); - Assert.Throws(() => - { - using JSValueScope rootScope = new(JSValueScopeType.Root); - }); - Assert.Equal(JSValueScopeType.NoContext, JSValueScope.Current.ScopeType); - - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); - Assert.Throws(() => - { - using JSValueScope rootScope = new(JSValueScopeType.Root); - }); - Assert.Equal(JSValueScopeType.Module, JSValueScope.Current.ScopeType); - - using JSValueScope callbackScope = new(JSValueScopeType.Callback); - Assert.Throws(() => - { - using JSValueScope rootScope = new(JSValueScopeType.Root); - }); - Assert.Equal(JSValueScopeType.Callback, JSValueScope.Current.ScopeType); - - using JSValueScope handleScope = new(JSValueScopeType.Handle); - Assert.Throws(() => - { - using JSValueScope rootScope = new(JSValueScopeType.Root); - }); - Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); - - using JSValueScope escapableScope = new(JSValueScopeType.Escapable); - Assert.Throws(() => - { - using JSValueScope rootScope = new(JSValueScopeType.Root); - }); - Assert.Equal(JSValueScopeType.Escapable, JSValueScope.Current.ScopeType); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); } [Fact] - public void InvalidModuleContextScopeNesting() + public void HandleScopeRequiresParentScope() { - using JSValueScope moduleScope = TestScope(JSValueScopeType.Module); - using JSValueScope callbackScope = new(JSValueScopeType.Callback); - Assert.Throws(() => - { - using JSValueScope nestedModuleScope = new(JSValueScopeType.Module); - }); - Assert.Equal(JSValueScopeType.Callback, JSValueScope.Current.ScopeType); - - using JSValueScope handleScope = new(JSValueScopeType.Handle); - Assert.Throws(() => - { - using JSValueScope nestedModuleScope = new(JSValueScopeType.Module); - }); - Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); - - using JSValueScope escapableScope = new(JSValueScopeType.Escapable); - Assert.Throws(() => - { - using JSValueScope nestedModuleScope = new(JSValueScopeType.Module); - }); - Assert.Equal(JSValueScopeType.Escapable, JSValueScope.Current.ScopeType); - } - - [Fact] - public void InvalidCallbackContextScopeNesting() - { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); - - using JSValueScope handleScope = new(JSValueScopeType.Handle); - Assert.Throws(() => - { - using JSValueScope callbackScope = new(JSValueScopeType.Callback); - }); - Assert.Equal(JSValueScopeType.Handle, JSValueScope.Current.ScopeType); - - using JSValueScope escapableScope = new(JSValueScopeType.Escapable); - Assert.Throws(() => - { - using JSValueScope callbackScope = new(JSValueScopeType.Callback); - }); - Assert.Equal(JSValueScopeType.Escapable, JSValueScope.Current.ScopeType); + Assert.Throws( + () => JSValueScope.CreateHandleScope()); + Assert.Throws( + () => JSValueScope.CreateEscapableScope()); } [Fact] public void AccessValueFromClosedScope() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestRuntimeScope(); JSValueScope handleScope; JSValue objectValue; - using (handleScope = new(JSValueScopeType.Handle)) + using (handleScope = JSValueScope.CreateHandleScope()) { objectValue = JSValue.CreateObject(); Assert.True(objectValue.IsObject()); @@ -296,13 +123,13 @@ public void AccessValueFromClosedScope() [Fact] public void AccessPropertyKeyFromClosedScope() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestRuntimeScope(); JSValue objectValue = JSValue.CreateObject(); JSValue propertyKey; JSValueScope handleScope; - using (handleScope = new(JSValueScopeType.Handle)) + using (handleScope = JSValueScope.CreateHandleScope()) { propertyKey = "test"; Assert.True(propertyKey.IsString()); @@ -321,7 +148,7 @@ public void AccessPropertyKeyFromClosedScope() [Fact] public void CreateValueFromDifferentThread() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestRuntimeScope(); // Run in a new thread which will not have any current scope. TestUtils.RunInThread(() => @@ -337,7 +164,7 @@ public void CreateValueFromDifferentThread() [Fact] public void AccessValueFromDifferentThread() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope = TestRuntimeScope(); JSValue objectValue = JSValue.CreateObject(); // Run in a new thread which will not have any current scope. @@ -354,18 +181,131 @@ public void AccessValueFromDifferentThread() [Fact] public void AccessValueFromDifferentRootScope() { - using JSValueScope rootScope1 = TestScope(JSValueScopeType.Root); + using JSValueScope rootScope1 = TestRuntimeScope(); JSValue objectValue = JSValue.CreateObject(); // Run in a new thread and establish another root scope there. TestUtils.RunInThread(() => { - using JSValueScope rootScope2 = TestScope(JSValueScopeType.Root); - Assert.Equal(JSValueScopeType.Root, JSValueScope.Current.ScopeType); + using JSValueScope rootScope2 = TestRuntimeScope(); + Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); JSInvalidThreadAccessException ex = Assert.Throws( () => objectValue.IsObject()); Assert.Equal(rootScope2, ex.CurrentScope); Assert.Equal(rootScope1, ex.TargetScope); }).Wait(); } + + // The module instance is captured through a shared holder: descriptors take the holder during + // initialization (before the instance exists) and observe the instance once dispatch assigns it. + // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. + [Fact] + public void ModuleInstanceRoundTripsThroughSharedHolder() + { + using JSValueScope runtimeScope = TestRuntimeScope(); + + // The runtime scope mints a holder; the module instance is not assigned yet. + StrongBox holder = JSValueScope.Current.ModuleHolder!; + Assert.NotNull(holder); + Assert.Null(JSValueScope.Current.Module); + + object moduleInstance = new(); + using (JSValueScope handleScope = JSValueScope.CreateHandleScope()) + { + // Inner scopes inherit the same holder instance. + Assert.Same(holder, JSValueScope.Current.ModuleHolder); + + // Assigning through the shared holder (as dispatch does) is visible as Current.Module. + holder.Value = moduleInstance; + Assert.Same(moduleInstance, JSValueScope.Current.Module); + } + + // The instance remains visible in the parent scope after the nested scope closes. + Assert.Same(moduleInstance, JSValueScope.Current.Module); + } + + // An escapable scope promotes one value to its parent so the value stays usable after the inner + // scope closes, while a value that was not escaped becomes invalid once the scope is disposed. + [Fact] + public void EscapableScopeEscapesValue() + { + using JSValueScope rootScope = TestRuntimeScope(); + + JSValue escaped; + JSValue notEscaped; + JSValueScope escapableScope; + using (escapableScope = JSValueScope.CreateEscapableScope()) + { + notEscaped = JSValue.CreateObject(); + escaped = escapableScope.Escape(JSValue.CreateObject()); + + Assert.True(escaped.IsObject()); + Assert.True(notEscaped.IsObject()); + } + + // The escaped value was promoted to the parent scope, so it remains usable. + Assert.True(escapableScope.IsDisposed); + Assert.True(escaped.IsObject()); + + // The value that was not escaped belonged to the now-closed scope. + JSValueScopeClosedException ex = Assert.Throws( + () => notEscaped.IsObject()); + Assert.Equal(escapableScope, ex.Scope); + } + + // With no explicit context and no parent scope, CreateRuntimeScope recovers the context from the + // env instance data (JSRuntimeContext.FromEnv) -- the path the dynamic module entry point relies + // on to resolve the context when no scope is on the thread yet. + [Fact] + public void CreateRuntimeScopeResolvesContextFromEnv() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + // The context registered itself in the env instance data, so FromEnv resolves it. + Assert.Same(context, JSRuntimeContext.FromEnv(env)); + + using JSValueScope runtimeScope = JSValueScope.CreateRuntimeScope(env); + Assert.Same(context, runtimeScope.RuntimeContext); + Assert.Same(context, JSValueScope.Current.RuntimeContext); + } + + // JSRuntimeContext.Create is the public factory used by AOT entry points and embedders. It uses + // the provided runtime, and a runtime scope over the context resolves it as the current context. + [Fact] + public void CreateRuntimeContextFactoryUsesProvidedRuntime() + { + napi_env env = new(Environment.CurrentManagedThreadId); + JSRuntimeContext context = JSRuntimeContext.Create( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + Assert.Same(_mockRuntime, context.Runtime); + Assert.False(context.IsDisposed); + + using JSValueScope runtimeScope = JSValueScope.CreateRuntimeScope(env, context); + Assert.Same(context, JSValueScope.Current.RuntimeContext); + Assert.Same(context, JSRuntimeContext.Current); + } + + // A runtime-context scope installs the context's synchronization context as the thread's current + // one for its lifetime (so await continuations marshal back to the JS thread) and restores the + // previously-current one when disposed. + [Fact] + public void RuntimeScopeInstallsAndRestoresSynchronizationContext() + { + System.Threading.SynchronizationContext? previous = + System.Threading.SynchronizationContext.Current; + + napi_env env = new(Environment.CurrentManagedThreadId); + var syncContext = new MockJSRuntime.SynchronizationContext(); + var context = new JSRuntimeContext(env, _mockRuntime, syncContext); + + using (JSValueScope runtimeScope = JSValueScope.CreateRuntimeScope(env, context)) + { + Assert.Same(syncContext, System.Threading.SynchronizationContext.Current); + } + + Assert.Same(previous, System.Threading.SynchronizationContext.Current); + } } diff --git a/test/MockJSRuntime.cs b/test/MockJSRuntime.cs index 39a84a9f..325ff4f4 100644 --- a/test/MockJSRuntime.cs +++ b/test/MockJSRuntime.cs @@ -83,6 +83,30 @@ public override napi_status CloseEscapableHandleScope( return napi_ok; } + public override napi_status EscapeHandle( + napi_env env, + napi_escapable_handle_scope scope, + napi_value escapee, + out napi_value result) + { + // Promote the value to the parent scope by mirroring it under a new handle, mimicking + // napi_escape_handle returning a new value that is valid in the outer scope. + if (!_values.TryGetValue(escapee.Handle, out MockJSValue? mockValue)) + { + result = default; + return napi_invalid_arg; + } + + nint handle = ++s_handleCounter; + _values.Add(handle, new MockJSValue + { + ValueType = mockValue.ValueType, + Value = mockValue.Value, + }); + result = new napi_value(handle); + return napi_ok; + } + public override napi_status CreateString( napi_env env, ReadOnlySpan utf16Str, out napi_value result) { diff --git a/test/TestBuilder.cs b/test/TestBuilder.cs index 047decf4..8d3ba5cb 100644 --- a/test/TestBuilder.cs +++ b/test/TestBuilder.cs @@ -178,10 +178,10 @@ public static void BuildProject( if (GetNoBuild()) return; string workingDirectory = Path.GetDirectoryName(projectFilePath)!; - if (target != "Publish") - { - WriteCurrentFrameworkGlobalJson(workingDirectory, projectFilePath); - } + + // Pin the SDK per build so a build never inherits a stale per-TFM global.json that another + // TFM's host left in the shared test-case directory. + WriteCurrentFrameworkGlobalJson(workingDirectory, projectFilePath); using StreamWriter logWriter = new(File.Open( logFilePath, FileMode.Create, FileAccess.Write, FileShare.Read)); @@ -213,22 +213,19 @@ public static void BuildProject( WorkingDirectory = workingDirectory, }; - // Prevent nested dotnet invocations from inheriting the current host path from the - // parent dotnet process, which can cause host/runtime mismatches when SDK selection - // rolls forward to a newer major version. - if (Environment.Version.Major != 4) - { - startInfo.Environment.Remove("MSBuildSDKsPath"); - startInfo.Environment.Remove("DOTNET_HOST_PATH"); - startInfo.Environment.Remove("DOTNET_ROOT"); - startInfo.Environment.Remove("DOTNET_ROOT(x86)"); - startInfo.Environment.Remove("DOTNET_ROOT_X86"); - startInfo.Environment.Remove("DOTNET_ROOT(x64)"); - startInfo.Environment.Remove("DOTNET_ROOT_X64"); - startInfo.Environment.Remove("DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR"); - startInfo.Environment.Remove("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_DIR"); - startInfo.Environment.Remove("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_VER"); - } + // Nested dotnet invocations must not inherit the parent's host/SDK resolver environment, + // or SDK roll-forward to a newer major version causes host/runtime mismatches. A .NET + // Framework (net472) test host inherits these from the outer `dotnet test` as well. + startInfo.Environment.Remove("MSBuildSDKsPath"); + startInfo.Environment.Remove("DOTNET_HOST_PATH"); + startInfo.Environment.Remove("DOTNET_ROOT"); + startInfo.Environment.Remove("DOTNET_ROOT(x86)"); + startInfo.Environment.Remove("DOTNET_ROOT_X86"); + startInfo.Environment.Remove("DOTNET_ROOT(x64)"); + startInfo.Environment.Remove("DOTNET_ROOT_X64"); + startInfo.Environment.Remove("DOTNET_MSBUILD_SDK_RESOLVER_CLI_DIR"); + startInfo.Environment.Remove("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_DIR"); + startInfo.Environment.Remove("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_VER"); logWriter.WriteLine($"dotnet {startInfo.Arguments}"); logWriter.WriteLine($"CWD={workingDirectory}"); diff --git a/test/TestCases/napi-dotnet/worker_teardown_stress.js b/test/TestCases/napi-dotnet/worker_teardown_stress.js new file mode 100644 index 00000000..07c23423 --- /dev/null +++ b/test/TestCases/napi-dotnet/worker_teardown_stress.js @@ -0,0 +1,49 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +// Stress variant of worker_teardown.js. It repeatedly creates a Worker that loads the .NET host +// (only inside the worker), waits for it to initialize, then terminates it. Each load initializes +// a native host and a managed host for that worker's environment; terminating the worker tears the +// environment down, which runs the native host's instance-data finalizer. That finalizer notifies +// the managed host to dispose its context, all without calling into JavaScript (the environment is +// being destroyed). Looping exercises that per-environment init/teardown path many times to +// surface teardown-ordering crashes or leaked references (the crash class this guards against). +// +// The workers run one at a time (each is terminated before the next is created), so this never +// holds two host instances at once. As with worker_teardown.js this validates the hosted host +// module and runs under HostedClrTests only (the name contains "worker_teardown", which also +// excludes it from NativeAotTests). + +const assert = require('assert'); +const { Worker, isMainThread, parentPort } = require('worker_threads'); + +const iterations = 8; + +if (isMainThread) { + (async () => { + for (let i = 0; i < iterations; i++) { + const worker = new Worker(__filename); + await new Promise((resolve, reject) => { + worker.once('message', (message) => { + try { + assert.strictEqual(message, 'ready'); + resolve(); + } catch (err) { + reject(err); + } + }); + worker.once('error', reject); + }); + await worker.terminate(); + } + + // Keep the process alive briefly so any teardown crash surfaces as a non-zero exit code + // instead of being skipped by an immediate process exit. + setTimeout(() => process.exit(0), 300); + })().catch((err) => { throw err; }); +} else { + // Load the native host ONLY in the worker. + const binding = require('../common').binding; + assert.ok(binding); + parentPort.postMessage('ready'); +} From c1e47c04bbe58ceb0ba1cc636f1bb2209d092874 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 14:28:19 -0700 Subject: [PATCH 02/38] Address Copilot review: scope/context correctness + per-env host teardown - JSValueScope: validate a supplied env against the resolved context on the inherited path; a nested runtime scope inherits the parent's module holder. - TracingJSRuntime: apply the descriptor's module holder to the callback scope (matching InvokeCallback) so module members work under NODE_API_TRACE_RUNTIME. - JSRuntimeContext.Dispose: dispose an already-created sync context only, never construct one during environment finalization. - ManagedHost: register as a per-env disposable annotation so its full Dispose (unsubscribing the process-wide resolve handlers) runs at environment teardown. - NativeHost: close the per-env CLR host at environment teardown; correct the process-level comments on both hosts. --- src/NodeApi.DotNetHost/ManagedHost.cs | 29 ++++++++++++++++++++----- src/NodeApi/DotNetHost/NativeHost.cs | 13 ++++++----- src/NodeApi/Interop/JSRuntimeContext.cs | 5 ++++- src/NodeApi/JSValueScope.cs | 27 +++++++++++++---------- src/NodeApi/Runtime/TracingJSRuntime.cs | 3 +++ 5 files changed, 55 insertions(+), 22 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index 98450c4c..d4b97a0c 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -232,6 +232,11 @@ public static unsafe napi_value InitializeModule( _context = context }; + // Dispose the host with its environment: as a disposable annotation on the context, the + // host's full Dispose (which unsubscribes the process-wide resolve handlers) runs when + // the context is disposed at environment teardown. Mirrors the native host. + context.SetDisposableAnnotation(host); + if (hosted) { // Root the managed host for the environment lifetime and give the native host a @@ -303,10 +308,11 @@ private static void OnEnvironmentFinalizeCore(nint addon) } /// - /// Disposes the runtime context in response to environment teardown. No JavaScript may be - /// called here; disposing the context marks it disposed (so any late cross-thread post becomes - /// a no-op) and frees its GC handles. The context's references are reclaimed by Node as the - /// environment is torn down. + /// Disposes the managed host in response to environment teardown. No JavaScript may be called + /// here; disposing the context marks it disposed (so any late cross-thread post becomes a + /// no-op), disposes the host (a disposable annotation on the context) so its process-wide + /// resolve handlers are unsubscribed, and frees the context's GC handles. The context's + /// references are reclaimed by Node as the environment is torn down. /// private void DisposeOnEnvironmentFinalize() { @@ -665,10 +671,18 @@ private JSValue RunWorker(JSCallbackArgs args) } } + private bool _isDisposed; + protected override void Dispose(bool disposing) { + if (_isDisposed) return; + _isDisposed = true; + if (disposing) { + // The context disposes this host (a disposable annotation) at teardown, so the + // re-entrant context dispose here is a guarded no-op. Unsubscribe the process-wide + // resolve handlers so a torn-down environment's host is not left rooted by them. _context?.Dispose(); _context = null; @@ -677,7 +691,12 @@ protected override void Dispose(bool disposing) #else AssemblyLoadContext.Default.Resolving -= OnResolvingAssembly; _loadContext.Resolving -= OnResolvingAssembly; - _loadContext.Unload(); + + // A non-collectible load context cannot be unloaded; only unload one created collectible. + if (_loadContext.IsCollectible) + { + _loadContext.Unload(); + } #endif } diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index a8d41fdb..f7892314 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -548,9 +548,11 @@ public void Dispose() { // Called by the host context when the environment is torn down (the NativeHost is a // disposable annotation on that context). Runs during environment finalization, where - // calling into JS is forbidden, so it only notifies the managed host (a native call) and - // drops the exports reference; the exports napi_ref is reclaimed by Node as the env dies. + // calling into JS is forbidden. Notify the managed host (a native call), then close this + // environment's CLR host, and drop the exports reference; the exports napi_ref is reclaimed + // by Node as the env dies. NotifyManagedHostEnvironmentFinalize(); + CloseRuntimeHost(); _addonGCHandle = default; _onEnvFinalize = default; _exports = null; @@ -558,9 +560,10 @@ public void Dispose() private void CloseRuntimeHost() { - // Closes the CLR host context handle / releases the .NET Framework runtime host. This is - // process/runtime-level teardown, separate from environment teardown, invoked only by the - // optional JS dispose() hook. + // Closes this environment's CLR host: the hostfxr context handle (.NET 5+) or the + // ICLRRuntimeHost COM reference (.NET Framework). Each environment initializes its own, so + // this is per-environment teardown (the underlying shared CLR is not unloaded). Invoked at + // environment teardown and by the optional JS dispose() hook; idempotent. // Close the CLR host context handle, if it's still open. if (_hostContextHandle != default) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 4d27ec39..d59154cf 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -883,7 +883,10 @@ public void Dispose() IsDisposed = true; - SynchronizationContext.Dispose(); + // Dispose an already-created sync context only; never construct one here. Disposal can run + // during env finalization when no scope is current, and creating a sync context then would + // throw and skip the rest of teardown. + _synchronizationContext?.Dispose(); #if !(NETFRAMEWORK || NETSTANDARD) // ConditionalWeakTable<> is not enumerable in .NET Framework. diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 779aa057..e1be4ebc 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -169,16 +169,7 @@ private JSValueScope(napi_env env, JSRuntimeContext? context) ScopeType = JSValueScopeType.RuntimeContext; _parentScope = CurrentOrNull; - if (context != null) - { - // An explicit context is a root boundary (host, AOT module, or embedding). - if (!env.IsNull && env != context.UncheckedEnvironmentHandle) - { - throw new ArgumentException( - "Environment does not match the runtime context.", nameof(env)); - } - } - else + if (context == null) { // Inherit the parent scope's context, else recover it from the env instance data. context = _parentScope?.RuntimeContext @@ -187,10 +178,24 @@ private JSValueScope(napi_env env, JSRuntimeContext? context) "A runtime context could not be resolved for the scope."); } + // A supplied env must match the resolved context — whether passed explicitly (a root + // boundary: host, AOT module, or embedding) or inherited from the parent — otherwise this + // scope would wrap handles from a different environment. + if (!env.IsNull && env != context.UncheckedEnvironmentHandle) + { + throw new ArgumentException( + "Environment does not match the runtime context.", nameof(env)); + } + _env = context.UncheckedEnvironmentHandle; ThreadId = Environment.CurrentManagedThreadId; Runtime = context.Runtime; - ModuleHolder = new StrongBox(); + + // A nested runtime scope that continues the parent's context inherits its module holder; + // only a root/module boundary (a new or explicitly-provided context) starts a fresh one. + ModuleHolder = _parentScope?.RuntimeContext == context + ? _parentScope.ModuleHolder + : new StrongBox(); JSValueScope? previousScope = CurrentOrNull; try diff --git a/src/NodeApi/Runtime/TracingJSRuntime.cs b/src/NodeApi/Runtime/TracingJSRuntime.cs index 4a6f0762..1da7695b 100644 --- a/src/NodeApi/Runtime/TracingJSRuntime.cs +++ b/src/NodeApi/Runtime/TracingJSRuntime.cs @@ -448,6 +448,9 @@ public napi_value TraceCallback( throw new InvalidOperationException("Callback data is null.")); JSCallbackDescriptor descriptor = getCallbackDescriptor(data); + // Mirror InvokeCallback: make the module instance available to module-level members. + scope.ModuleHolder = descriptor.ModuleHolder; + Span argsSpan = stackalloc napi_value[length]; JSCallbackArgs args = new(scope, cbinfo, argsSpan, descriptor.Data); From 3a0345bb0e98ad6356bdd1593494aff0d1959cf4 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 16:13:37 -0700 Subject: [PATCH 03/38] Fix formatting --- src/NodeApi/JSValueScope.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index e1be4ebc..0765c516 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -169,14 +169,11 @@ private JSValueScope(napi_env env, JSRuntimeContext? context) ScopeType = JSValueScopeType.RuntimeContext; _parentScope = CurrentOrNull; - if (context == null) - { - // Inherit the parent scope's context, else recover it from the env instance data. - context = _parentScope?.RuntimeContext - ?? JSRuntimeContext.FromEnv(env) - ?? throw new InvalidOperationException( - "A runtime context could not be resolved for the scope."); - } + // Inherit the parent scope's context, else recover it from the env instance data. + context ??= _parentScope?.RuntimeContext + ?? JSRuntimeContext.FromEnv(env) + ?? throw new InvalidOperationException( + "A runtime context could not be resolved for the scope."); // A supplied env must match the resolved context — whether passed explicitly (a root // boundary: host, AOT module, or embedding) or inherited from the parent — otherwise this From e1ed27e52c865e1d5b8656d42beb2220c353e5ce Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 16:32:59 -0700 Subject: [PATCH 04/38] Free runtime context root at teardown - Wrapped-object and action finalizers resolve the context from napi_env (FromEnv) instead of a GCHandle finalize hint, so the context's rooting handle no longer needs to stay rooted. - At teardown the context clears its instance-data slot and frees its rooting GCHandle so it can be collected; the small instance-data block is intentionally kept so a late finalizer's FromEnv resolves no context rather than reading freed memory. - Clarify that the FromEnv runtime static is safe: JSRuntime is a stateless dispatch v-table. --- src/NodeApi.DotNetHost/ManagedHost.cs | 6 ++-- src/NodeApi/Interop/JSRuntimeContext.cs | 37 ++++++++++++++++----- src/NodeApi/JSValue.cs | 44 ++++++++++++++++--------- src/NodeApi/JSValueScope.cs | 3 -- 4 files changed, 61 insertions(+), 29 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index d4b97a0c..d7c02674 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -34,8 +34,10 @@ public sealed class ManagedHost : JSEventEmitter, IDisposable #if !(NETFRAMEWORK || NETSTANDARD) /// - /// Each instance of a managed host uses a separate assembly load context. - /// That way, static data is not shared across multiple host instances. + /// Each instance of a managed host uses a separate assembly load context, so static data is not + /// shared across host instances. It is not collectible: JSInterfaceMarshaller emits interface + /// adapter types with Reflection.Emit, which a collectible load context does not support, so the + /// context cannot be unloaded at teardown (only its resolve handlers are unsubscribed). /// private readonly AssemblyLoadContext _loadContext = new(name: default); #endif diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index d59154cf..ef19704f 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -128,12 +128,14 @@ public sealed class JSRuntimeContext : IDisposable // once the native host calls UseHostContextSlot() at startup. private static int s_instanceDataSlot = ModuleContextSlot; - // The runtime used to read env instance data in FromEnv, captured when a context registers. + // The runtime used to read env instance data in FromEnv, captured when a context registers. A + // JSRuntime is a stateless dispatch v-table, so any registered runtime can read any env's + // instance data; the process-wide static is intentional and safe. private static JSRuntime? s_instanceDataRuntime; - // A GCHandle rooting this context, used both as its env instance-data slot value and as the - // finalize hint for pooled GC handles. It is intentionally never freed: pooled-handle - // finalizers dereference it during env teardown, after this context is already disposed. + // A GCHandle rooting this context, used as its env instance-data slot value. It is freed when + // the context is disposed at env teardown; wrapped-object finalizers resolve the context via + // FromEnv(env) rather than this handle, so freeing it leaves no dangling finalize hint. internal napi_env EnvironmentHandle { @@ -291,8 +293,11 @@ private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hint) { // Runs during env teardown, where calling into JS is forbidden. Dispose the owning - // runtime's context and free the shared block. Only this runtime's slot is read (never the - // other runtime's); the slot GCHandles are intentionally left rooted (see _contextHandle). + // runtime's context (which clears its slot and frees its rooting GCHandle). Only this + // runtime's slot is read, never the other runtime's (whose GCHandle belongs to a separate + // GC heap). The block is intentionally not freed: a wrapped-object finalizer may still run + // after this and resolve the context via FromEnv, which reads this block; a freed block + // would be a use-after-free, whereas a cleared slot safely resolves to no context. nint slotHandle = ((nint*)data)[s_instanceDataSlot]; if (slotHandle != default) { @@ -305,8 +310,6 @@ private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hi // A finalizer must never throw; teardown continues regardless. } } - - Marshal.FreeHGlobal(data); } /// @@ -912,6 +915,24 @@ public void Dispose() } } } + + // Remove this context's root so it can be collected: clear its instance-data slot (a late + // wrapped-object finalizer then resolves no context via FromEnv and frees only its own + // handle) and free the rooting GCHandle. The block is left allocated (see + // FinalizeInstanceData) so a late FromEnv reads a cleared slot rather than freed memory. + if (ContextHandle != default) + { + Runtime.GetInstanceData(UncheckedEnvironmentHandle, out nint instanceData); + if (instanceData != default) + { + unsafe + { + ((nint*)instanceData)[s_instanceDataSlot] = default; + } + } + + GCHandle.FromIntPtr(ContextHandle).Free(); + } } private static void DisposeReferences( diff --git a/src/NodeApi/JSValue.cs b/src/NodeApi/JSValue.cs index 76f45e96..8536f95e 100644 --- a/src/NodeApi/JSValue.cs +++ b/src/NodeApi/JSValue.cs @@ -228,7 +228,7 @@ public static unsafe JSValue CreateExternal(object value) currentScope.UncheckedEnvironmentHandle, (nint)valueHandle, new napi_finalize(s_finalizeGCHandle), - currentScope.RuntimeContextHandle, + default, out napi_value result) .ThrowIfFailed(result); } @@ -825,7 +825,7 @@ public unsafe JSValue Wrap(object value) handle, (nint)valueHandle, new napi_finalize(s_finalizeGCHandle), - _scope!.RuntimeContextHandle).ThrowIfFailed(); + default).ThrowIfFailed(); return this; } @@ -844,7 +844,7 @@ public unsafe JSValue Wrap(object value, out JSReference wrapperWeakRef) handle, (nint)valueHandle, new napi_finalize(s_finalizeGCHandle), - _scope!.RuntimeContextHandle, + default, out napi_ref weakRef).ThrowIfFailed(); wrapperWeakRef = new JSReference(weakRef, isWeak: true); return this; @@ -1092,7 +1092,7 @@ public unsafe void AddFinalizer(Action finalize) handle, (nint)finalizeHandle, new napi_finalize(s_callFinalizeAction), - _scope!.RuntimeContextHandle).ThrowIfFailed(); + default).ThrowIfFailed(); } public unsafe void AddFinalizer(Action finalize, out JSReference finalizerRef) @@ -1104,7 +1104,7 @@ public unsafe void AddFinalizer(Action finalize, out JSReference finalizerRef) handle, (nint)finalizeHandle, new napi_finalize(s_callFinalizeAction), - _scope!.RuntimeContextHandle, + default, out napi_ref reference).ThrowIfFailed(); finalizerRef = new JSReference(reference, isWeak: true); } @@ -1284,11 +1284,13 @@ private static unsafe napi_value InvokeCallback( #endif internal static unsafe void FinalizeGCHandle(napi_env env, nint data, nint hint) { + // Resolve the context from the env rather than a finalize hint, so the context's rooting + // GCHandle can be freed at teardown. A null/disposed context means teardown already ran; + // just free the wrapped object's handle. GCHandle handle = GCHandle.FromIntPtr(data); - if (hint != default) + JSRuntimeContext? context = JSRuntimeContext.FromEnv(env); + if (context != null && !context.IsDisposed) { - GCHandle contextHandle = GCHandle.FromIntPtr(hint); - JSRuntimeContext context = (JSRuntimeContext)contextHandle.Target!; context.FreeGCHandle(handle); } else @@ -1321,19 +1323,29 @@ internal static unsafe void FinalizeGCHandleToPinnedMemory(napi_env env, nint da #endif private static unsafe void CallFinalizeAction(napi_env env, nint data, nint hint) { + // Resolve the context from the env rather than a finalize hint (see FinalizeGCHandle). GCHandle gcHandle = GCHandle.FromIntPtr(data); - GCHandle contextHandle = GCHandle.FromIntPtr(hint); - JSRuntimeContext context = (JSRuntimeContext)contextHandle.Target!; + JSRuntimeContext? context = JSRuntimeContext.FromEnv(env); try { - // TODO: [vmoroz] In future we will be not allowed to run JS in finalizers. - // We must remove creation of the scope. - using var scope = JSValueScope.CreateRuntimeScope(env, context); - ((Action)gcHandle.Target!)(); + if (context != null && !context.IsDisposed) + { + // TODO: [vmoroz] In future we will be not allowed to run JS in finalizers. + // We must remove creation of the scope. + using var scope = JSValueScope.CreateRuntimeScope(env, context); + ((Action)gcHandle.Target!)(); + } } finally { - context.FreeGCHandle(gcHandle); + if (context != null && !context.IsDisposed) + { + context.FreeGCHandle(gcHandle); + } + else + { + gcHandle.Free(); + } } } @@ -1483,7 +1495,7 @@ public unsafe void AddGCHandleFinalizer(nint finalizeData) handle, finalizeData, new napi_finalize(s_finalizeGCHandle), - Scope.RuntimeContextHandle).ThrowIfFailed(); + default).ThrowIfFailed(); } } diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 0765c516..770930b8 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -119,7 +119,6 @@ public static explicit operator napi_env(JSValueScope scope) public JSRuntime Runtime { get; } public JSRuntimeContext RuntimeContext { get; } - internal nint RuntimeContextHandle { get; } internal static JSRuntime CurrentRuntime => Current.Runtime; internal static JSRuntimeContext? CurrentRuntimeContext => CurrentOrNull?.RuntimeContext; @@ -199,7 +198,6 @@ private JSValueScope(napi_env env, JSRuntimeContext? context) { CurrentOrNull = this; RuntimeContext = context; - RuntimeContextHandle = context.ContextHandle; _previousSyncContext = SynchronizationContext.Current; SynchronizationContext.SetSynchronizationContext(context.SynchronizationContext); @@ -250,7 +248,6 @@ private JSValueScope(JSValueScopeType scopeType) { CurrentOrNull = this; RuntimeContext = _parentScope.RuntimeContext; - RuntimeContextHandle = _parentScope.RuntimeContextHandle; } catch (Exception) { From 2454f05a4807990fa8804a833abd6022e04a13fe Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 16:55:52 -0700 Subject: [PATCH 05/38] Reuse one JSRuntimeContext per env in embedding adapters The embedding runtime callbacks and Node-API scopes constructed a new JSRuntimeContext for the env on every invocation, leaking a context and overwriting the env instance-data slot each time. They now resolve the env's registered context (FromEnv) and create one only if absent, so there is a single context per env, disposed by the instance-data finalizer at teardown. --- src/NodeApi/Runtime/NodeEmbedding.cs | 17 ++++++++++++----- .../Runtime/NodeEmbeddingNodeApiScope.cs | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index 69a53e1b..1f2bba5c 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -353,6 +353,13 @@ internal static unsafe NodeEmbeddingStatus RuntimeConfigureCallbackAdapter( } } + // The embedding invokes these adapters (and opens Node-API scopes) repeatedly for the same + // env; reuse the env's registered context to keep one context per env, instead of leaking a new + // context and overwriting the instance-data slot on each call. The instance-data finalizer + // disposes the context at env teardown. + internal static JSRuntimeContext GetOrCreateContext(napi_env env) + => JSRuntimeContext.FromEnv(env) ?? new JSRuntimeContext(env, JSRuntime); + #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif @@ -363,7 +370,7 @@ internal static unsafe void RuntimePreloadCallbackAdapter( napi_value process, napi_value require) { - JSRuntimeContext context = new(env, JSRuntime); + JSRuntimeContext context = GetOrCreateContext(env); using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { @@ -388,7 +395,7 @@ internal static unsafe napi_value RuntimeLoadingCallbackAdapter( napi_value require, napi_value run_cjs) { - JSRuntimeContext context = new(env, JSRuntime); + JSRuntimeContext context = GetOrCreateContext(env); using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { @@ -413,7 +420,7 @@ internal static unsafe void RuntimeLoadedCallbackAdapter( napi_env env, napi_value loading_result) { - JSRuntimeContext context = new(env, JSRuntime); + JSRuntimeContext context = GetOrCreateContext(env); using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { @@ -437,7 +444,7 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( nint module_name, napi_value exports) { - JSRuntimeContext context = new(env, JSRuntime); + JSRuntimeContext context = GetOrCreateContext(env); using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { @@ -506,7 +513,7 @@ internal static unsafe NodeEmbeddingStatus TaskPostCallbackAdapter( #endif internal static unsafe void NodeApiRunCallbackAdapter(nint cb_data, napi_env env) { - JSRuntimeContext context = new(env, JSRuntime); + JSRuntimeContext context = GetOrCreateContext(env); using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); try { diff --git a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs index eca34635..9526b40c 100644 --- a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs +++ b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs @@ -20,7 +20,7 @@ public NodeEmbeddingNodeApiScope(NodeEmbeddingRuntime runtime) NodeEmbedding.JSRuntime.EmbeddingRuntimeOpenNodeApiScope( runtime.Handle, out _nodeApiScope, out napi_env env) .ThrowIfFailed(); - JSRuntimeContext context = new(env, NodeEmbedding.JSRuntime); + JSRuntimeContext context = NodeEmbedding.GetOrCreateContext(env); _valueScope = JSValueScope.CreateRuntimeScope(env, context); } From d217d0dfc3fd8bdf03ba47407ba41caac6f6fec8 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 17:20:14 -0700 Subject: [PATCH 06/38] Dispose IDisposable module instance at environment teardown JSModuleAttribute documents that a module class implementing IDisposable is disposed when the module is unloaded. Register the module instance as a disposable annotation on its runtime context so it is disposed at environment teardown, restoring that contract. --- src/NodeApi/Interop/JSModuleBuilderOfT.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/NodeApi/Interop/JSModuleBuilderOfT.cs b/src/NodeApi/Interop/JSModuleBuilderOfT.cs index b45f1b70..07c0ee69 100644 --- a/src/NodeApi/Interop/JSModuleBuilderOfT.cs +++ b/src/NodeApi/Interop/JSModuleBuilderOfT.cs @@ -34,6 +34,14 @@ public JSValue ExportModule(T module, JSObject exports) // Write through the holder the descriptors captured, so callbacks bound before the module // instance existed observe it. JSValueScope.Current.ModuleHolder!.Value = module; + + // Honor JSModuleAttribute's contract: an IDisposable module instance is disposed at + // environment teardown, when the context disposes its disposable annotations. + if (module is IDisposable disposable) + { + JSValueScope.Current.RuntimeContext.SetDisposableAnnotation(disposable); + } + exports.DefineProperties(Properties.ToArray()); return exports; } From 85e2611b7320655f2b453bee75a017cc4da91dec Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 19:55:52 -0700 Subject: [PATCH 07/38] Document the runtime model and add agent instructions Add docs/concepts/runtime-model.md covering the napi_env-per-module relationship, the node::Environment vs napi_env vs isolate/worker distinction (environment cleanup hook vs per-napi_env instance-data finalizer), instance-data slot ownership, the three JSValueScope types, and the rules for holding napi_value/napi_ref safely. Add AGENTS.md with thin CLAUDE.md and .github/copilot-instructions.md pointers, and surface the concepts docs in the site navigation. --- .github/copilot-instructions.md | 7 ++ AGENTS.md | 59 ++++++++++++++ CLAUDE.md | 7 ++ docs/.vitepress/config.mts | 7 ++ docs/concepts/runtime-model.md | 134 ++++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+) create mode 100644 .github/copilot-instructions.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md create mode 100644 docs/concepts/runtime-model.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..51cbb056 --- /dev/null +++ b/.github/copilot-instructions.md @@ -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`. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..37240b15 --- /dev/null +++ b/AGENTS.md @@ -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. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..aaca4204 --- /dev/null +++ b/CLAUDE.md @@ -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`. diff --git a/docs/.vitepress/config.mts b/docs/.vitepress/config.mts index 3f87ab30..fdf9a0c5 100644 --- a/docs/.vitepress/config.mts +++ b/docs/.vitepress/config.mts @@ -17,6 +17,13 @@ export default defineConfig({ sidebar: [ { text: 'Overview', link: '/overview' }, + { + text: 'Concepts', + items: [ + { text: 'Runtime model', link: '/concepts/runtime-model' }, + { text: 'Project layers', link: '/NodeApi-Layers' }, + ] + }, { text: 'Get Started', items: [ diff --git a/docs/concepts/runtime-model.md b/docs/concepts/runtime-model.md new file mode 100644 index 00000000..71fa39da --- /dev/null +++ b/docs/concepts/runtime-model.md @@ -0,0 +1,134 @@ +# 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`**. The instance-data block itself is intentionally *not* +freed: a wrapped-object finalizer may still run afterward and call `FromEnv`, and reading a freed +block would be a use-after-free — whereas reading a **cleared slot** simply resolves to "no context." + +## 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. + +## 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 run during teardown — no JS allowed + +A finalizer (for a wrapped .NET object, an external, or a reference) may run while the environment is +being torn down, where **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. +2. **Never assume ordering** between an individual wrapped-object finalizer and the instance-data + finalizer. Node drains its pending finalizers without a guaranteed order relative to instance-data + finalization, which is exactly why the context block is retained after its slot is cleared. + +## 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. From 047ed0b9702e671d19ca98f40fde9c365efefdf8 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 21:22:04 -0700 Subject: [PATCH 08/38] Harden SetDisposableAnnotation against post-dispose and replacement SetDisposableAnnotation now throws ObjectDisposedException if called after the context is disposed (the value would otherwise never be disposed), and disposes any same-type annotation it displaces so an owned annotation is never silently leaked. --- src/NodeApi/Interop/JSRuntimeContext.cs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index ef19704f..3d0c4842 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -872,12 +872,24 @@ public void SetAnnotation(T value) where T : class /// /// Associates an owning annotation with this context, keyed by its type. The context disposes - /// it when the context itself is disposed (at environment teardown). + /// it when the context itself is disposed (at environment teardown). Replacing an existing + /// annotation of the same type disposes the one being displaced. /// + /// The context is already disposed, so the value + /// would never be disposed. public void SetDisposableAnnotation(T value) where T : class, IDisposable { if (value is null) throw new ArgumentNullException(nameof(value)); - (_disposableAnnotations ??= new())[typeof(T)] = value; + if (IsDisposed) throw new ObjectDisposedException(nameof(JSRuntimeContext)); + + _disposableAnnotations ??= new(); + if (_disposableAnnotations.TryGetValue(typeof(T), out IDisposable? existing) && + !ReferenceEquals(existing, value)) + { + existing.Dispose(); + } + + _disposableAnnotations[typeof(T)] = value; } public void Dispose() From a4e6e4234f9909d27cf4fc062cfeab79acbfdd53 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 21:22:04 -0700 Subject: [PATCH 09/38] Give each loaded module its own module holder A generated module's hosted entry point opened a runtime scope that inherited the managed host's module holder, so loading a second module overwrote the first module's instance and later callbacks from the first module resolved the wrong instance. Add JSValueScope.CreateModuleScope, which references the surrounding context but starts a fresh module holder, and use it from the generated module entry points. --- src/NodeApi.Generator/ModuleGenerator.cs | 4 ++-- src/NodeApi/JSValueScope.cs | 25 +++++++++++++++++++----- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/NodeApi.Generator/ModuleGenerator.cs b/src/NodeApi.Generator/ModuleGenerator.cs index d9e8c3cf..2ecf5c68 100644 --- a/src/NodeApi.Generator/ModuleGenerator.cs +++ b/src/NodeApi.Generator/ModuleGenerator.cs @@ -295,7 +295,7 @@ private SourceBuilder GenerateModuleInitializer( s += $"public static napi_value _{ModuleInitializeMethodName}(napi_env env, napi_value exports)"; s += "{"; s += "JSRuntimeContext context = JSRuntimeContext.Create(env);"; - s += "using var moduleScope = JSValueScope.CreateRuntimeScope(env, context);"; + s += "using var moduleScope = JSValueScope.CreateModuleScope(env, context);"; s += $"return {ModuleExportsMethodName}(moduleScope, exports);"; s += "}"; s += "#endif"; @@ -305,7 +305,7 @@ private SourceBuilder GenerateModuleInitializer( // module; the scope resolves the runtime context from that host. s += $"public static napi_value {ModuleInitializeMethodName}(napi_env env, napi_value exports)"; s += "{"; - s += "using var moduleScope = JSValueScope.CreateRuntimeScope(env);"; + s += "using var moduleScope = JSValueScope.CreateModuleScope(env);"; s += $"return {ModuleExportsMethodName}(moduleScope, exports);"; s += "}"; s++; diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 770930b8..9888211b 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -147,6 +147,19 @@ public static JSValueScope CreateRuntimeScope( napi_env env = default, JSRuntimeContext? context = null) => new(env, context); + /// + /// Creates a scope that starts a fresh module + /// boundary: it references the same (inherited or supplied) but + /// begins a new module holder, so each loaded module resolves its own module instance via + /// . + /// + /// The JS environment handle. + /// The runtime context to reference. When null it is inherited from the + /// parent scope, or recovered from the environment instance data. + public static JSValueScope CreateModuleScope( + napi_env env = default, JSRuntimeContext? context = null) + => new(env, context, moduleBoundary: true); + /// /// Creates a napi handle scope nested within the current scope. JS values created within it /// are released when it is disposed, unless held by a . @@ -161,9 +174,10 @@ public static JSValueScope CreateRuntimeScope( /// /// Creates a scope that references an existing - /// (it never creates one). + /// (it never creates one). When + /// is true it starts a fresh module holder even if the context is inherited from the parent. /// - private JSValueScope(napi_env env, JSRuntimeContext? context) + private JSValueScope(napi_env env, JSRuntimeContext? context, bool moduleBoundary = false) { ScopeType = JSValueScopeType.RuntimeContext; _parentScope = CurrentOrNull; @@ -187,9 +201,10 @@ private JSValueScope(napi_env env, JSRuntimeContext? context) ThreadId = Environment.CurrentManagedThreadId; Runtime = context.Runtime; - // A nested runtime scope that continues the parent's context inherits its module holder; - // only a root/module boundary (a new or explicitly-provided context) starts a fresh one. - ModuleHolder = _parentScope?.RuntimeContext == context + // A nested runtime scope that continues the parent's context inherits its module holder; a + // module boundary, or a root with a new/explicit context, starts a fresh one so each loaded + // module resolves its own module instance. + ModuleHolder = !moduleBoundary && _parentScope?.RuntimeContext == context ? _parentScope.ModuleHolder : new StrongBox(); From 3b84450636dedca1b261807dae5b0d0728b96da3 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 21:27:19 -0700 Subject: [PATCH 10/38] Return a context from FromEnv only when it matches the env The embedding adapters resolve the env's context via FromEnv, which reads instance data through the process-wide static runtime. When a different runtime last registered (for example a mock in unit tests), that read can return another env's block, so FromEnv returned a context whose env did not match and the scope constructor threw, crashing the host. FromEnv now returns a context only when its environment handle matches the requested env. --- src/NodeApi/Interop/JSRuntimeContext.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 3d0c4842..1015f1fb 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -190,9 +190,16 @@ public static explicit operator napi_env(JSRuntimeContext context) } nint slotHandle = ((nint*)instanceData)[s_instanceDataSlot]; - return slotHandle == default - ? null - : GCHandle.FromIntPtr(slotHandle).Target as JSRuntimeContext; + if (slotHandle == default) + { + return null; + } + + // Resolve the context only if it actually belongs to this env. The runtime that reads the + // instance data is a process-wide static, so a stale or foreign registration could point at + // another env's block; a context whose env does not match must not be returned. + JSRuntimeContext? context = GCHandle.FromIntPtr(slotHandle).Target as JSRuntimeContext; + return context is not null && context.UncheckedEnvironmentHandle == env ? context : null; } /// From 65ec52e5e2eda2cfe960e2c3ba838a3bf36ab8e5 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 27 Aug 2026 21:45:55 -0700 Subject: [PATCH 11/38] Address pre-PR code review: shared-context module disposal Fix a regression where IDisposable module instances loaded into one managed host disposed each other: ExportModule inferred T=IDisposable and registered every module (and, on the module-less path, the context itself) under one type-keyed annotation, so loading a second module displaced and disposed the first mid-load. Module instances now register in an append-many list on the context (AddModuleDisposable), each disposed once at teardown; the context is never registered as its own module disposable. Adds a regression test that loads two IDisposable modules through ExportModule. Also: JSValueScope.Dispose fetches the env only for handle/escapable scopes so disposing a runtime scope after its context is torn down does not throw; document the intentional per-env instance-data block retention at its allocation; move the rooting-GCHandle doc onto ContextHandle. --- src/NodeApi/Interop/JSModuleBuilderOfT.cs | 11 ++++-- src/NodeApi/Interop/JSRuntimeContext.cs | 46 ++++++++++++++++++++--- src/NodeApi/JSValueScope.cs | 10 +++-- test/JSValueScopeTests.cs | 45 ++++++++++++++++++++++ test/MockJSRuntime.cs | 4 ++ 5 files changed, 102 insertions(+), 14 deletions(-) diff --git a/src/NodeApi/Interop/JSModuleBuilderOfT.cs b/src/NodeApi/Interop/JSModuleBuilderOfT.cs index 07c0ee69..7fecbb8e 100644 --- a/src/NodeApi/Interop/JSModuleBuilderOfT.cs +++ b/src/NodeApi/Interop/JSModuleBuilderOfT.cs @@ -35,11 +35,14 @@ public JSValue ExportModule(T module, JSObject exports) // instance existed observe it. JSValueScope.Current.ModuleHolder!.Value = module; - // Honor JSModuleAttribute's contract: an IDisposable module instance is disposed at - // environment teardown, when the context disposes its disposable annotations. - if (module is IDisposable disposable) + // Honor JSModuleAttribute's IDisposable contract. Modules loaded into one host share a + // context, and the module-less path passes the context as the module, so append real + // instances (not the type-keyed annotation, which would collide on typeof(IDisposable)); + // the context disposes itself via its own teardown. + JSRuntimeContext context = JSValueScope.Current.RuntimeContext; + if (module is IDisposable disposable && !ReferenceEquals(module, context)) { - JSValueScope.Current.RuntimeContext.SetDisposableAnnotation(disposable); + context.AddModuleDisposable(disposable); } exports.DefineProperties(Properties.ToArray()); diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 1015f1fb..c1246ecd 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -116,6 +116,10 @@ public sealed class JSRuntimeContext : IDisposable private Dictionary? _annotations; private Dictionary? _disposableAnnotations; + // Module instances disposed at context teardown. Unlike the type-keyed annotations, several + // modules share one context, so these are appended rather than keyed by type. + private List? _moduleDisposables; + // Env instance-data layout: one GCHandle slot per runtime sharing the napi_env. Slot 0 is the // module context (managed host / AOT module / embedding); slot 1 is the native host context. // A runtime reads and writes only its own slot, so it never dereferences the other runtime's @@ -133,10 +137,6 @@ public sealed class JSRuntimeContext : IDisposable // instance data; the process-wide static is intentional and safe. private static JSRuntime? s_instanceDataRuntime; - // A GCHandle rooting this context, used as its env instance-data slot value. It is freed when - // the context is disposed at env teardown; wrapped-object finalizers resolve the context via - // FromEnv(env) rather than this handle, so freeing it leaves no dangling finalize hint. - internal napi_env EnvironmentHandle { get @@ -158,8 +158,9 @@ internal napi_env EnvironmentHandle internal napi_env UncheckedEnvironmentHandle { get; } /// - /// Gets the GCHandle that roots this context, for use as a finalize hint by scopes that adopt - /// this context. + /// Gets the GCHandle that roots this context and is stored in its env instance-data slot. It is + /// freed when the context is disposed at env teardown; finalizers resolve the context via + /// rather than this handle, so freeing it leaves nothing dangling. /// internal nint ContextHandle { get; } @@ -271,6 +272,9 @@ private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) runtime.GetInstanceData(env, out nint instanceData).ThrowIfFailed(); if (instanceData == default) { + // Retained for the env's lifetime, never freed here or by the finalizer: a late + // wrapped-object finalizer may still read a cleared slot via FromEnv after teardown + // (see FinalizeInstanceData), so freeing this block would risk a use-after-free. instanceData = Marshal.AllocHGlobal(IntPtr.Size * InstanceDataSlotCount); for (int i = 0; i < InstanceDataSlotCount; i++) { @@ -899,6 +903,21 @@ public void SetDisposableAnnotation(T value) where T : class, IDisposable _disposableAnnotations[typeof(T)] = value; } + /// + /// Registers a module instance to be disposed at environment teardown. Unlike + /// , several modules can share one context, so instances + /// are appended rather than keyed by type, and each is disposed once. + /// + internal void AddModuleDisposable(IDisposable disposable) + { + if (disposable is null) throw new ArgumentNullException(nameof(disposable)); + _moduleDisposables ??= new(); + if (!_moduleDisposables.Contains(disposable)) + { + _moduleDisposables.Add(disposable); + } + } + public void Dispose() { if (IsDisposed) return; @@ -920,6 +939,21 @@ public void Dispose() DisposeReferences(_structMap.Values); // Disposed after IsDisposed is set, so a late cross-thread post is already a no-op. + if (_moduleDisposables != null) + { + foreach (IDisposable moduleDisposable in _moduleDisposables) + { + try + { + moduleDisposable.Dispose(); + } + catch + { + // A failing module disposal must not prevent the rest of teardown. + } + } + } + if (_disposableAnnotations != null) { foreach (IDisposable annotation in _disposableAnnotations.Values) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 9888211b..6f5c9ecc 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -276,17 +276,19 @@ public void Dispose() if (IsDisposed) return; IsDisposed = true; - napi_env env = RuntimeContext.EnvironmentHandle; - switch (ScopeType) { + // Fetch the env only where it is used, so disposing a runtime scope after its context + // is torn down does not throw from the checked handle accessor. case JSValueScopeType.Handle: Runtime.CloseHandleScope( - env, new napi_handle_scope(_scopeHandle)).ThrowIfFailed(); + RuntimeContext.EnvironmentHandle, + new napi_handle_scope(_scopeHandle)).ThrowIfFailed(); break; case JSValueScopeType.Escapable: Runtime.CloseEscapableHandleScope( - env, new napi_escapable_handle_scope(_scopeHandle)).ThrowIfFailed(); + RuntimeContext.EnvironmentHandle, + new napi_escapable_handle_scope(_scopeHandle)).ThrowIfFailed(); break; default: SynchronizationContext.SetSynchronizationContext(_previousSyncContext); diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 8a2e180b..7664e27c 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -101,6 +101,51 @@ public void HandleScopeRequiresParentScope() () => JSValueScope.CreateEscapableScope()); } + private sealed class DisposableModule : IDisposable + { + public int DisposeCount { get; private set; } + + public void Dispose() => DisposeCount++; + } + + [Fact] + public void DisposableModulesShareContextAndAreDisposedOnceAtTeardown() + { + var moduleA = new DisposableModule(); + var moduleB = new DisposableModule(); + + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + using (JSValueScope.CreateRuntimeScope(env, context)) + { + // Two generated modules loaded into one managed host share this context; each opens a + // module-boundary scope and exports its instance. + using (JSValueScope.CreateModuleScope(env)) + { + new JSModuleBuilder().ExportModule( + moduleA, (JSObject)JSValue.CreateObject()); + } + + using (JSValueScope.CreateModuleScope(env)) + { + new JSModuleBuilder().ExportModule( + moduleB, (JSObject)JSValue.CreateObject()); + } + + // Loading the second module must not dispose the first. + Assert.Equal(0, moduleA.DisposeCount); + Assert.Equal(0, moduleB.DisposeCount); + } + + context.Dispose(); + + // Each module instance is disposed exactly once at env teardown. + Assert.Equal(1, moduleA.DisposeCount); + Assert.Equal(1, moduleB.DisposeCount); + } + [Fact] public void AccessValueFromClosedScope() { diff --git a/test/MockJSRuntime.cs b/test/MockJSRuntime.cs index 325ff4f4..b7df9bee 100644 --- a/test/MockJSRuntime.cs +++ b/test/MockJSRuntime.cs @@ -129,6 +129,10 @@ public override napi_status CreateObject( return napi_ok; } + public override napi_status DefineProperties( + napi_env env, napi_value js_object, ReadOnlySpan properties) + => napi_ok; + public override napi_status GetValueType( napi_env env, napi_value value, out napi_valuetype result) { From ba16dcec75f6c9bc448d8910c8eae19c4ebabc32 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 10:43:06 -0700 Subject: [PATCH 12/38] Free the env instance-data block at teardown; fix module-disposable dedup and failed managed-init leak - FinalizeInstanceData frees the per-env instance-data block once the last context on the env is disposed, instead of retaining it for the process lifetime; a host context's disposal cascades synchronously to the other slot, and the block is not nulled via napi_set_instance_data (that would double-free Node's finalizer record). Updates runtime-model.md. - AddModuleDisposable deduplicates by reference identity rather than Equals, so equal-but-distinct IDisposable module instances are each disposed once. Adds a regression test. - ManagedHost disposes its JSRuntimeContext on the failed-initialization path so its instance-data GCHandle, synchronization context, and resolve handlers are released when no teardown handshake was established. --- docs/concepts/runtime-model.md | 16 ++++++---- src/NodeApi.DotNetHost/ManagedHost.cs | 12 ++++++- src/NodeApi/Interop/JSRuntimeContext.cs | 42 ++++++++++++++++++------- test/JSValueScopeTests.cs | 34 ++++++++++++++++++++ 4 files changed, 85 insertions(+), 19 deletions(-) diff --git a/docs/concepts/runtime-model.md b/docs/concepts/runtime-model.md index 71fa39da..ec521fc8 100644 --- a/docs/concepts/runtime-model.md +++ b/docs/concepts/runtime-model.md @@ -75,9 +75,13 @@ contexts share an env. A runtime **reads and writes only its own slot**, so it n 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`**. The instance-data block itself is intentionally *not* -freed: a wrapped-object finalizer may still run afterward and call `FromEnv`, and reading a freed -block would be a use-after-free — whereas reading a **cleared slot** simply resolves to "no context." +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 @@ -121,9 +125,9 @@ 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. -2. **Never assume ordering** between an individual wrapped-object finalizer and the instance-data - finalizer. Node drains its pending finalizers without a guaranteed order relative to instance-data - finalization, which is exactly why the context block is retained after its slot is cleared. +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 diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index d7c02674..f34ae189 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -255,7 +255,17 @@ public static unsafe napi_value InitializeModule( catch (Exception ex) { Trace($"Failed to load CLR managed host module: {ex}"); - JSError.ThrowError(ex); + try + { + JSError.ThrowError(ex); + } + finally + { + // Failed init: nothing else disposes this context (when hosted, the native host + // never received a teardown callback), so dispose it here to release its slot + // GCHandle, synchronization context, and resolve handlers. + context.Dispose(); + } } #if NETFRAMEWORK || NETSTANDARD diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index c1246ecd..f03a207e 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -272,9 +272,8 @@ private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) runtime.GetInstanceData(env, out nint instanceData).ThrowIfFailed(); if (instanceData == default) { - // Retained for the env's lifetime, never freed here or by the finalizer: a late - // wrapped-object finalizer may still read a cleared slot via FromEnv after teardown - // (see FinalizeInstanceData), so freeing this block would risk a use-after-free. + // One block per env, freed by FinalizeInstanceData when the last context on the env is + // disposed at teardown. instanceData = Marshal.AllocHGlobal(IntPtr.Size * InstanceDataSlotCount); for (int i = 0; i < InstanceDataSlotCount; i++) { @@ -306,9 +305,7 @@ private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hi // Runs during env teardown, where calling into JS is forbidden. Dispose the owning // runtime's context (which clears its slot and frees its rooting GCHandle). Only this // runtime's slot is read, never the other runtime's (whose GCHandle belongs to a separate - // GC heap). The block is intentionally not freed: a wrapped-object finalizer may still run - // after this and resolve the context via FromEnv, which reads this block; a freed block - // would be a use-after-free, whereas a cleared slot safely resolves to no context. + // GC heap). nint slotHandle = ((nint*)data)[s_instanceDataSlot]; if (slotHandle != default) { @@ -321,6 +318,20 @@ private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hi // A finalizer must never throw; teardown continues regardless. } } + + // Free the shared block once the last context on the env is gone (all slots cleared); + // disposing a host context cascades synchronously to the other slot. Do not null it out + // via napi_set_instance_data: that deletes this very TrackedFinalizer, which Node then + // deletes again (double free). + for (int i = 0; i < InstanceDataSlotCount; i++) + { + if (((nint*)data)[i] != default) + { + return; + } + } + + Marshal.FreeHGlobal(data); } /// @@ -912,10 +923,18 @@ internal void AddModuleDisposable(IDisposable disposable) { if (disposable is null) throw new ArgumentNullException(nameof(disposable)); _moduleDisposables ??= new(); - if (!_moduleDisposables.Contains(disposable)) + + // Dedupe by identity, not Equals: a module class may override equality, but each distinct + // instance must be disposed once. + foreach (IDisposable existing in _moduleDisposables) { - _moduleDisposables.Add(disposable); + if (ReferenceEquals(existing, disposable)) + { + return; + } } + + _moduleDisposables.Add(disposable); } public void Dispose() @@ -969,10 +988,9 @@ public void Dispose() } } - // Remove this context's root so it can be collected: clear its instance-data slot (a late - // wrapped-object finalizer then resolves no context via FromEnv and frees only its own - // handle) and free the rooting GCHandle. The block is left allocated (see - // FinalizeInstanceData) so a late FromEnv reads a cleared slot rather than freed memory. + // Remove this context's root so it can be collected: clear its instance-data slot (a + // concurrent FromEnv then resolves no context) and free the rooting GCHandle. The shared + // block itself is freed by FinalizeInstanceData once every context on the env is gone. if (ContextHandle != default) { Runtime.GetInstanceData(UncheckedEnvironmentHandle, out nint instanceData); diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 7664e27c..7e85a05c 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -146,6 +146,40 @@ public void DisposableModulesShareContextAndAreDisposedOnceAtTeardown() Assert.Equal(1, moduleB.DisposeCount); } + private sealed class EqualDisposable : IDisposable + { + public int DisposeCount { get; private set; } + + public void Dispose() => DisposeCount++; + + // All instances compare equal, to prove module disposables dedupe by identity, not Equals. + public override bool Equals(object? obj) => obj is EqualDisposable; + + public override int GetHashCode() => 0; + } + + [Fact] + public void ModuleDisposablesAreDedupedByIdentityNotEquality() + { + var moduleA = new EqualDisposable(); + var moduleB = new EqualDisposable(); + Assert.True(moduleA.Equals(moduleB)); + + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + context.AddModuleDisposable(moduleA); + context.AddModuleDisposable(moduleB); + context.AddModuleDisposable(moduleA); // Re-adding the same instance is a no-op. + + context.Dispose(); + + // Both distinct instances are disposed once, despite comparing equal. + Assert.Equal(1, moduleA.DisposeCount); + Assert.Equal(1, moduleB.DisposeCount); + } + [Fact] public void AccessValueFromClosedScope() { From e5114f401cbf6d8d8e4a891bda8f689db419e74e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 11:36:14 -0700 Subject: [PATCH 13/38] Pin JSRuntimeContext to its creation thread Record the owning managed thread on JSRuntimeContext at construction and reject entering a runtime scope from another thread, so a background thread cannot adopt a context created for a different JS thread and invoke Node-API off that thread. Adds a regression test. Addresses the remaining Copilot re-review thread on JSValueScope thread affinity. --- src/NodeApi/Interop/JSRuntimeContext.cs | 7 +++++++ src/NodeApi/JSValueScope.cs | 10 ++++++++++ test/JSValueScopeTests.cs | 15 +++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index f03a207e..1c69a588 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -164,6 +164,12 @@ internal napi_env EnvironmentHandle /// internal nint ContextHandle { get; } + /// + /// The managed thread that constructed this context — its environment's JS thread. A runtime + /// scope may be entered only on this thread. + /// + internal int OwningThreadId { get; } + public static explicit operator napi_env(JSRuntimeContext context) { if (context is null) throw new ArgumentNullException(nameof(context)); @@ -255,6 +261,7 @@ internal JSRuntimeContext( UncheckedEnvironmentHandle = env; Runtime = runtime; + OwningThreadId = Environment.CurrentManagedThreadId; ContextHandle = (nint)GCHandle.Alloc(this); RegisterInstanceData(env, runtime); diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 6f5c9ecc..87a38236 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -198,6 +198,16 @@ private JSValueScope(napi_env env, JSRuntimeContext? context, bool moduleBoundar } _env = context.UncheckedEnvironmentHandle; + + // A runtime context is bound to the thread that created it (its env's JS thread); entering it + // from another thread would allow napi to be called off that thread. + if (context.OwningThreadId != Environment.CurrentManagedThreadId) + { + throw new JSInvalidThreadAccessException( + _parentScope, + "A runtime context may be entered only on the thread that created it."); + } + ThreadId = Environment.CurrentManagedThreadId; Runtime = context.Runtime; diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 7e85a05c..f86a01c0 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -275,6 +275,21 @@ public void AccessValueFromDifferentRootScope() }).Wait(); } + [Fact] + public void EnterRuntimeContextFromDifferentThreadThrows() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + // A runtime context may be entered only on the thread that created it. + TestUtils.RunInThread(() => + { + Assert.Throws( + () => JSValueScope.CreateRuntimeScope(env, context)); + }).Wait(); + } + // The module instance is captured through a shared holder: descriptors take the holder during // initialization (before the instance exists) and observe the instance once dispatch assigns it. // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. From 5dc04b62423d53e6634e7df5babedd8f0a183b6e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 15:08:00 -0700 Subject: [PATCH 14/38] Harden runtime-context construction and scope entry - CreateRuntimeScope rejects a disposed context (its env is torn down), instead of adopting it and calling Node-API on a dead env via the unchecked handle. Adds a regression test. - JSRuntimeContext construction rolls back the rooting GCHandle and the instance-data block if instance-data registration fails, so a failed constructor leaks neither. - ManagedHost subscribes the process-wide assembly-resolve handlers after all fallible construction, so a constructor that throws (before being registered for disposal) does not leave them rooting the failed host. - worker_teardown_stress.js fails the test on any worker error for the worker's full lifetime, including during terminate(), instead of only while awaiting readiness. --- src/NodeApi.DotNetHost/ManagedHost.cs | 25 +++++++++++-------- src/NodeApi/Interop/JSRuntimeContext.cs | 23 ++++++++++++++--- src/NodeApi/JSValueScope.cs | 7 ++++++ test/JSValueScopeTests.cs | 13 ++++++++++ .../napi-dotnet/worker_teardown_stress.js | 14 +++++------ 5 files changed, 60 insertions(+), 22 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index f34ae189..4787b741 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -82,17 +82,6 @@ public sealed class ManagedHost : JSEventEmitter, IDisposable /// JS object on which the managed host APIs will be exported. public ManagedHost(JSObject exports) { -#if NETFRAMEWORK || NETSTANDARD - AppDomain.CurrentDomain.AssemblyResolve += OnResolvingAssembly; -#else - _loadContext.Resolving += OnResolvingAssembly; - - // It shouldn't be necessary to handle resolve events in the default load context. - // But TypeBuilder (used by JSInterfaceMarshaller) seems to require it when a nuget - // package referenced type is replaced with a system type, as with IAsyncEnumerable. - AssemblyLoadContext.Default.Resolving += OnResolvingAssembly; -#endif - JSValue addListener(JSCallbackArgs args) { AddListener(eventName: (string)args[0], listener: args[1]); @@ -145,6 +134,20 @@ JSValue removeListener(JSCallbackArgs args) { _exportedAssembliesByName.Add(typeof(Console).Assembly.GetName().Name!); } + + // Subscribe the process-wide resolve handlers last, after all fallible construction: a + // constructor that throws is never registered for disposal, so leaving them subscribed + // would root the failed host. +#if NETFRAMEWORK || NETSTANDARD + AppDomain.CurrentDomain.AssemblyResolve += OnResolvingAssembly; +#else + _loadContext.Resolving += OnResolvingAssembly; + + // It shouldn't be necessary to handle resolve events in the default load context. + // But TypeBuilder (used by JSInterfaceMarshaller) seems to require it when a nuget + // package referenced type is replaced with a system type, as with IAsyncEnumerable. + AssemblyLoadContext.Default.Resolving += OnResolvingAssembly; +#endif } public static bool IsTracingEnabled { get; } = diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 1c69a588..14fb829b 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -263,7 +263,18 @@ internal JSRuntimeContext( Runtime = runtime; OwningThreadId = Environment.CurrentManagedThreadId; ContextHandle = (nint)GCHandle.Alloc(this); - RegisterInstanceData(env, runtime); + try + { + RegisterInstanceData(env, runtime); + } + catch + { + // Registration failed before any caller holds this context to dispose it; free the + // rooting handle so a failed construction leaks nothing (the block, if allocated, is + // freed by RegisterInstanceData). + GCHandle.FromIntPtr(ContextHandle).Free(); + throw; + } _synchronizationContext = synchronizationContext; } @@ -287,11 +298,17 @@ private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) ((nint*)instanceData)[i] = default; } - runtime.SetInstanceData( + napi_status status = runtime.SetInstanceData( env, instanceData, new napi_finalize(s_finalizeInstanceData), - finalizeHint: default).ThrowIfFailed(); + finalizeHint: default); + if (status != napi_status.napi_ok) + { + // Registration failed, so Node never took ownership of the block; free it here. + Marshal.FreeHGlobal(instanceData); + status.ThrowIfFailed(); + } } ((nint*)instanceData)[s_instanceDataSlot] = ContextHandle; diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 87a38236..7cfc087c 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -188,6 +188,13 @@ private JSValueScope(napi_env env, JSRuntimeContext? context, bool moduleBoundar ?? throw new InvalidOperationException( "A runtime context could not be resolved for the scope."); + // A disposed context's environment is torn down; entering it would call Node-API on a + // dead env, which the scope's own unchecked handle would not catch. + if (context.IsDisposed) + { + throw new ObjectDisposedException(nameof(JSRuntimeContext)); + } + // A supplied env must match the resolved context — whether passed explicitly (a root // boundary: host, AOT module, or embedding) or inherited from the parent — otherwise this // scope would wrap handles from a different environment. diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index f86a01c0..6c740d3d 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -290,6 +290,19 @@ public void EnterRuntimeContextFromDifferentThreadThrows() }).Wait(); } + [Fact] + public void EnterDisposedRuntimeContextThrows() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + context.Dispose(); + + // A disposed context's environment is torn down, so a scope must not adopt it. + Assert.Throws( + () => JSValueScope.CreateRuntimeScope(env, context)); + } + // The module instance is captured through a shared holder: descriptors take the holder during // initialization (before the instance exists) and observe the instance once dispatch assigns it. // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. diff --git a/test/TestCases/napi-dotnet/worker_teardown_stress.js b/test/TestCases/napi-dotnet/worker_teardown_stress.js index 07c23423..724ea1ad 100644 --- a/test/TestCases/napi-dotnet/worker_teardown_stress.js +++ b/test/TestCases/napi-dotnet/worker_teardown_stress.js @@ -23,16 +23,14 @@ if (isMainThread) { (async () => { for (let i = 0; i < iterations; i++) { const worker = new Worker(__filename); - await new Promise((resolve, reject) => { + // Fail the test on any worker error for the worker's full lifetime -- including during + // terminate() -- not just while awaiting readiness (as worker_teardown.js does). + worker.on('error', (err) => { throw err; }); + await new Promise((resolve) => { worker.once('message', (message) => { - try { - assert.strictEqual(message, 'ready'); - resolve(); - } catch (err) { - reject(err); - } + assert.strictEqual(message, 'ready'); + resolve(); }); - worker.once('error', reject); }); await worker.terminate(); } From 7ab932b9756d9cc68fc4c2953426c7e7deb71711 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 15:31:28 -0700 Subject: [PATCH 15/38] Run full host disposal from the JS dispose() hook The native host's JS dispose() hook now runs the full idempotent Dispose() -- notify the managed host, then close the runtime-host channel -- instead of only CloseRuntimeHost(). On .NET Framework the managed host is notified only through the runtime-host channel, so closing it first stranded the managed context, its registration GCHandle, and the resolve handlers. --- src/NodeApi/DotNetHost/NativeHost.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index f7892314..1a939ba8 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -495,10 +495,11 @@ private JSValue InitializeDotNetHost( exports.SetProperty("require", require); exports.SetProperty("import", import); - // Define a dispose method implemented by the native host that closes the CLR context. - // The managed host proxy will pass through dispose calls to this callback. + // The dispose method runs the full idempotent host disposal -- notifying the managed host + // before closing the runtime-host channel -- so on .NET Framework (which notifies managed + // code only through that channel) the managed registration is released, not stranded. exports.DefineProperties(new JSPropertyDescriptor( - "dispose", (_) => { CloseRuntimeHost(); return default; })); + "dispose", (_) => { Dispose(); return default; })); // Invoke the managed host initialize method. It defines properties on the exports object // and fills in the registration so the native host can keep the managed host alive and From 372ce30db186f7ae66385abd90ff5c5e7922d37e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 15:57:28 -0700 Subject: [PATCH 16/38] Guard lazy sync-context creation; report a stackless init error - JSRuntimeContext.SynchronizationContext rejects lazy creation unless this context is current (and after disposal). The factory captures JSValueScope.Current's env, so creating context A's sync context while context B is current would otherwise bind A to B's environment. Adds a regression test. - ManagedHost reports a stackless error on the failed-initialization path before disposing the context, so the disposed context's lazy context-backed stack getter cannot fault when JavaScript later reads the error's stack. --- src/NodeApi.DotNetHost/ManagedHost.cs | 5 ++++- src/NodeApi/Interop/JSRuntimeContext.cs | 28 ++++++++++++++++++++++--- test/JSValueScopeTests.cs | 22 +++++++++++++++++++ 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index 4787b741..480ccdea 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -260,7 +260,10 @@ public static unsafe napi_value InitializeModule( Trace($"Failed to load CLR managed host module: {ex}"); try { - JSError.ThrowError(ex); + // Report a stackless error: this context is disposed below, so an error carrying + // the usual lazy context-backed `stack` getter would fault when JavaScript later + // reads `stack` (callback dispatch could not resolve the disposed context). + JSError.ThrowError(ex.ToString()); } finally { diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 14fb829b..a8ccf52c 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -230,11 +230,33 @@ public static explicit operator napi_env(JSRuntimeContext context) /// /// Gets the synchronization context that marshals callbacks and continuations to the JS thread. - /// A default one is created on first access, which happens while a scope for this context is - /// current, because creating it requires the current scope's runtime and environment. + /// A default one is created on first access, which must happen while a scope for this context is + /// current, because creating it captures the current scope's runtime and environment. /// public JSSynchronizationContext SynchronizationContext - => _synchronizationContext ??= JSSynchronizationContext.Create(); + { + get + { + if (_synchronizationContext is not null) + { + return _synchronizationContext; + } + + // Lazy creation captures the CURRENT scope's env/thread, so it must run only while this + // context is current -- otherwise it would bind this context to a different environment. + if (IsDisposed) + { + throw new ObjectDisposedException(nameof(JSRuntimeContext)); + } + if (JSValueScope.Current.RuntimeContext != this) + { + throw new InvalidOperationException( + "The synchronization context must be created while its runtime context is current."); + } + + return _synchronizationContext = JSSynchronizationContext.Create(); + } + } /// /// Creates a runtime context for a JS environment. Used by AOT module entry points and other diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 6c740d3d..85ccbc61 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -303,6 +303,28 @@ public void EnterDisposedRuntimeContextThrows() () => JSValueScope.CreateRuntimeScope(env, context)); } + [Fact] + public void SynchronizationContextRejectsLazyCreateWhenContextNotCurrent() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var contextA = new JSRuntimeContext(env, _mockRuntime); // no sync context -> lazy + var contextB = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + using (JSValueScope.CreateRuntimeScope(env, contextB)) + { + // contextB is current, so lazily creating contextA's sync context (which would capture + // the current scope's environment) must be rejected. + Assert.Throws(() => contextA.SynchronizationContext); + } + + contextA.Dispose(); + + // After disposal, lazy creation is rejected too. + Assert.Throws(() => contextA.SynchronizationContext); + contextB.Dispose(); + } + // The module instance is captured through a shared holder: descriptors take the holder during // initialization (before the instance exists) and observe the instance once dispatch assigns it. // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. From 1ca52e2d65deac38cd495a4dc8bf998a842803c0 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 16:38:39 -0700 Subject: [PATCH 17/38] Dispose exports reference on host dispose; clarify finalizer doc - NativeHost.Dispose now disposes the exports JSReference before clearing it. On an explicit JS dispose() (environment still alive) this releases its napi_ref on the JS thread instead of leaking it -- the dropped reference's off-thread finalizer delete is intentionally dropped by the inline synchronization context. At environment teardown the already-disposed context makes the disposal a safe no-op. - runtime-model.md clarifies that calling into JavaScript is forbidden once the context is disposed at teardown; while the context is still live a finalizer action may run (JSValue.CallFinalizeAction opens a runtime scope to invoke it). --- docs/concepts/runtime-model.md | 13 ++++++++----- src/NodeApi/DotNetHost/NativeHost.cs | 12 +++++++----- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/docs/concepts/runtime-model.md b/docs/concepts/runtime-model.md index ec521fc8..569e815e 100644 --- a/docs/concepts/runtime-model.md +++ b/docs/concepts/runtime-model.md @@ -116,15 +116,18 @@ one every module's callbacks resolve. `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 run during teardown — no JS allowed +### Finalizers and teardown — no JS once the context is disposed -A finalizer (for a wrapped .NET object, an external, or a reference) may run while the environment is -being torn down, where **calling into JavaScript is forbidden**. Finalizer code in this library -follows two rules: +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. + 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. diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index 1a939ba8..78e8ba50 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -547,15 +547,17 @@ private hostfxr_handle InitializeManagedRuntime( public void Dispose() { - // Called by the host context when the environment is torn down (the NativeHost is a - // disposable annotation on that context). Runs during environment finalization, where - // calling into JS is forbidden. Notify the managed host (a native call), then close this - // environment's CLR host, and drop the exports reference; the exports napi_ref is reclaimed - // by Node as the env dies. + // Called at environment teardown (the NativeHost is a disposable annotation on the host + // context) and by the explicit JS dispose() hook while the environment is still alive. + // Notify the managed host (a native call), then close this environment's CLR host. NotifyManagedHostEnvironmentFinalize(); CloseRuntimeHost(); _addonGCHandle = default; _onEnvFinalize = default; + + // Release the exports reference. On an explicit dispose() (env alive) this frees its napi_ref + // on the JS thread; at env teardown the disposed context makes it a safe no-op. + _exports?.Dispose(); _exports = null; } From 31e752db8f922ff15ab2136bbbcab5bbf0500961 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 16:55:43 -0700 Subject: [PATCH 18/38] Trim NativeHost.Dispose comments Comment-only: the exports-reference disposal relies on JSReference.Dispose short-circuiting once its context is disposed (env teardown), so no logic change. --- src/NodeApi/DotNetHost/NativeHost.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index 78e8ba50..15a3d43b 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -547,16 +547,14 @@ private hostfxr_handle InitializeManagedRuntime( public void Dispose() { - // Called at environment teardown (the NativeHost is a disposable annotation on the host - // context) and by the explicit JS dispose() hook while the environment is still alive. - // Notify the managed host (a native call), then close this environment's CLR host. + // Called at env teardown (disposable annotation on the host context) and by the JS dispose() hook. NotifyManagedHostEnvironmentFinalize(); CloseRuntimeHost(); _addonGCHandle = default; _onEnvFinalize = default; - // Release the exports reference. On an explicit dispose() (env alive) this frees its napi_ref - // on the JS thread; at env teardown the disposed context makes it a safe no-op. + // JSReference.Dispose no-ops once its context is disposed, so this frees the napi_ref only on + // an explicit dispose() (env alive), never during env-teardown finalization. _exports?.Dispose(); _exports = null; } From 43db3ae895052167b701241888c95e64c2597f2b Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 17:14:17 -0700 Subject: [PATCH 19/38] Contain managed-host scope creation in the failure path; harden stress test - ManagedHost.InitializeModule now opens the runtime scope inside the try and reports failures via a scope-less runtime.ThrowError. A CreateRuntimeScope failure (its lazy TSFN synchronization context can throw) now disposes the context and surfaces a JS error, instead of leaking the rooted context and letting the exception escape the unmanaged entry point. - worker_teardown_stress.js rejects readiness if a worker exits before signaling 'ready', so a silent premature exit fails the test instead of leaving the promise pending and letting the process exit successfully. --- src/NodeApi.DotNetHost/ManagedHost.cs | 15 +++++++-------- .../napi-dotnet/worker_teardown_stress.js | 15 ++++++++++++--- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index 480ccdea..e8f22aeb 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -213,10 +213,13 @@ public static unsafe napi_value InitializeModule( // does not claim the finalizer, and is disposed via the registration notification below. bool hosted = registration != null; JSRuntimeContext context = new(env, runtime); - using JSValueScope scope = JSValueScope.CreateRuntimeScope(env, context); try { + // CreateRuntimeScope lazily builds the sync context and can throw; keep it in the try so + // a failure disposes the context instead of leaking it and escaping this entry point. + using JSValueScope scope = JSValueScope.CreateRuntimeScope(env, context); + JSObject exportsObject = (JSObject)new JSValue(exports, scope); // Save the require() and import() functions that were passed in by the init script. @@ -260,16 +263,12 @@ public static unsafe napi_value InitializeModule( Trace($"Failed to load CLR managed host module: {ex}"); try { - // Report a stackless error: this context is disposed below, so an error carrying - // the usual lazy context-backed `stack` getter would fault when JavaScript later - // reads `stack` (callback dispatch could not resolve the disposed context). - JSError.ThrowError(ex.ToString()); + // Throw via the runtime directly: scope creation may have failed, and the disposed + // context below would make a scope-bound JSError's lazy stack getter unusable. + runtime.ThrowError(env, code: null, ex.ToString()); } finally { - // Failed init: nothing else disposes this context (when hosted, the native host - // never received a teardown callback), so dispose it here to release its slot - // GCHandle, synchronization context, and resolve handlers. context.Dispose(); } } diff --git a/test/TestCases/napi-dotnet/worker_teardown_stress.js b/test/TestCases/napi-dotnet/worker_teardown_stress.js index 724ea1ad..796d1a01 100644 --- a/test/TestCases/napi-dotnet/worker_teardown_stress.js +++ b/test/TestCases/napi-dotnet/worker_teardown_stress.js @@ -26,10 +26,19 @@ if (isMainThread) { // Fail the test on any worker error for the worker's full lifetime -- including during // terminate() -- not just while awaiting readiness (as worker_teardown.js does). worker.on('error', (err) => { throw err; }); - await new Promise((resolve) => { + await new Promise((resolve, reject) => { worker.once('message', (message) => { - assert.strictEqual(message, 'ready'); - resolve(); + try { + assert.strictEqual(message, 'ready'); + resolve(); + } catch (err) { + reject(err); + } + }); + // A worker that exits before signaling 'ready' without an error would otherwise leave this + // promise pending, letting the test process exit successfully after a failed iteration. + worker.once('exit', (code) => { + reject(new Error(`Worker exited before signaling ready (code ${code}).`)); }); }); await worker.terminate(); From 54e0f6041b9687e9eab62240ad15ff241b6e390c Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Aug 2026 17:44:37 -0700 Subject: [PATCH 20/38] Guard native-host init at the boundary; safe slot-clear order; net472 global.json cleanup - NativeHost.InitializeModule creates the context and scope inside the try and reports failures via a scope-less s_jsRuntime.ThrowError, so a fallible instance-data registration or scope creation returns a JS error instead of escaping the unmanaged entry point. - JSRuntimeContext.Dispose frees the rooting GCHandle only after clearing its instance-data slot, so a failed GetInstanceData does not leave the slot pointing at a freed handle for a later FromEnv or finalizer. - TestBuilder deletes any stale per-TFM global.json on the net472 build path so the nested build resolves the repo-root SDK. --- src/NodeApi/DotNetHost/NativeHost.cs | 16 +++++++++------- src/NodeApi/Interop/JSRuntimeContext.cs | 7 +++++-- test/TestBuilder.cs | 6 ++++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index 15a3d43b..387fcf2a 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -204,14 +204,15 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) // dispose() callbacks (dispatched later with no parent scope) recover it via FromEnv. JSRuntimeContext.UseHostContextSlot(); - // The host owns its context (inline, non-TSFN sync context); the transient scope only - // references it and is opened before the try so the catch can still build a JSValue error. - // The context outlives the scope -- rooted by its instance-data slot, disposed by that - // slot's finalizer (which disposes the NativeHost). - JSRuntimeContext context = new(env, s_jsRuntime, new JSInlineSynchronizationContext()); - using JSValueScope hostScope = JSValueScope.CreateRuntimeScope(env, context); try { + // Context creation (fallible instance-data registration) and scope creation are inside + // the try so a failure returns a JS error instead of escaping this unmanaged entry point. + // The context outlives the scope -- rooted by its instance-data slot, disposed by that + // slot's finalizer (which disposes the NativeHost). + JSRuntimeContext context = new(env, s_jsRuntime, new JSInlineSynchronizationContext()); + using JSValueScope hostScope = JSValueScope.CreateRuntimeScope(env, context); + NativeHost host = new(); context.SetDisposableAnnotation(host); @@ -224,7 +225,8 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) { string message = $"Failed to load CLR native host module: {ex}"; Trace(message); - s_jsRuntime.Throw(env, (napi_value)JSValue.CreateError(null, (JSValue)message)); + // Scope-less throw: context or scope creation may have failed, so no scope exists. + s_jsRuntime.ThrowError(env, code: null, message); } Trace("< NativeHost.InitializeModule()"); diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index a8ccf52c..470c4b3b 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -1046,9 +1046,12 @@ public void Dispose() { ((nint*)instanceData)[s_instanceDataSlot] = default; } - } - GCHandle.FromIntPtr(ContextHandle).Free(); + // Free the rooting handle only after clearing its slot; a failed GetInstanceData + // would otherwise leave the slot pointing at a freed handle for a later FromEnv or + // finalizer to dereference. + GCHandle.FromIntPtr(ContextHandle).Free(); + } } } diff --git a/test/TestBuilder.cs b/test/TestBuilder.cs index 8d3ba5cb..966b5c6a 100644 --- a/test/TestBuilder.cs +++ b/test/TestBuilder.cs @@ -256,8 +256,10 @@ private static void WriteCurrentFrameworkGlobalJson( Version frameworkVersion = Environment.Version; if (frameworkVersion.Major == 4) { - // .NET 4.x is supported at runtime, but not at build time. - // So the global.json at the repo root will determine the SDK. + // .NET 4.x builds via the repo-root global.json. Delete any per-TFM global.json another + // TFM's host left in the shared directories so it does not override that. + File.Delete(Path.Combine(workingDirectory, "global.json")); + File.Delete(Path.Combine(Path.GetDirectoryName(projectFilePath)!, "global.json")); return; } From a6edc7970520ebf1d996edcec0beee8dae8ce330 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sat, 29 Aug 2026 18:55:06 -0700 Subject: [PATCH 21/38] Document native-host context finalizer ownership in the init catch The failed-init catch deliberately does not dispose the context: the host-slot context owns the instance-data finalizer that disposes it at env teardown, unlike the managed host's module-slot context whose failure path must dispose. Note this so the host asymmetry is not mistaken for a bug. --- src/NodeApi/DotNetHost/NativeHost.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index 387fcf2a..b674bf92 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -226,6 +226,9 @@ public static napi_value InitializeModule(napi_env env, napi_value exports) string message = $"Failed to load CLR native host module: {ex}"; Trace(message); // Scope-less throw: context or scope creation may have failed, so no scope exists. + // Not disposed here: the host-slot context owns the instance-data finalizer that + // disposes it at env teardown even if partly initialized -- unlike the managed host's + // module-slot context, whose failure path must dispose it. s_jsRuntime.ThrowError(env, code: null, message); } From ca4d8fbe3225d76fb3e685657d31fdd91f273693 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sat, 29 Aug 2026 19:23:53 -0700 Subject: [PATCH 22/38] Guard managed-host and generated-AOT entry points at the boundary Move the fallible JSRuntimeContext creation inside the try in ManagedHost.InitializeModule and dispose it null-tolerantly on failure (the module-slot context is not finalizer-owned, so a failed init must release it). Wrap the generated AOT entry point's context and module-scope setup in a try/catch that reports through a scope-less NodejsRuntime.ThrowError, so instance-data registration or lazy synchronization-context creation can no longer throw across the UnmanagedCallersOnly boundary. Mirrors the native-host boundary guard. --- src/NodeApi.DotNetHost/ManagedHost.cs | 15 +++++++++------ src/NodeApi.Generator/ModuleGenerator.cs | 12 ++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index e8f22aeb..07ffd335 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -212,12 +212,13 @@ public static unsafe napi_value InitializeModule( // environment teardown, so the managed context is a non-owner: it writes its own slot but // does not claim the finalizer, and is disposed via the registration notification below. bool hosted = registration != null; - JSRuntimeContext context = new(env, runtime); + JSRuntimeContext? context = null; try { - // CreateRuntimeScope lazily builds the sync context and can throw; keep it in the try so - // a failure disposes the context instead of leaking it and escaping this entry point. + // Context creation (fallible instance-data registration) and scope creation are inside + // the try so a failure returns a JS error instead of escaping this unmanaged entry point. + context = new(env, runtime); using JSValueScope scope = JSValueScope.CreateRuntimeScope(env, context); JSObject exportsObject = (JSObject)new JSValue(exports, scope); @@ -263,13 +264,15 @@ public static unsafe napi_value InitializeModule( Trace($"Failed to load CLR managed host module: {ex}"); try { - // Throw via the runtime directly: scope creation may have failed, and the disposed - // context below would make a scope-bound JSError's lazy stack getter unusable. + // Throw via the runtime directly: context or scope creation may have failed, and the + // disposed context below would make a scope-bound JSError's lazy stack getter unusable. runtime.ThrowError(env, code: null, ex.ToString()); } finally { - context.Dispose(); + // The module-slot context does not own the instance-data finalizer, so a failed init + // must dispose it here; tolerate construction not having completed. + context?.Dispose(); } } diff --git a/src/NodeApi.Generator/ModuleGenerator.cs b/src/NodeApi.Generator/ModuleGenerator.cs index 2ecf5c68..7fb3a703 100644 --- a/src/NodeApi.Generator/ModuleGenerator.cs +++ b/src/NodeApi.Generator/ModuleGenerator.cs @@ -294,10 +294,22 @@ private SourceBuilder GenerateModuleInitializer( s += $"[UnmanagedCallersOnly(EntryPoint = \"{ModuleRegisterFunctionName}\")]"; s += $"public static napi_value _{ModuleInitializeMethodName}(napi_env env, napi_value exports)"; s += "{"; + // Guard the fallible context/module-scope setup (instance-data registration, lazy + // sync-context creation) so a failure returns a JS error instead of escaping this + // UnmanagedCallersOnly entry point; the catch throws scope-lessly since setup itself failed. + s += "try"; + s += "{"; s += "JSRuntimeContext context = JSRuntimeContext.Create(env);"; s += "using var moduleScope = JSValueScope.CreateModuleScope(env, context);"; s += $"return {ModuleExportsMethodName}(moduleScope, exports);"; s += "}"; + s += "catch (System.Exception ex)"; + s += "{"; + s += "System.Console.Error.WriteLine($\"Failed to initialize module: {ex}\");"; + s += "new Microsoft.JavaScript.NodeApi.Runtime.NodejsRuntime().ThrowError(env, null, ex.ToString());"; + s += "return exports;"; + s += "}"; + s += "}"; s += "#endif"; s++; From e3be21164bb6a241bddf3aec841394c8205accef Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sun, 30 Aug 2026 10:49:44 -0700 Subject: [PATCH 23/38] Reject disposing a value scope off-thread or out of order JSValueScope.Dispose now throws if the scope is not the current scope, before mutating state or calling napi. An off-thread close would call napi_close_*_scope on the wrong thread and leave the owning thread pointing at a disposed scope; an out-of-order close would violate Node-API's LIFO requirement and restore the wrong parent. Adds regression tests for the off-thread and out-of-order disposal cases. --- src/NodeApi/JSValueScope.cs | 17 +++++++++++++++++ test/JSValueScopeTests.cs | 25 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 7cfc087c..597d0f50 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -291,6 +291,23 @@ private JSValueScope(JSValueScopeType scopeType) public void Dispose() { if (IsDisposed) return; + + // A scope must be disposed on its creating thread and in reverse order of creation: an + // off-thread or out-of-order close calls napi on the wrong thread or restores the wrong parent. + if (CurrentOrNull != this) + { + if (CurrentOrNull?._env != _env) + { + throw new JSInvalidThreadAccessException( + currentScope: CurrentOrNull, + targetScope: this, + "A value scope must be disposed on the thread that created it."); + } + + throw new InvalidOperationException( + "A value scope cannot be disposed while a scope created within it is still open."); + } + IsDisposed = true; switch (ScopeType) diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 85ccbc61..c39cd8b4 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -325,6 +325,31 @@ public void SynchronizationContextRejectsLazyCreateWhenContextNotCurrent() contextB.Dispose(); } + [Fact] + public void DisposeScopeWhileNestedScopeOpenThrows() + { + using JSValueScope runtimeScope = TestRuntimeScope(); + JSValueScope handleScope = JSValueScope.CreateHandleScope(); + + // A scope cannot be disposed while a scope created within it is still open (LIFO order). + Assert.Throws(() => runtimeScope.Dispose()); + + // Disposing in the correct reverse order succeeds. + handleScope.Dispose(); + } + + [Fact] + public void DisposeScopeFromDifferentThreadThrows() + { + using JSValueScope runtimeScope = TestRuntimeScope(); + + // A scope must be disposed on the thread that created it, not another thread. + TestUtils.RunInThread(() => + { + Assert.Throws(() => runtimeScope.Dispose()); + }).Wait(); + } + // The module instance is captured through a shared holder: descriptors take the holder during // initialization (before the instance exists) and observe the instance once dispatch assigns it. // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. From ff248c17a3229c0b145b4ca4befc4d66b40291af Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sun, 30 Aug 2026 11:26:17 -0700 Subject: [PATCH 24/38] Guard the embedding boundary; close the native scope on failed setup The embedding callback adapters resolved the context and opened the runtime scope before their try block, so a fallible FromEnv / instance-data registration / synchronization-context setup could throw across the UnmanagedCallersOnly boundary and terminate the process. A new TryEnterRuntimeScope helper performs that setup inside a guarded path and reports failures through the embedding error path. NodeEmbeddingNodeApiScope's constructor now closes the native Node-API scope it opened if the managed setup that follows throws, so a failed construction no longer leaks it. --- src/NodeApi/Runtime/NodeEmbedding.cs | 38 ++++++++++++++----- .../Runtime/NodeEmbeddingNodeApiScope.cs | 14 ++++++- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index 1f2bba5c..d445949b 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -360,6 +360,24 @@ internal static unsafe NodeEmbeddingStatus RuntimeConfigureCallbackAdapter( internal static JSRuntimeContext GetOrCreateContext(napi_env env) => JSRuntimeContext.FromEnv(env) ?? new JSRuntimeContext(env, JSRuntime); + // Opens the runtime scope for an embedding callback inside a guarded path: context resolution + // (instance-data registration) and scope creation are fallible, and every caller is an + // UnmanagedCallersOnly adapter, so a failure is reported through the embedding error path + // rather than escaping the native boundary. Returns null (with the last error set) on failure. + private static JSValueScope? TryEnterRuntimeScope(napi_env env) + { + try + { + JSRuntimeContext context = GetOrCreateContext(env); + return JSValueScope.CreateRuntimeScope(env, context); + } + catch (Exception ex) + { + JSRuntime.EmbeddingSetLastErrorMessage(ex.Message.AsSpan()); + return null; + } + } + #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif @@ -370,8 +388,8 @@ internal static unsafe void RuntimePreloadCallbackAdapter( napi_value process, napi_value require) { - JSRuntimeContext context = GetOrCreateContext(env); - using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); + using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + if (jsValueScope is null) return; try { var callback = (PreloadCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -395,8 +413,8 @@ internal static unsafe napi_value RuntimeLoadingCallbackAdapter( napi_value require, napi_value run_cjs) { - JSRuntimeContext context = GetOrCreateContext(env); - using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); + using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + if (jsValueScope is null) return napi_value.Null; try { var callback = (LoadingCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -420,8 +438,8 @@ internal static unsafe void RuntimeLoadedCallbackAdapter( napi_env env, napi_value loading_result) { - JSRuntimeContext context = GetOrCreateContext(env); - using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); + using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + if (jsValueScope is null) return; try { var callback = (LoadedCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -444,8 +462,8 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( nint module_name, napi_value exports) { - JSRuntimeContext context = GetOrCreateContext(env); - using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); + using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + if (jsValueScope is null) return napi_value.Null; try { var callback = (InitializeModuleCallback)GCHandle.FromIntPtr(cb_data).Target!; @@ -513,8 +531,8 @@ internal static unsafe NodeEmbeddingStatus TaskPostCallbackAdapter( #endif internal static unsafe void NodeApiRunCallbackAdapter(nint cb_data, napi_env env) { - JSRuntimeContext context = GetOrCreateContext(env); - using var jsValueScope = JSValueScope.CreateRuntimeScope(env, context); + using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + if (jsValueScope is null) return; try { var callback = (RunNodeApiCallback)GCHandle.FromIntPtr(cb_data).Target!; diff --git a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs index 9526b40c..8f7367bb 100644 --- a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs +++ b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs @@ -20,8 +20,18 @@ public NodeEmbeddingNodeApiScope(NodeEmbeddingRuntime runtime) NodeEmbedding.JSRuntime.EmbeddingRuntimeOpenNodeApiScope( runtime.Handle, out _nodeApiScope, out napi_env env) .ThrowIfFailed(); - JSRuntimeContext context = NodeEmbedding.GetOrCreateContext(env); - _valueScope = JSValueScope.CreateRuntimeScope(env, context); + try + { + JSRuntimeContext context = NodeEmbedding.GetOrCreateContext(env); + _valueScope = JSValueScope.CreateRuntimeScope(env, context); + } + catch + { + // A throwing constructor cannot be disposed, so close the native scope opened above + // before rethrowing, or it would leak for the lifetime of the embedding runtime. + NodeEmbedding.JSRuntime.EmbeddingRuntimeCloseNodeApiScope(runtime.Handle, _nodeApiScope); + throw; + } } /// From 3de5b9822555c72052f69851baed6a887838256a Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sun, 30 Aug 2026 11:47:47 -0700 Subject: [PATCH 25/38] Clear the context's instance-data slot only if it still owns it JSRuntimeContext.Dispose cleared the env instance-data slot unconditionally, so disposing an older context that a newer context had replaced for the same env unregistered the live context: FromEnv could no longer resolve it and its env instance-data finalizer could not dispose it. Now the slot is cleared only when it still holds this context's handle; the rooting GCHandle is freed regardless. Adds a regression test. --- src/NodeApi/Interop/JSRuntimeContext.cs | 14 ++++++++++---- test/JSValueScopeTests.cs | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 470c4b3b..dc699678 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -1044,12 +1044,18 @@ public void Dispose() { unsafe { - ((nint*)instanceData)[s_instanceDataSlot] = default; + // Clear the slot only if it still holds this context; a newer context created + // for the same env may have replaced it, and clearing then would unregister the + // live context so FromEnv and its env finalizer could no longer reach it. + if (((nint*)instanceData)[s_instanceDataSlot] == ContextHandle) + { + ((nint*)instanceData)[s_instanceDataSlot] = default; + } } - // Free the rooting handle only after clearing its slot; a failed GetInstanceData - // would otherwise leave the slot pointing at a freed handle for a later FromEnv or - // finalizer to dereference. + // Free this context's now-unregistered rooting handle. If the slot pointed here it + // was cleared above, so no later FromEnv or finalizer can dereference the freed + // handle; if a newer context replaced it, that context still owns the slot. GCHandle.FromIntPtr(ContextHandle).Free(); } } diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index c39cd8b4..c95c99c9 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -350,6 +350,24 @@ public void DisposeScopeFromDifferentThreadThrows() }).Wait(); } + [Fact] + public void DisposingReplacedContextKeepsNewerContextRegistered() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var contextA = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + var contextB = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + // contextB replaced contextA in the env instance-data slot; disposing the older contextA + // must not clear contextB's registration. + contextA.Dispose(); + Assert.Same(contextB, JSRuntimeContext.FromEnv(env)); + + contextB.Dispose(); + Assert.Null(JSRuntimeContext.FromEnv(env)); + } + // The module instance is captured through a shared holder: descriptors take the holder during // initialization (before the instance exists) and observe the instance once dispatch assigns it. // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. From cf1b737fcabadb3f5a0ba13905974d0085d2dc8e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sun, 30 Aug 2026 12:14:03 -0700 Subject: [PATCH 26/38] Guard teardown cleanup in finally so a failure can't strand state Two teardown paths could leave half-torn-down state if a fallible step threw. NativeHost.Dispose now clears the managed-host registration in a finally around the environment-finalize notification, before the fallible CloseRuntimeHost, so a retry can't re-invoke the callback with an already-freed GCHandle across the unmanaged boundary. JSRuntimeContext.Dispose now releases its instance-data slot and rooting GCHandle from a finally, so a throwing earlier teardown step (for example synchronization-context disposal) no longer skips the root release and leaks the context and shared block (a retry no-ops because IsDisposed is already set). Adds a regression test. --- src/NodeApi/DotNetHost/NativeHost.cs | 16 +++- src/NodeApi/Interop/JSRuntimeContext.cs | 101 +++++++++++++----------- test/JSValueScopeTests.cs | 19 +++++ 3 files changed, 85 insertions(+), 51 deletions(-) diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index b674bf92..f6273faa 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -553,10 +553,20 @@ private hostfxr_handle InitializeManagedRuntime( public void Dispose() { // Called at env teardown (disposable annotation on the host context) and by the JS dispose() hook. - NotifyManagedHostEnvironmentFinalize(); + try + { + NotifyManagedHostEnvironmentFinalize(); + } + finally + { + // Clear the registration before the fallible CloseRuntimeHost: the notification frees the + // addon GCHandle, so a later retry that re-invoked it would pass an already-freed handle + // across the unmanaged finalizer boundary. + _addonGCHandle = default; + _onEnvFinalize = default; + } + CloseRuntimeHost(); - _addonGCHandle = default; - _onEnvFinalize = default; // JSReference.Dispose no-ops once its context is disposed, so this frees the napi_ref only on // an explicit dispose() (env alive), never during env-teardown finalization. diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index dc699678..35eac2f0 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -989,74 +989,79 @@ public void Dispose() IsDisposed = true; - // Dispose an already-created sync context only; never construct one here. Disposal can run - // during env finalization when no scope is current, and creating a sync context then would - // throw and skip the rest of teardown. - _synchronizationContext?.Dispose(); + try + { + // Dispose an already-created sync context only; never construct one here. Disposal can + // run during env finalization when no scope is current, and creating a sync context then + // would throw and skip the rest of teardown. + _synchronizationContext?.Dispose(); #if !(NETFRAMEWORK || NETSTANDARD) - // ConditionalWeakTable<> is not enumerable in .NET Framework. - // The JS references will still be released eventually by their finalizers. - DisposeReferences(_objectMap.Select((entry) => entry.Value)); + // ConditionalWeakTable<> is not enumerable in .NET Framework. + // The JS references will still be released eventually by their finalizers. + DisposeReferences(_objectMap.Select((entry) => entry.Value)); #endif - DisposeReferences(_classMap.Values); - DisposeReferences(_staticClassMap.Values); - DisposeReferences(_structMap.Values); + DisposeReferences(_classMap.Values); + DisposeReferences(_staticClassMap.Values); + DisposeReferences(_structMap.Values); - // Disposed after IsDisposed is set, so a late cross-thread post is already a no-op. - if (_moduleDisposables != null) - { - foreach (IDisposable moduleDisposable in _moduleDisposables) + // Disposed after IsDisposed is set, so a late cross-thread post is already a no-op. + if (_moduleDisposables != null) { - try - { - moduleDisposable.Dispose(); - } - catch + foreach (IDisposable moduleDisposable in _moduleDisposables) { - // A failing module disposal must not prevent the rest of teardown. + try + { + moduleDisposable.Dispose(); + } + catch + { + // A failing module disposal must not prevent the rest of teardown. + } } } - } - if (_disposableAnnotations != null) - { - foreach (IDisposable annotation in _disposableAnnotations.Values) + if (_disposableAnnotations != null) { - try - { - annotation.Dispose(); - } - catch + foreach (IDisposable annotation in _disposableAnnotations.Values) { - // A failing annotation must not prevent the rest of teardown. + try + { + annotation.Dispose(); + } + catch + { + // A failing annotation must not prevent the rest of teardown. + } } } } - - // Remove this context's root so it can be collected: clear its instance-data slot (a - // concurrent FromEnv then resolves no context) and free the rooting GCHandle. The shared - // block itself is freed by FinalizeInstanceData once every context on the env is gone. - if (ContextHandle != default) + finally { - Runtime.GetInstanceData(UncheckedEnvironmentHandle, out nint instanceData); - if (instanceData != default) + // Release this context's root even if an earlier teardown step threw, or the context root + // and shared instance-data block would leak (a retry no-ops because IsDisposed is already + // set). Clear the instance-data slot and free the rooting GCHandle. + if (ContextHandle != default) { - unsafe + Runtime.GetInstanceData(UncheckedEnvironmentHandle, out nint instanceData); + if (instanceData != default) { - // Clear the slot only if it still holds this context; a newer context created - // for the same env may have replaced it, and clearing then would unregister the - // live context so FromEnv and its env finalizer could no longer reach it. - if (((nint*)instanceData)[s_instanceDataSlot] == ContextHandle) + unsafe { - ((nint*)instanceData)[s_instanceDataSlot] = default; + // Clear the slot only if it still holds this context; a newer context created + // for the same env may have replaced it, and clearing then would unregister + // the live context so FromEnv and its env finalizer could no longer reach it. + if (((nint*)instanceData)[s_instanceDataSlot] == ContextHandle) + { + ((nint*)instanceData)[s_instanceDataSlot] = default; + } } - } - // Free this context's now-unregistered rooting handle. If the slot pointed here it - // was cleared above, so no later FromEnv or finalizer can dereference the freed - // handle; if a newer context replaced it, that context still owns the slot. - GCHandle.FromIntPtr(ContextHandle).Free(); + // Free this context's now-unregistered rooting handle. If the slot pointed here it + // was cleared above, so no later FromEnv or finalizer can dereference the freed + // handle; if a newer context replaced it, that context still owns the slot. + GCHandle.FromIntPtr(ContextHandle).Free(); + } } } } diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index c95c99c9..d19fb905 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -368,6 +368,25 @@ public void DisposingReplacedContextKeepsNewerContextRegistered() Assert.Null(JSRuntimeContext.FromEnv(env)); } + [Fact] + public void ContextRootReleasedWhenTeardownStepThrows() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext(env, _mockRuntime, new ThrowingSynchronizationContext()); + + // The sync context's Dispose throws, but the slot/root release runs from a finally, so the + // context is still unregistered (otherwise its env finalizer could never reclaim it). + Assert.Throws(() => context.Dispose()); + Assert.Null(JSRuntimeContext.FromEnv(env)); + } + + private sealed class ThrowingSynchronizationContext : JSSynchronizationContext + { + public override void Dispose() => throw new InvalidOperationException("teardown failure"); + public override void OpenAsyncScope() { } + public override void CloseAsyncScope() { } + } + // The module instance is captured through a shared holder: descriptors take the holder during // initialization (before the instance exists) and observe the instance once dispatch assigns it. // Nested handle/escapable scopes inherit the same holder, so Current.Module round-trips through it. From 89fc05b13cb082a12a79ad07f36778ad2d75a6a1 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Sun, 30 Aug 2026 12:43:23 -0700 Subject: [PATCH 27/38] Run all context teardown phases and guard host cleanup in finally Round-12's JSRuntimeContext.Dispose finally released the root, but a throwing early phase (for example synchronization-context disposal) jumped straight to the finally, skipping module-disposable and annotation cleanup. Dispose now runs every teardown phase independently, capturing failures, releases the root regardless, then rethrows the first failure -- so a throwing phase can no longer strand [JSModule] instances or resolver handlers while IsDisposed blocks a retry. ManagedHost.Dispose(bool) likewise unsubscribes the process-wide resolve handlers and calls base.Dispose from finally blocks, so a throwing context disposal cannot leave the host rooted. Extends the regression test to assert a module disposable still runs when the sync context's Dispose throws. --- src/NodeApi.DotNetHost/ManagedHost.cs | 47 ++++++---- src/NodeApi/Interop/JSRuntimeContext.cs | 112 ++++++++++++------------ test/JSValueScopeTests.cs | 7 +- 3 files changed, 89 insertions(+), 77 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index 07ffd335..e5c9b87c 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -698,29 +698,42 @@ protected override void Dispose(bool disposing) if (_isDisposed) return; _isDisposed = true; - if (disposing) + try { - // The context disposes this host (a disposable annotation) at teardown, so the - // re-entrant context dispose here is a guarded no-op. Unsubscribe the process-wide - // resolve handlers so a torn-down environment's host is not left rooted by them. - _context?.Dispose(); - _context = null; - + if (disposing) + { + try + { + // The context disposes this host (a disposable annotation) at teardown, so this + // re-entrant dispose is a guarded no-op; on an explicit dispose it runs the + // context teardown, which can throw. + _context?.Dispose(); + _context = null; + } + finally + { + // Unsubscribe the process-wide resolve handlers even if context disposal threw, + // so a torn-down environment's host is not left rooted (a retry no-ops on + // _isDisposed). #if NETFRAMEWORK || NETSTANDARD - AppDomain.CurrentDomain.AssemblyResolve -= OnResolvingAssembly; + AppDomain.CurrentDomain.AssemblyResolve -= OnResolvingAssembly; #else - AssemblyLoadContext.Default.Resolving -= OnResolvingAssembly; - _loadContext.Resolving -= OnResolvingAssembly; + AssemblyLoadContext.Default.Resolving -= OnResolvingAssembly; + _loadContext.Resolving -= OnResolvingAssembly; - // A non-collectible load context cannot be unloaded; only unload one created collectible. - if (_loadContext.IsCollectible) - { - _loadContext.Unload(); - } + // A non-collectible load context cannot be unloaded; only unload one created collectible. + if (_loadContext.IsCollectible) + { + _loadContext.Unload(); + } #endif + } + } + } + finally + { + base.Dispose(disposing); } - - base.Dispose(disposing); } #if NETSTANDARD diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 35eac2f0..714b6247 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -8,6 +8,7 @@ using System.IO; using System.Linq; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; using System.Threading; using System.Threading.Tasks; @@ -989,81 +990,76 @@ public void Dispose() IsDisposed = true; - try - { - // Dispose an already-created sync context only; never construct one here. Disposal can - // run during env finalization when no scope is current, and creating a sync context then - // would throw and skip the rest of teardown. - _synchronizationContext?.Dispose(); + // Run every teardown phase even if an earlier one throws, then release the context root and + // rethrow the first failure. A throwing phase must not skip a later phase or the root + // release: IsDisposed is already set, so a retry returns immediately and could otherwise + // strand module instances, annotations, or the context root and its shared block. + ExceptionDispatchInfo? firstFailure = null; + + // Dispose an already-created sync context only; never construct one here. Disposal can run + // during env finalization when no scope is current, and creating one then would throw. + try { _synchronizationContext?.Dispose(); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } #if !(NETFRAMEWORK || NETSTANDARD) - // ConditionalWeakTable<> is not enumerable in .NET Framework. - // The JS references will still be released eventually by their finalizers. - DisposeReferences(_objectMap.Select((entry) => entry.Value)); + // ConditionalWeakTable<> is not enumerable in .NET Framework; those references are released + // by their finalizers instead. + try { DisposeReferences(_objectMap.Select((entry) => entry.Value)); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } #endif - DisposeReferences(_classMap.Values); - DisposeReferences(_staticClassMap.Values); - DisposeReferences(_structMap.Values); - - // Disposed after IsDisposed is set, so a late cross-thread post is already a no-op. - if (_moduleDisposables != null) + try { DisposeReferences(_classMap.Values); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } + try { DisposeReferences(_staticClassMap.Values); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } + try { DisposeReferences(_structMap.Values); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } + + // Disposed after IsDisposed is set, so a late cross-thread post is already a no-op. Each item + // is guarded so one failure does not skip the rest. + if (_moduleDisposables != null) + { + foreach (IDisposable moduleDisposable in _moduleDisposables) { - foreach (IDisposable moduleDisposable in _moduleDisposables) - { - try - { - moduleDisposable.Dispose(); - } - catch - { - // A failing module disposal must not prevent the rest of teardown. - } - } + try { moduleDisposable.Dispose(); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } } + } - if (_disposableAnnotations != null) + if (_disposableAnnotations != null) + { + foreach (IDisposable annotation in _disposableAnnotations.Values) { - foreach (IDisposable annotation in _disposableAnnotations.Values) - { - try - { - annotation.Dispose(); - } - catch - { - // A failing annotation must not prevent the rest of teardown. - } - } + try { annotation.Dispose(); } + catch (Exception ex) { firstFailure ??= ExceptionDispatchInfo.Capture(ex); } } } - finally + + // Release this context's root so it can be collected, even if a phase above threw. The + // shared block is freed by FinalizeInstanceData once every context on the env is gone. + if (ContextHandle != default) { - // Release this context's root even if an earlier teardown step threw, or the context root - // and shared instance-data block would leak (a retry no-ops because IsDisposed is already - // set). Clear the instance-data slot and free the rooting GCHandle. - if (ContextHandle != default) + Runtime.GetInstanceData(UncheckedEnvironmentHandle, out nint instanceData); + if (instanceData != default) { - Runtime.GetInstanceData(UncheckedEnvironmentHandle, out nint instanceData); - if (instanceData != default) + unsafe { - unsafe + // Clear the slot only if it still holds this context; a newer context for the + // same env may have replaced it (clearing then would unregister the live one). + if (((nint*)instanceData)[s_instanceDataSlot] == ContextHandle) { - // Clear the slot only if it still holds this context; a newer context created - // for the same env may have replaced it, and clearing then would unregister - // the live context so FromEnv and its env finalizer could no longer reach it. - if (((nint*)instanceData)[s_instanceDataSlot] == ContextHandle) - { - ((nint*)instanceData)[s_instanceDataSlot] = default; - } + ((nint*)instanceData)[s_instanceDataSlot] = default; } - - // Free this context's now-unregistered rooting handle. If the slot pointed here it - // was cleared above, so no later FromEnv or finalizer can dereference the freed - // handle; if a newer context replaced it, that context still owns the slot. - GCHandle.FromIntPtr(ContextHandle).Free(); } + + // Free this context's now-unregistered rooting handle. If the slot pointed here it + // was cleared above, so no later FromEnv or finalizer can dereference the freed + // handle; if a newer context replaced it, that context still owns the slot. + GCHandle.FromIntPtr(ContextHandle).Free(); } } + + // Surface the first teardown failure now that every phase has run and the root is released. + firstFailure?.Throw(); } private static void DisposeReferences( diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index d19fb905..0e1d445b 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -373,10 +373,13 @@ public void ContextRootReleasedWhenTeardownStepThrows() { napi_env env = new(Environment.CurrentManagedThreadId); var context = new JSRuntimeContext(env, _mockRuntime, new ThrowingSynchronizationContext()); + var module = new DisposableModule(); + context.AddModuleDisposable(module); - // The sync context's Dispose throws, but the slot/root release runs from a finally, so the - // context is still unregistered (otherwise its env finalizer could never reclaim it). + // The sync context's Dispose throws, but every later teardown phase still runs: the module + // disposable is disposed and the context root is released. The failure surfaces after cleanup. Assert.Throws(() => context.Dispose()); + Assert.Equal(1, module.DisposeCount); Assert.Null(JSRuntimeContext.FromEnv(env)); } From 8c1205bbd6e9c068f74e9062949db0d309e9b45c Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 31 Aug 2026 10:49:27 -0700 Subject: [PATCH 28/38] Associate a napi_env with a runtime context exactly once RegisterInstanceData now rejects a second registration on a slot -- whether it holds a live context or a tombstone left after disposal -- so a napi_env is bound to a single JSRuntimeContext for its lifetime and can never be re-associated. Dispose tombstones the slot instead of clearing it; FromEnv and FinalizeInstanceData treat the tombstone as no live context. This dissolves the class of bugs where a replacement context on the same env corrupted the original's registration or GCHandle bookkeeping: a finalizer resolving FromEnv now always gets the single allocating context, or null once it is disposed. Also guards the fallible runtime-scope creation in JSValue.InvokeCallback and the tracing callbacks with a scope-less error path, so a context that cannot be resolved (for example a retained JS function invoked after teardown) returns a JS error instead of escaping the unmanaged callback boundary. Rewrites the two unit tests that relied on two contexts sharing one env. --- src/NodeApi/Interop/JSRuntimeContext.cs | 35 ++++++++---- src/NodeApi/JSValue.cs | 22 +++++++- src/NodeApi/Runtime/TracingJSRuntime.cs | 74 ++++++++++++++----------- test/JSValueScopeTests.cs | 27 +++++---- 4 files changed, 101 insertions(+), 57 deletions(-) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 714b6247..ffcb8efa 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -129,6 +129,12 @@ public sealed class JSRuntimeContext : IDisposable private const int HostContextSlot = 1; private const int InstanceDataSlotCount = 2; + // Written into a slot when its context is disposed, so an env is associated with a runtime + // context exactly once: RegisterInstanceData rejects a non-empty slot (a live handle or this + // tombstone), and FromEnv/FinalizeInstanceData treat the tombstone as "no live context". A real + // GCHandle is never -1. + private static readonly nint s_disposedSlot = -1; + // This runtime's slot in the instance-data block: the module slot by default, or the host slot // once the native host calls UseHostContextSlot() at startup. private static int s_instanceDataSlot = ModuleContextSlot; @@ -198,7 +204,7 @@ public static explicit operator napi_env(JSRuntimeContext context) } nint slotHandle = ((nint*)instanceData)[s_instanceDataSlot]; - if (slotHandle == default) + if (slotHandle == default || slotHandle == s_disposedSlot) { return null; } @@ -333,6 +339,14 @@ private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) status.ThrowIfFailed(); } } + else if (((nint*)instanceData)[s_instanceDataSlot] != default) + { + // An env is associated with a runtime context exactly once. The slot holds a live + // context's handle, or a tombstone after one was disposed; either way a second + // association is rejected rather than silently replacing the first. + throw new InvalidOperationException( + "The environment is already associated with a runtime context."); + } ((nint*)instanceData)[s_instanceDataSlot] = ContextHandle; } @@ -354,7 +368,7 @@ private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hi // runtime's slot is read, never the other runtime's (whose GCHandle belongs to a separate // GC heap). nint slotHandle = ((nint*)data)[s_instanceDataSlot]; - if (slotHandle != default) + if (slotHandle != default && slotHandle != s_disposedSlot) { try { @@ -366,13 +380,14 @@ private static unsafe void FinalizeInstanceData(napi_env env, nint data, nint hi } } - // Free the shared block once the last context on the env is gone (all slots cleared); + // Free the shared block once no slot holds a live context (each is empty or tombstoned); // disposing a host context cascades synchronously to the other slot. Do not null it out // via napi_set_instance_data: that deletes this very TrackedFinalizer, which Node then // deletes again (double free). for (int i = 0; i < InstanceDataSlotCount; i++) { - if (((nint*)data)[i] != default) + nint slot = ((nint*)data)[i]; + if (slot != default && slot != s_disposedSlot) { return; } @@ -1043,17 +1058,17 @@ public void Dispose() { unsafe { - // Clear the slot only if it still holds this context; a newer context for the - // same env may have replaced it (clearing then would unregister the live one). + // Tombstone the slot (not empty) so the env can never re-associate a new context + // (see RegisterInstanceData). The one-context invariant means the slot still + // holds this context, but stay defensive. if (((nint*)instanceData)[s_instanceDataSlot] == ContextHandle) { - ((nint*)instanceData)[s_instanceDataSlot] = default; + ((nint*)instanceData)[s_instanceDataSlot] = s_disposedSlot; } } - // Free this context's now-unregistered rooting handle. If the slot pointed here it - // was cleared above, so no later FromEnv or finalizer can dereference the freed - // handle; if a newer context replaced it, that context still owns the slot. + // Free this context's now-unregistered rooting handle. The slot was tombstoned above, + // so no later FromEnv or finalizer can dereference the freed handle. GCHandle.FromIntPtr(ContextHandle).Free(); } } diff --git a/src/NodeApi/JSValue.cs b/src/NodeApi/JSValue.cs index 8536f95e..9622fef2 100644 --- a/src/NodeApi/JSValue.cs +++ b/src/NodeApi/JSValue.cs @@ -1260,9 +1260,21 @@ private static unsafe napi_value InvokeCallback( napi_callback_info callbackInfo, Func getCallbackDescriptor) { - // The scope references the context inherited from the parent scope, or -- when the native - // host dispatches a callback with no scope on the thread -- recovered from env instance data. - using var scope = JSValueScope.CreateRuntimeScope(env); + JSValueScope scope; + try + { + // The scope references the context inherited from the parent scope, or -- when the native + // host dispatches a callback with no scope on the thread -- recovered from env instance data. + scope = JSValueScope.CreateRuntimeScope(env); + } + catch (Exception ex) + { + // Scope-less throw: no context resolved (e.g. a retained JS function invoked after its + // context was disposed), so there is no scope in which to build a JSError. + new NodejsRuntime().ThrowError(env, code: null, ex.ToString()); + return napi_value.Null; + } + try { JSCallbackArgs.GetDataAndLength(scope, callbackInfo, out object? data, out int length); @@ -1277,6 +1289,10 @@ private static unsafe napi_value InvokeCallback( JSError.ThrowError(ex); return napi_value.Null; } + finally + { + scope.Dispose(); + } } #if UNMANAGED_DELEGATES diff --git a/src/NodeApi/Runtime/TracingJSRuntime.cs b/src/NodeApi/Runtime/TracingJSRuntime.cs index 1da7695b..95e7b543 100644 --- a/src/NodeApi/Runtime/TracingJSRuntime.cs +++ b/src/NodeApi/Runtime/TracingJSRuntime.cs @@ -382,57 +382,67 @@ private static readonly unsafe delegate* unmanaged[Cdecl] private static JSValueScope CreateCallbackScope(napi_env env) => JSValueScope.CreateRuntimeScope(env); + // Guards the fallible scope creation like JSValue.InvokeCallback: a failure to resolve a context + // (for example a retained JS function invoked after teardown) is reported scope-lessly instead of + // escaping the UnmanagedCallersOnly callbacks below. + private static napi_value InvokeTraceCallback( + napi_env env, + napi_callback_info cbinfo, + Func getCallbackDescriptor) + { + JSValueScope scope; + try + { + scope = CreateCallbackScope(env); + } + catch (Exception ex) + { + new NodejsRuntime().ThrowError(env, code: null, ex.ToString()); + return napi_value.Null; + } + + using (scope) + { + return ((TracingJSRuntime)scope.Runtime).TraceCallback( + scope, cbinfo, getCallbackDescriptor); + } + } + #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif private static unsafe napi_value TraceFunctionCallback(napi_env env, napi_callback_info cbinfo) - { - using JSValueScope scope = CreateCallbackScope(env); - return ((TracingJSRuntime)scope.Runtime).TraceCallback( - scope, cbinfo, (descriptor) => descriptor); - } + => InvokeTraceCallback(env, cbinfo, (descriptor) => descriptor); #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif private static unsafe napi_value TraceMethodCallback(napi_env env, napi_callback_info cbinfo) - { - using JSValueScope scope = CreateCallbackScope(env); - return ((TracingJSRuntime)scope.Runtime).TraceCallback( - scope, cbinfo, (propertyDescriptor) => new( - propertyDescriptor.Name, - propertyDescriptor.Method!, - propertyDescriptor.Data, - propertyDescriptor.ModuleHolder)); - } + => InvokeTraceCallback(env, cbinfo, (propertyDescriptor) => new( + propertyDescriptor.Name, + propertyDescriptor.Method!, + propertyDescriptor.Data, + propertyDescriptor.ModuleHolder)); #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif private static unsafe napi_value TraceGetterCallback(napi_env env, napi_callback_info cbinfo) - { - using JSValueScope scope = CreateCallbackScope(env); - return ((TracingJSRuntime)scope.Runtime).TraceCallback( - scope, cbinfo, (propertyDescriptor) => new( - propertyDescriptor.Name, - propertyDescriptor.Getter!, - propertyDescriptor.Data, - propertyDescriptor.ModuleHolder)); - } + => InvokeTraceCallback(env, cbinfo, (propertyDescriptor) => new( + propertyDescriptor.Name, + propertyDescriptor.Getter!, + propertyDescriptor.Data, + propertyDescriptor.ModuleHolder)); #if UNMANAGED_DELEGATES [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] #endif private static unsafe napi_value TraceSetterCallback(napi_env env, napi_callback_info cbinfo) - { - using JSValueScope scope = CreateCallbackScope(env); - return ((TracingJSRuntime)scope.Runtime).TraceCallback( - scope, cbinfo, (propertyDescriptor) => new( - propertyDescriptor.Name, - propertyDescriptor.Setter!, - propertyDescriptor.Data, - propertyDescriptor.ModuleHolder)); - } + => InvokeTraceCallback(env, cbinfo, (propertyDescriptor) => new( + propertyDescriptor.Name, + propertyDescriptor.Setter!, + propertyDescriptor.Data, + propertyDescriptor.ModuleHolder)); /// /// Traces a callback function, method, getter, or setter, including args and return value. diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 0e1d445b..7a019ace 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -307,9 +307,12 @@ public void EnterDisposedRuntimeContextThrows() public void SynchronizationContextRejectsLazyCreateWhenContextNotCurrent() { napi_env env = new(Environment.CurrentManagedThreadId); - var contextA = new JSRuntimeContext(env, _mockRuntime); // no sync context -> lazy + + // Separate runtimes so each context has its own instance data (an env is associated with a + // single context), letting contextB be current while contextA is not. + var contextA = new JSRuntimeContext(env, new MockJSRuntime()); // no sync context -> lazy var contextB = new JSRuntimeContext( - env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); using (JSValueScope.CreateRuntimeScope(env, contextB)) { @@ -351,21 +354,21 @@ public void DisposeScopeFromDifferentThreadThrows() } [Fact] - public void DisposingReplacedContextKeepsNewerContextRegistered() + public void RegisteringSecondContextOnEnvIsRejected() { napi_env env = new(Environment.CurrentManagedThreadId); - var contextA = new JSRuntimeContext( - env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); - var contextB = new JSRuntimeContext( + var context = new JSRuntimeContext( env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); - // contextB replaced contextA in the env instance-data slot; disposing the older contextA - // must not clear contextB's registration. - contextA.Dispose(); - Assert.Same(contextB, JSRuntimeContext.FromEnv(env)); + // An env is associated with a runtime context exactly once. + Assert.Throws( + () => new JSRuntimeContext(env, _mockRuntime, new MockJSRuntime.SynchronizationContext())); - contextB.Dispose(); - Assert.Null(JSRuntimeContext.FromEnv(env)); + context.Dispose(); + + // Even after disposal the env cannot be re-associated (the slot is tombstoned). + Assert.Throws( + () => new JSRuntimeContext(env, _mockRuntime, new MockJSRuntime.SynchronizationContext())); } [Fact] From 3632a47d0f65e3a3024a28e9690c5c97d79a079c Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 31 Aug 2026 13:33:10 -0700 Subject: [PATCH 29/38] Fix RuntimeScope creation --- src/NodeApi/JSValue.cs | 21 ++++++------------- src/NodeApi/JSValueScope.cs | 13 ++++++++++++ src/NodeApi/Runtime/TracingJSRuntime.cs | 27 +++++++------------------ test/JSReferenceTests.cs | 11 +++++++++- test/JSValueScopeTests.cs | 9 ++++++--- 5 files changed, 42 insertions(+), 39 deletions(-) diff --git a/src/NodeApi/JSValue.cs b/src/NodeApi/JSValue.cs index 9622fef2..c8f11c8d 100644 --- a/src/NodeApi/JSValue.cs +++ b/src/NodeApi/JSValue.cs @@ -1260,18 +1260,13 @@ private static unsafe napi_value InvokeCallback( napi_callback_info callbackInfo, Func getCallbackDescriptor) { - JSValueScope scope; - try - { - // The scope references the context inherited from the parent scope, or -- when the native - // host dispatches a callback with no scope on the thread -- recovered from env instance data. - scope = JSValueScope.CreateRuntimeScope(env); - } - catch (Exception ex) + // The scope references the context inherited from the parent scope, or -- when the native + // host dispatches a callback with no scope on the thread -- recovered from env instance data. + // A retained JS function invoked after its context was disposed resolves none, so the call + // is a no-op instead of throwing across the unmanaged boundary. + using JSValueScope? scope = JSValueScope.TryCreateRuntimeScope(env); + if (scope is null) { - // Scope-less throw: no context resolved (e.g. a retained JS function invoked after its - // context was disposed), so there is no scope in which to build a JSError. - new NodejsRuntime().ThrowError(env, code: null, ex.ToString()); return napi_value.Null; } @@ -1289,10 +1284,6 @@ private static unsafe napi_value InvokeCallback( JSError.ThrowError(ex); return napi_value.Null; } - finally - { - scope.Dispose(); - } } #if UNMANAGED_DELEGATES diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 597d0f50..8d5bb1ed 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -147,6 +147,19 @@ public static JSValueScope CreateRuntimeScope( napi_env env = default, JSRuntimeContext? context = null) => new(env, context); + /// + /// Attempts to create a scope for a callback + /// entering from JS, returning null when no live context can be resolved for the environment -- + /// for example a retained JS function invoked after its context was disposed. Callback adapters + /// use it to become a no-op in that case rather than let an exception cross the unmanaged + /// boundary. Genuine misuse (wrong thread or mismatched environment) still throws. + /// + internal static JSValueScope? TryCreateRuntimeScope(napi_env env) + { + JSRuntimeContext? context = CurrentOrNull?.RuntimeContext ?? JSRuntimeContext.FromEnv(env); + return context is { IsDisposed: false } ? new JSValueScope(env, context) : null; + } + /// /// Creates a scope that starts a fresh module /// boundary: it references the same (inherited or supplied) but diff --git a/src/NodeApi/Runtime/TracingJSRuntime.cs b/src/NodeApi/Runtime/TracingJSRuntime.cs index 95e7b543..e24f1946 100644 --- a/src/NodeApi/Runtime/TracingJSRuntime.cs +++ b/src/NodeApi/Runtime/TracingJSRuntime.cs @@ -377,35 +377,22 @@ private static readonly unsafe delegate* unmanaged[Cdecl] s_traceSetterCallback = &TraceSetterCallback; #endif - // Like InvokeCallback (which these replace when tracing is on), the scope references the context - // inherited from the parent scope, or recovered from env instance data when there is none. - private static JSValueScope CreateCallbackScope(napi_env env) - => JSValueScope.CreateRuntimeScope(env); - - // Guards the fallible scope creation like JSValue.InvokeCallback: a failure to resolve a context - // (for example a retained JS function invoked after teardown) is reported scope-lessly instead of - // escaping the UnmanagedCallersOnly callbacks below. + // Like JSValue.InvokeCallback (which these replace when tracing is on): a callback that can no + // longer resolve a context -- for example a retained JS function invoked after its context was + // disposed -- is a no-op instead of throwing across the UnmanagedCallersOnly callbacks below. private static napi_value InvokeTraceCallback( napi_env env, napi_callback_info cbinfo, Func getCallbackDescriptor) { - JSValueScope scope; - try - { - scope = CreateCallbackScope(env); - } - catch (Exception ex) + using JSValueScope? scope = JSValueScope.TryCreateRuntimeScope(env); + if (scope is null) { - new NodejsRuntime().ThrowError(env, code: null, ex.ToString()); return napi_value.Null; } - using (scope) - { - return ((TracingJSRuntime)scope.Runtime).TraceCallback( - scope, cbinfo, getCallbackDescriptor); - } + return ((TracingJSRuntime)scope.Runtime).TraceCallback( + scope, cbinfo, getCallbackDescriptor); } #if UNMANAGED_DELEGATES diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index 49cb89a8..620eab19 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -23,6 +23,14 @@ private JSValueScope TestScope(JSSynchronizationContext synchronizationContext) return JSValueScope.CreateRuntimeScope(env, context); } + private static JSValueScope TestScope(MockJSRuntime runtime) + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, runtime, new MockJSRuntime.SynchronizationContext()); + return JSValueScope.CreateRuntimeScope(env, context); + } + [Fact] public void GetReferenceFromSameScope() { @@ -74,7 +82,8 @@ public void GetReferenceFromDifferentRootScope() // Run in a new thread and establish another root scope there. TestUtils.RunInThread(() => { - using JSValueScope rootScope2 = TestScope(); + // Separate runtime so rootScope2's env has its own instance data (one context per env). + using JSValueScope rootScope2 = JSReferenceTests.TestScope(new MockJSRuntime()); Assert.Throws(() => reference.GetValue()); }).Wait(); } diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 7a019ace..d8b1b541 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -18,11 +18,13 @@ public class JSValueScopeTests { private readonly MockJSRuntime _mockRuntime = new(); - private JSValueScope TestRuntimeScope() + private JSValueScope TestRuntimeScope() => JSValueScopeTests.TestRuntimeScope(_mockRuntime); + + private static JSValueScope TestRuntimeScope(MockJSRuntime runtime) { napi_env env = new(Environment.CurrentManagedThreadId); var context = new JSRuntimeContext( - env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + env, runtime, new MockJSRuntime.SynchronizationContext()); return JSValueScope.CreateRuntimeScope(env, context); } @@ -266,7 +268,8 @@ public void AccessValueFromDifferentRootScope() // Run in a new thread and establish another root scope there. TestUtils.RunInThread(() => { - using JSValueScope rootScope2 = TestRuntimeScope(); + // Separate runtime so rootScope2's env has its own instance data (one context per env). + using JSValueScope rootScope2 = JSValueScopeTests.TestRuntimeScope(new MockJSRuntime()); Assert.Equal(JSValueScopeType.RuntimeContext, JSValueScope.Current.ScopeType); JSInvalidThreadAccessException ex = Assert.Throws( () => objectValue.IsObject()); From 61fd81537761c1556f3b4f4a1b5f1d06e4d67311 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 31 Aug 2026 14:06:44 -0700 Subject: [PATCH 30/38] Make the callback runtime-scope factory non-throwing and env-matched TryCreateRuntimeScope now inherits the current scope's context only when its environment matches the callback's env, so a synchronous callback for another environment resolves that env's context from the instance data instead of hitting the env-mismatch check. It also guards the fallible resolution (FromEnv and scope construction) so no setup exception can escape the UnmanagedCallersOnly callback boundary and terminate the process; an unresolvable or disposed context still makes the callback a no-op. --- src/NodeApi/JSValueScope.cs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 8d5bb1ed..9f84a24f 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -151,13 +151,26 @@ public static JSValueScope CreateRuntimeScope( /// Attempts to create a scope for a callback /// entering from JS, returning null when no live context can be resolved for the environment -- /// for example a retained JS function invoked after its context was disposed. Callback adapters - /// use it to become a no-op in that case rather than let an exception cross the unmanaged - /// boundary. Genuine misuse (wrong thread or mismatched environment) still throws. + /// use it to become a no-op in that case. It never throws: an exception escaping an + /// callback would cross the + /// native boundary and terminate the process. /// internal static JSValueScope? TryCreateRuntimeScope(napi_env env) { - JSRuntimeContext? context = CurrentOrNull?.RuntimeContext ?? JSRuntimeContext.FromEnv(env); - return context is { IsDisposed: false } ? new JSValueScope(env, context) : null; + try + { + // Inherit the current scope's context only when it belongs to this env; a synchronous + // callback for a different env must resolve that env's context, not the active one. + JSValueScope? current = CurrentOrNull; + JSRuntimeContext? context = current is null || current.UncheckedEnvironmentHandle != env + ? JSRuntimeContext.FromEnv(env) + : current.RuntimeContext; + return context is { IsDisposed: false } ? new JSValueScope(env, context) : null; + } + catch (Exception) + { + return null; + } } /// From 980ff4ddfff52fdd2f8eeb607fc3aca5614688ce Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 31 Aug 2026 14:49:27 -0700 Subject: [PATCH 31/38] Contain callback-scope disposal at native boundaries; fail reference access on a disposed context The seven UnmanagedCallersOnly callback boundaries (JSValue.InvokeCallback, the four tracing callbacks, and the five embedding adapters) declared the runtime scope with a using before the try, so a scope-disposal exception -- for example the LIFO order check when callback code left a nested scope open -- ran after the catch and could cross the native boundary. Each now disposes the scope inside an outer try, while callback errors are still reported as a JS exception from an inner catch that runs with the scope current. JSReference.ThrowIfDisposed now also fails when the owning context is disposed, so Handle, GetValue, and TryGetValue cannot read or invoke a napi_ref whose environment was already torn down. --- src/NodeApi/JSReference.cs | 4 +- src/NodeApi/JSValue.cs | 31 +++++-- src/NodeApi/Runtime/NodeEmbedding.cs | 116 +++++++++++++++++------- src/NodeApi/Runtime/TracingJSRuntime.cs | 18 +++- 4 files changed, 123 insertions(+), 46 deletions(-) diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 40456de4..d1a61f48 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -305,7 +305,9 @@ T GetValueAndRunAction() private void ThrowIfDisposed() { - if (IsDisposed) + // Once the owning context is disposed its env was torn down and the napi_ref is invalid, so + // every access must fail -- not only after the reference itself was explicitly disposed. + if (IsDisposed || _context.IsDisposed) { throw new ObjectDisposedException(nameof(JSReference)); } diff --git a/src/NodeApi/JSValue.cs b/src/NodeApi/JSValue.cs index c8f11c8d..6af74396 100644 --- a/src/NodeApi/JSValue.cs +++ b/src/NodeApi/JSValue.cs @@ -1264,24 +1264,37 @@ private static unsafe napi_value InvokeCallback( // host dispatches a callback with no scope on the thread -- recovered from env instance data. // A retained JS function invoked after its context was disposed resolves none, so the call // is a no-op instead of throwing across the unmanaged boundary. - using JSValueScope? scope = JSValueScope.TryCreateRuntimeScope(env); + JSValueScope? scope = JSValueScope.TryCreateRuntimeScope(env); if (scope is null) { return napi_value.Null; } + // The inner catch reports a callback error as a JS exception while the scope is still + // current; the outer catch keeps the scope's disposal -- which validates LIFO order and can + // throw when callback code left a nested scope open -- from escaping the unmanaged boundary. try { - JSCallbackArgs.GetDataAndLength(scope, callbackInfo, out object? data, out int length); - Span args = stackalloc napi_value[length]; - JSCallbackDescriptor descriptor = getCallbackDescriptor((TDescriptor)data!); - scope.ModuleHolder = descriptor.ModuleHolder; - return (napi_value)descriptor.Callback( - new JSCallbackArgs(scope, callbackInfo, args, descriptor.Data)); + using (scope) + { + try + { + JSCallbackArgs.GetDataAndLength(scope, callbackInfo, out object? data, out int length); + Span args = stackalloc napi_value[length]; + JSCallbackDescriptor descriptor = getCallbackDescriptor((TDescriptor)data!); + scope.ModuleHolder = descriptor.ModuleHolder; + return (napi_value)descriptor.Callback( + new JSCallbackArgs(scope, callbackInfo, args, descriptor.Data)); + } + catch (Exception ex) + { + JSError.ThrowError(ex); + return napi_value.Null; + } + } } - catch (Exception ex) + catch (Exception) { - JSError.ThrowError(ex); return napi_value.Null; } } diff --git a/src/NodeApi/Runtime/NodeEmbedding.cs b/src/NodeApi/Runtime/NodeEmbedding.cs index d445949b..9e776d2a 100644 --- a/src/NodeApi/Runtime/NodeEmbedding.cs +++ b/src/NodeApi/Runtime/NodeEmbedding.cs @@ -388,17 +388,27 @@ internal static unsafe void RuntimePreloadCallbackAdapter( napi_value process, napi_value require) { - using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + JSValueScope? jsValueScope = TryEnterRuntimeScope(env); if (jsValueScope is null) return; try { - var callback = (PreloadCallback)GCHandle.FromIntPtr(cb_data).Target!; - NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); - callback(embeddingRuntime, new JSValue(process), new JSValue(require)); + using (jsValueScope) + { + try + { + var callback = (PreloadCallback)GCHandle.FromIntPtr(cb_data).Target!; + NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); + callback(embeddingRuntime, new JSValue(process), new JSValue(require)); + } + catch (Exception ex) + { + JSError.ThrowError(ex); + } + } } - catch (Exception ex) + catch (Exception) { - JSError.ThrowError(ex); + // A scope-disposal exception must not escape the unmanaged boundary. } } @@ -413,18 +423,28 @@ internal static unsafe napi_value RuntimeLoadingCallbackAdapter( napi_value require, napi_value run_cjs) { - using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + JSValueScope? jsValueScope = TryEnterRuntimeScope(env); if (jsValueScope is null) return napi_value.Null; try { - var callback = (LoadingCallback)GCHandle.FromIntPtr(cb_data).Target!; - NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); - return (napi_value)callback( - embeddingRuntime, new JSValue(process), new JSValue(require), new JSValue(run_cjs)); + using (jsValueScope) + { + try + { + var callback = (LoadingCallback)GCHandle.FromIntPtr(cb_data).Target!; + NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); + return (napi_value)callback( + embeddingRuntime, new JSValue(process), new JSValue(require), new JSValue(run_cjs)); + } + catch (Exception ex) + { + JSError.ThrowError(ex); + return napi_value.Null; + } + } } - catch (Exception ex) + catch (Exception) { - JSError.ThrowError(ex); return napi_value.Null; } } @@ -438,17 +458,27 @@ internal static unsafe void RuntimeLoadedCallbackAdapter( napi_env env, napi_value loading_result) { - using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + JSValueScope? jsValueScope = TryEnterRuntimeScope(env); if (jsValueScope is null) return; try { - var callback = (LoadedCallback)GCHandle.FromIntPtr(cb_data).Target!; - NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); - callback(embeddingRuntime, new JSValue(loading_result)); + using (jsValueScope) + { + try + { + var callback = (LoadedCallback)GCHandle.FromIntPtr(cb_data).Target!; + NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); + callback(embeddingRuntime, new JSValue(loading_result)); + } + catch (Exception ex) + { + JSError.ThrowError(ex); + } + } } - catch (Exception ex) + catch (Exception) { - JSError.ThrowError(ex); + // A scope-disposal exception must not escape the unmanaged boundary. } } @@ -462,20 +492,30 @@ internal static unsafe napi_value ModuleInitializeCallbackAdapter( nint module_name, napi_value exports) { - using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + JSValueScope? jsValueScope = TryEnterRuntimeScope(env); if (jsValueScope is null) return napi_value.Null; try { - var callback = (InitializeModuleCallback)GCHandle.FromIntPtr(cb_data).Target!; - NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); - return (napi_value)callback( - embeddingRuntime, - Utf8StringArray.PtrToStringUTF8((byte*)module_name), - new JSValue(exports)); + using (jsValueScope) + { + try + { + var callback = (InitializeModuleCallback)GCHandle.FromIntPtr(cb_data).Target!; + NodeEmbeddingRuntime embeddingRuntime = NodeEmbeddingRuntime.FromHandle(runtime); + return (napi_value)callback( + embeddingRuntime, + Utf8StringArray.PtrToStringUTF8((byte*)module_name), + new JSValue(exports)); + } + catch (Exception ex) + { + JSError.ThrowError(ex); + return napi_value.Null; + } + } } - catch (Exception ex) + catch (Exception) { - JSError.ThrowError(ex); return napi_value.Null; } } @@ -531,16 +571,26 @@ internal static unsafe NodeEmbeddingStatus TaskPostCallbackAdapter( #endif internal static unsafe void NodeApiRunCallbackAdapter(nint cb_data, napi_env env) { - using JSValueScope? jsValueScope = TryEnterRuntimeScope(env); + JSValueScope? jsValueScope = TryEnterRuntimeScope(env); if (jsValueScope is null) return; try { - var callback = (RunNodeApiCallback)GCHandle.FromIntPtr(cb_data).Target!; - callback(); + using (jsValueScope) + { + try + { + var callback = (RunNodeApiCallback)GCHandle.FromIntPtr(cb_data).Target!; + callback(); + } + catch (Exception ex) + { + JSError.ThrowError(ex); + } + } } - catch (Exception ex) + catch (Exception) { - JSError.ThrowError(ex); + // A scope-disposal exception must not escape the unmanaged boundary. } } } diff --git a/src/NodeApi/Runtime/TracingJSRuntime.cs b/src/NodeApi/Runtime/TracingJSRuntime.cs index e24f1946..ab29242f 100644 --- a/src/NodeApi/Runtime/TracingJSRuntime.cs +++ b/src/NodeApi/Runtime/TracingJSRuntime.cs @@ -385,14 +385,26 @@ private static napi_value InvokeTraceCallback( napi_callback_info cbinfo, Func getCallbackDescriptor) { - using JSValueScope? scope = JSValueScope.TryCreateRuntimeScope(env); + JSValueScope? scope = JSValueScope.TryCreateRuntimeScope(env); if (scope is null) { return napi_value.Null; } - return ((TracingJSRuntime)scope.Runtime).TraceCallback( - scope, cbinfo, getCallbackDescriptor); + // TraceCallback reports callback errors as JS exceptions itself; the outer try only keeps a + // scope-disposal exception from escaping the UnmanagedCallersOnly boundary. + try + { + using (scope) + { + return ((TracingJSRuntime)scope.Runtime).TraceCallback( + scope, cbinfo, getCallbackDescriptor); + } + } + catch (Exception) + { + return napi_value.Null; + } } #if UNMANAGED_DELEGATES From 7344eba62001cd61d6024729f3a971884e849116 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 31 Aug 2026 16:25:22 -0700 Subject: [PATCH 32/38] Contain finalizer and teardown exceptions; dispose a context on its owning thread Wrap the napi_finalize and cleanup-hook UnmanagedCallersOnly callbacks so a managed exception can no longer escape across the native boundary during teardown, where no JS error can be reported. A new FreeFinalizerGCHandle helper frees each finalizer's GC handle exactly once -- tracked when the owning context is still usable, untracked otherwise -- without throwing. Reject disposing a JSRuntimeContext from a thread other than the one that created it, since teardown calls thread-affine napi (the sync context's RemoveEnvCleanupHook). The instance-data finalizer and the JS dispose functions, the only expected callers, both run on that thread. --- src/NodeApi/Interop/JSRuntimeContext.cs | 10 +++ .../Interop/JSSynchronizationContext.cs | 11 ++- src/NodeApi/Interop/JSThreadSafeFunction.cs | 21 ++++-- src/NodeApi/JSValue.cs | 70 ++++++++++++++----- test/JSValueScopeTests.cs | 19 +++++ 5 files changed, 106 insertions(+), 25 deletions(-) diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index ffcb8efa..234e5737 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -1003,6 +1003,16 @@ public void Dispose() { if (IsDisposed) return; + // Disposal calls thread-affine napi (the sync context's RemoveEnvCleanupHook), so it must + // run on the thread that created the context -- where the instance-data finalizer and the + // JS dispose functions, the only expected callers, both run. + if (OwningThreadId != Environment.CurrentManagedThreadId) + { + throw new JSInvalidThreadAccessException( + currentScope: null, + "A runtime context may be disposed only on the thread that created it."); + } + IsDisposed = true; // Run every teardown phase even if an earlier one throws, then release the context root and diff --git a/src/NodeApi/Interop/JSSynchronizationContext.cs b/src/NodeApi/Interop/JSSynchronizationContext.cs index e260ba7d..50d7d822 100644 --- a/src/NodeApi/Interop/JSSynchronizationContext.cs +++ b/src/NodeApi/Interop/JSSynchronizationContext.cs @@ -315,13 +315,18 @@ public override void Dispose() private static unsafe void Cleanup(nint data) { GCHandle cleanupHandle = GCHandle.FromIntPtr(data); - JSTsfnSynchronizationContext context = - (JSTsfnSynchronizationContext)cleanupHandle.Target!; - context._cleanupHandle = default; try { + JSTsfnSynchronizationContext context = + (JSTsfnSynchronizationContext)cleanupHandle.Target!; + context._cleanupHandle = default; context.Dispose(); } + catch (Exception) + { + // A cleanup hook must not throw across the native boundary; teardown continues + // regardless. + } finally { cleanupHandle.Free(); diff --git a/src/NodeApi/Interop/JSThreadSafeFunction.cs b/src/NodeApi/Interop/JSThreadSafeFunction.cs index ca7bbb17..012fbec4 100644 --- a/src/NodeApi/Interop/JSThreadSafeFunction.cs +++ b/src/NodeApi/Interop/JSThreadSafeFunction.cs @@ -211,12 +211,25 @@ private static readonly unsafe delegate* unmanaged[Cdecl] private static unsafe void FinalizeFunctionData(napi_env env, nint _, nint hint) { GCHandle functionDataHandle = GCHandle.FromIntPtr(hint); - if (functionDataHandle.Target is FunctionData functionData && functionData.Finalize is not null) + try { - functionData.Finalize(functionData.FunctionContext); + if (functionDataHandle.Target is FunctionData functionData && + functionData.Finalize is not null) + { + functionData.Finalize(functionData.FunctionContext); + } + } + catch (Exception) + { + // A finalizer must not throw across the native boundary; no JS error can be reported + // during teardown. + } + finally + { + // The handle was allocated on another thread, so free it directly rather than via the + // context's tracked FreeGCHandle. + functionDataHandle.Free(); } - - functionDataHandle.Free(); } #if UNMANAGED_DELEGATES diff --git a/src/NodeApi/JSValue.cs b/src/NodeApi/JSValue.cs index 6af74396..4a996f74 100644 --- a/src/NodeApi/JSValue.cs +++ b/src/NodeApi/JSValue.cs @@ -1304,19 +1304,21 @@ private static unsafe napi_value InvokeCallback( #endif internal static unsafe void FinalizeGCHandle(napi_env env, nint data, nint hint) { - // Resolve the context from the env rather than a finalize hint, so the context's rooting - // GCHandle can be freed at teardown. A null/disposed context means teardown already ran; - // just free the wrapped object's handle. GCHandle handle = GCHandle.FromIntPtr(data); - JSRuntimeContext? context = JSRuntimeContext.FromEnv(env); - if (context != null && !context.IsDisposed) + JSRuntimeContext? context = null; + try { - context.FreeGCHandle(handle); + // Resolve the context from the env rather than a finalize hint, so the context's + // rooting GCHandle can be freed at teardown. + context = JSRuntimeContext.FromEnv(env); } - else + catch (Exception) { - handle.Free(); + // A finalizer must not throw across the native boundary; a failed context lookup falls + // back to the best-effort untracked free below. } + + FreeFinalizerGCHandle(handle, context); } #if UNMANAGED_DELEGATES @@ -1327,14 +1329,18 @@ internal static unsafe void FinalizeGCHandleToPinnedMemory(napi_env env, nint da // The GC handle is passed via the hint parameter. // (The data parameter is the pointer to raw memory.) GCHandle handle = GCHandle.FromIntPtr(hint); - PinnedMemory pinnedMemory = (PinnedMemory)handle.Target!; + PinnedMemory? pinnedMemory = handle.Target as PinnedMemory; try { - pinnedMemory.Dispose(); + pinnedMemory?.Dispose(); + } + catch (Exception) + { + // A finalizer must not throw across the native boundary; teardown continues regardless. } finally { - pinnedMemory.RuntimeContext.FreeGCHandle(handle); + FreeFinalizerGCHandle(handle, pinnedMemory?.RuntimeContext); } } @@ -1343,11 +1349,12 @@ internal static unsafe void FinalizeGCHandleToPinnedMemory(napi_env env, nint da #endif private static unsafe void CallFinalizeAction(napi_env env, nint data, nint hint) { - // Resolve the context from the env rather than a finalize hint (see FinalizeGCHandle). GCHandle gcHandle = GCHandle.FromIntPtr(data); - JSRuntimeContext? context = JSRuntimeContext.FromEnv(env); + JSRuntimeContext? context = null; try { + // Resolve the context from the env rather than a finalize hint (see FinalizeGCHandle). + context = JSRuntimeContext.FromEnv(env); if (context != null && !context.IsDisposed) { // TODO: [vmoroz] In future we will be not allowed to run JS in finalizers. @@ -1356,17 +1363,44 @@ private static unsafe void CallFinalizeAction(napi_env env, nint data, nint hint ((Action)gcHandle.Target!)(); } } + catch (Exception) + { + // A finalizer must not throw across the native boundary and cannot report a JS error + // during teardown; a failed lookup, scope, or action is swallowed. + } finally + { + FreeFinalizerGCHandle(gcHandle, context); + } + } + + // Frees a finalizer's GC handle exactly once without throwing across the native boundary. When + // the owning context is still usable the free is tracked; otherwise -- teardown already ran, or + // the tracked free itself failed before releasing the handle -- the handle is freed untracked. + private static void FreeFinalizerGCHandle(GCHandle handle, JSRuntimeContext? context) + { + try { if (context != null && !context.IsDisposed) { - context.FreeGCHandle(gcHandle); - } - else - { - gcHandle.Free(); + context.FreeGCHandle(handle); + return; } } + catch (Exception) + { + // The tracked free failed (e.g. a debug handle-map mismatch) before releasing the + // handle; fall back to an untracked free below. + } + + try + { + handle.Free(); + } + catch (Exception) + { + // The handle was already freed or is invalid; nothing more to do. + } } internal abstract class PinnedMemory : IDisposable diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index d8b1b541..7ded3b62 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -306,6 +306,25 @@ public void EnterDisposedRuntimeContextThrows() () => JSValueScope.CreateRuntimeScope(env, context)); } + [Fact] + public void DisposeRuntimeContextFromDifferentThreadThrows() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + // A runtime context may be disposed only on the thread that created it, because teardown + // calls thread-affine napi. + TestUtils.RunInThread(() => + { + Assert.Throws(() => context.Dispose()); + }).Wait(); + + // The failed off-thread attempt left the context live, so disposing on the owning thread + // still succeeds. + context.Dispose(); + } + [Fact] public void SynchronizationContextRejectsLazyCreateWhenContextNotCurrent() { From fad0d6023f783685645eac17f0f842e627e9c7a4 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 31 Aug 2026 17:11:55 -0700 Subject: [PATCH 33/38] Clarify why a disposed context never deletes its napi_refs At env teardown Node runs finalizers in no defined order, so a napi_ref may already be finalized and freed; deleting it again would crash. On an explicit dispose() the env is still alive, so the few undeleted references are reclaimed when the env is torn down. The previous comment claimed the env is always torn down, which is not true on the explicit-dispose path. --- src/NodeApi/JSReference.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index d1a61f48..224c3473 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -352,9 +352,10 @@ protected virtual void Dispose(bool disposing) return; } - // Once the context is disposed its napi_env was torn down and Node already reclaimed every - // napi_ref, so there is nothing to delete and touching the env would be unsafe. This single - // flag invalidates all references at once, for both the explicit and finalizer paths. + // A disposed context never deletes its napi_refs, and this one flag invalidates every + // reference at once. At env teardown Node runs finalizers in no defined order, so a napi_ref + // may already be finalized and freed -- deleting it again would crash; on an explicit + // dispose() the env is still alive and the few undeleted refs are reclaimed at env teardown. if (_context.IsDisposed) { IsDisposed = true; From 2541ad766ac7d94b486b86d9ebe20dd27347e8b1 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 1 Sep 2026 16:59:44 -0700 Subject: [PATCH 34/38] Defer runtime context disposal until its value scopes close The JS dispose() hook and the managed environment-finalize notification are native calls dispatched through Node-API, which can be nested inside open value scopes. Disposing the runtime context then would leave those scopes to close their napi handle scopes on a disposed context as they unwind -- an unbalanced close that Node-API rejects. Flag the innermost open scope instead; the request moves outward as scopes close, so the outermost scope disposes the context once none remain open (or immediately when no scope is open). --- src/NodeApi.DotNetHost/ManagedHost.cs | 8 +++++- src/NodeApi/DotNetHost/NativeHost.cs | 22 +++++++++++----- src/NodeApi/JSValueScope.cs | 37 +++++++++++++++++++++++++++ test/JSValueScopeTests.cs | 37 +++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 8 deletions(-) diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index e5c9b87c..b7082d90 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -338,7 +338,13 @@ private void DisposeOnEnvironmentFinalize() { JSRuntimeContext? context = _context; _context = null; - context?.Dispose(); + if (context is not null) + { + // Defer disposal until any open value scope closes -- the notification can arrive while a + // managed callback scope is on the stack (managed calling JS calling dispose) -- mirroring + // the native host's dispose() hook. With no scope open it disposes immediately. + JSValueScope.DisposeRuntimeContextWhenIdle(context); + } } /// diff --git a/src/NodeApi/DotNetHost/NativeHost.cs b/src/NodeApi/DotNetHost/NativeHost.cs index f6273faa..eabef070 100644 --- a/src/NodeApi/DotNetHost/NativeHost.cs +++ b/src/NodeApi/DotNetHost/NativeHost.cs @@ -500,11 +500,17 @@ private JSValue InitializeDotNetHost( exports.SetProperty("require", require); exports.SetProperty("import", import); - // The dispose method runs the full idempotent host disposal -- notifying the managed host - // before closing the runtime-host channel -- so on .NET Framework (which notifies managed - // code only through that channel) the managed registration is released, not stranded. + // Defer disposing the host context until this callback (and any value scope it is nested in) + // has closed: dispose() is a native call dispatched through Node-API, so disposing the context + // while a scope is open would leave that scope to close its napi handle scope on a disposed + // context as it unwinds. Disposing the host context runs the full host teardown via its + // disposable annotation -- notifying the managed host and closing the runtime-host channel. exports.DefineProperties(new JSPropertyDescriptor( - "dispose", (_) => { Dispose(); return default; })); + "dispose", (_) => + { + JSValueScope.DisposeRuntimeContextWhenIdle(JSValueScope.Current.RuntimeContext); + return default; + })); // Invoke the managed host initialize method. It defines properties on the exports object // and fills in the registration so the native host can keep the managed host alive and @@ -552,7 +558,8 @@ private hostfxr_handle InitializeManagedRuntime( public void Dispose() { - // Called at env teardown (disposable annotation on the host context) and by the JS dispose() hook. + // Runs as the host context's disposable annotation when that context is disposed: at env + // teardown by its instance-data finalizer, or (deferred to scope close) by the JS dispose() hook. try { NotifyManagedHostEnvironmentFinalize(); @@ -568,8 +575,9 @@ public void Dispose() CloseRuntimeHost(); - // JSReference.Dispose no-ops once its context is disposed, so this frees the napi_ref only on - // an explicit dispose() (env alive), never during env-teardown finalization. + // Both dispose paths now run while the host context is disposed, so JSReference.Dispose + // short-circuits and Node reclaims the napi_ref at env teardown; the assignment drops the + // managed reference so it can be collected. _exports?.Dispose(); _exports = null; } diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 9f84a24f..cdff9fa3 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -53,6 +53,7 @@ public sealed class JSValueScope : IDisposable #pragma warning restore IDE0032 private readonly SynchronizationContext? _previousSyncContext; private readonly nint _scopeHandle; + private bool _disposeRuntimeContextOnClose; internal JSValueScopeType ScopeType { get; } @@ -314,6 +315,27 @@ private JSValueScope(JSValueScopeType scopeType) } } + /// + /// Disposes once every value scope open on the current thread + /// has closed, or immediately if none is open. A dispose request can arrive through a native + /// callback nested inside open value scopes (JS calling native calling JS…); disposing the + /// context then would leave those scopes to close their napi handle scopes on a disposed context + /// as they unwind, an unbalanced close that Node-API rejects. The innermost open scope is flagged + /// and the request moves outward as scopes close (LIFO), so the outermost scope disposes the + /// context once none remain open. + /// + internal static void DisposeRuntimeContextWhenIdle(JSRuntimeContext runtimeContext) + { + if (CurrentOrNull is { } scope) + { + scope._disposeRuntimeContextOnClose = true; + } + else + { + runtimeContext.Dispose(); + } + } + public void Dispose() { if (IsDisposed) return; @@ -356,6 +378,21 @@ public void Dispose() } CurrentOrNull = _parentScope; + + // Carry a deferred context-disposal request (see DisposeRuntimeContextWhenIdle) out to the + // parent, or -- at the outermost scope, where no scope remains open on the context -- dispose + // the context now that it has no open scopes. + if (_disposeRuntimeContextOnClose) + { + if (_parentScope is null) + { + RuntimeContext.Dispose(); + } + else + { + _parentScope._disposeRuntimeContextOnClose = true; + } + } } public JSValue Escape(JSValue value) diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 7ded3b62..653d527d 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -375,6 +375,43 @@ public void DisposeScopeFromDifferentThreadThrows() }).Wait(); } + [Fact] + public void DisposeRuntimeContextWhenIdleDefersUntilOutermostScopeCloses() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + using (JSValueScope outerScope = JSValueScope.CreateRuntimeScope(env, context)) + { + using (JSValueScope handleScope = JSValueScope.CreateHandleScope()) + { + // A disposal request while scopes are open must be deferred: disposing now would + // leave the handle scope to close on a disposed context as it unwinds. + JSValueScope.DisposeRuntimeContextWhenIdle(context); + Assert.False(context.IsDisposed); + } + + // The inner scope closed, but the outer scope keeps the context alive. + Assert.False(context.IsDisposed); + } + + // The outermost scope closed, so the context is disposed once no scope remains open. + Assert.True(context.IsDisposed); + } + + [Fact] + public void DisposeRuntimeContextWhenIdleDisposesImmediatelyWithNoScope() + { + napi_env env = new(Environment.CurrentManagedThreadId); + var context = new JSRuntimeContext( + env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + + // With no value scope open, the request is applied immediately. + JSValueScope.DisposeRuntimeContextWhenIdle(context); + Assert.True(context.IsDisposed); + } + [Fact] public void RegisteringSecondContextOnEnvIsRejected() { From f26d08350a01685eccf9096f2363cbcf38f5053d Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 1 Sep 2026 17:24:19 -0700 Subject: [PATCH 35/38] Track the target context when deferring runtime-context disposal The deferred-disposal request recorded only a boolean on the scope, so with a scope for one context nested under a scope for another (which TryCreateRuntimeScope allows across environments) the request propagated into the parent and disposed the parent's context instead of the requested one, leaving the requested context live. Record the target context with the request and stop propagating once the parent is null or belongs to a different context, so the requested context is disposed when its own outermost scope closes. --- src/NodeApi/JSValueScope.cs | 33 +++++++++++++++++---------------- test/JSValueScopeTests.cs | 29 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index cdff9fa3..f9d9ee23 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -53,7 +53,7 @@ public sealed class JSValueScope : IDisposable #pragma warning restore IDE0032 private readonly SynchronizationContext? _previousSyncContext; private readonly nint _scopeHandle; - private bool _disposeRuntimeContextOnClose; + private JSRuntimeContext? _runtimeContextToDisposeOnClose; internal JSValueScopeType ScopeType { get; } @@ -316,19 +316,19 @@ private JSValueScope(JSValueScopeType scopeType) } /// - /// Disposes once every value scope open on the current thread - /// has closed, or immediately if none is open. A dispose request can arrive through a native - /// callback nested inside open value scopes (JS calling native calling JS…); disposing the - /// context then would leave those scopes to close their napi handle scopes on a disposed context - /// as they unwind, an unbalanced close that Node-API rejects. The innermost open scope is flagged - /// and the request moves outward as scopes close (LIFO), so the outermost scope disposes the - /// context once none remain open. + /// Disposes once every value scope open on it has closed, or + /// immediately if none is open. A dispose request can arrive through a native callback nested + /// inside open value scopes (JS calling native calling JS…); disposing the context then would + /// leave those scopes to close their napi handle scopes on a disposed context as they unwind, an + /// unbalanced close that Node-API rejects. The request records the target context on the innermost + /// open scope and moves outward as scopes close (LIFO); the outermost scope that belongs to the + /// target context disposes it, so a foreign context nested below on the stack is left untouched. /// internal static void DisposeRuntimeContextWhenIdle(JSRuntimeContext runtimeContext) { if (CurrentOrNull is { } scope) { - scope._disposeRuntimeContextOnClose = true; + scope._runtimeContextToDisposeOnClose = runtimeContext; } else { @@ -379,18 +379,19 @@ public void Dispose() CurrentOrNull = _parentScope; - // Carry a deferred context-disposal request (see DisposeRuntimeContextWhenIdle) out to the - // parent, or -- at the outermost scope, where no scope remains open on the context -- dispose - // the context now that it has no open scopes. - if (_disposeRuntimeContextOnClose) + // Carry a deferred context-disposal request out to the parent, or dispose the target once this + // is its outermost open scope (parent is null or a different context). Tracking the target + // context -- not a flag -- avoids disposing a foreign context nested below on the stack, which + // TryCreateRuntimeScope allows, in the requested context's place. + if (_runtimeContextToDisposeOnClose is { } runtimeContext) { - if (_parentScope is null) + if (_parentScope is null || _parentScope.RuntimeContext != runtimeContext) { - RuntimeContext.Dispose(); + runtimeContext.Dispose(); } else { - _parentScope._disposeRuntimeContextOnClose = true; + _parentScope._runtimeContextToDisposeOnClose = runtimeContext; } } } diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 653d527d..79799fec 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -412,6 +412,35 @@ public void DisposeRuntimeContextWhenIdleDisposesImmediatelyWithNoScope() Assert.True(context.IsDisposed); } + [Fact] + public void DisposeRuntimeContextWhenIdleDisposesRequestedContextUnderForeignNesting() + { + napi_env env = new(Environment.CurrentManagedThreadId); + + // Separate runtimes so each context owns its own instance data, letting a scope for one be + // nested under a scope for the other (as TryCreateRuntimeScope allows across environments). + var contextA = new JSRuntimeContext( + env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); + var contextB = new JSRuntimeContext( + env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); + + using (JSValueScope.CreateRuntimeScope(env, contextA)) + { + using (JSValueScope.CreateRuntimeScope(env, contextB)) + { + // A scope for B is nested under a scope for A. The request must dispose B when B's + // scope closes, not propagate into A's scope and dispose A in its place. + JSValueScope.DisposeRuntimeContextWhenIdle(contextB); + Assert.False(contextB.IsDisposed); + } + + Assert.True(contextB.IsDisposed); + Assert.False(contextA.IsDisposed); + } + + contextA.Dispose(); + } + [Fact] public void RegisteringSecondContextOnEnvIsRejected() { From 2d76ea93995836675c76be82fa81f5eb029ab13c Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 1 Sep 2026 17:49:58 -0700 Subject: [PATCH 36/38] Contain and order disposal at more teardown boundaries Publish the process-wide runtime FromEnv uses only after instance-data registration succeeds, so a failed GetInstanceData/SetInstanceData or a rejected duplicate slot can't repoint FromEnv at a runtime that never registered a context. Guard the managed host's failed-init context disposal, since JSRuntimeContext.Dispose rethrows its first cleanup error, which must not escape the UnmanagedCallersOnly entry point. Mark NodeEmbeddingNodeApiScope and HermesRuntime disposed only after their scope and native closes succeed, so an out-of-order close (the value scope's LIFO/thread check) stays retryable instead of leaking the native scope or runtime. --- examples/hermes-engine/HermesRuntime.cs | 3 ++- src/NodeApi.DotNetHost/ManagedHost.cs | 13 +++++++++++-- src/NodeApi/Interop/JSRuntimeContext.cs | 7 +++++-- src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs | 5 ++++- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/examples/hermes-engine/HermesRuntime.cs b/examples/hermes-engine/HermesRuntime.cs index 37df0ede..25ac6e65 100644 --- a/examples/hermes-engine/HermesRuntime.cs +++ b/examples/hermes-engine/HermesRuntime.cs @@ -83,9 +83,10 @@ public void Dispose() { if (IsDisposed) return; VerifyElseThrow(JSDispatcherQueue.GetForCurrentThread() == _dispatcherQueue); - IsDisposed = true; + // Mark disposal only after both closes succeed so a throwing root-scope close stays retryable. _rootScope.Dispose(); hermes_delete_runtime(_runtime).ThrowIfFailed(); + IsDisposed = true; } public static explicit operator hermes_runtime(HermesRuntime value) => value._runtime; diff --git a/src/NodeApi.DotNetHost/ManagedHost.cs b/src/NodeApi.DotNetHost/ManagedHost.cs index b7082d90..951310d3 100644 --- a/src/NodeApi.DotNetHost/ManagedHost.cs +++ b/src/NodeApi.DotNetHost/ManagedHost.cs @@ -271,8 +271,17 @@ public static unsafe napi_value InitializeModule( finally { // The module-slot context does not own the instance-data finalizer, so a failed init - // must dispose it here; tolerate construction not having completed. - context?.Dispose(); + // must dispose it here; tolerate construction not having completed. Swallow a teardown + // failure -- JSRuntimeContext.Dispose rethrows its first cleanup error, which must not + // escape this UnmanagedCallersOnly entry point. + try + { + context?.Dispose(); + } + catch (Exception disposeError) + { + Trace($"Failed to dispose context after failed module init: {disposeError}"); + } } } diff --git a/src/NodeApi/Interop/JSRuntimeContext.cs b/src/NodeApi/Interop/JSRuntimeContext.cs index 234e5737..8900bcc0 100644 --- a/src/NodeApi/Interop/JSRuntimeContext.cs +++ b/src/NodeApi/Interop/JSRuntimeContext.cs @@ -314,8 +314,6 @@ internal JSRuntimeContext( /// private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) { - s_instanceDataRuntime = runtime; - runtime.GetInstanceData(env, out nint instanceData).ThrowIfFailed(); if (instanceData == default) { @@ -349,6 +347,11 @@ private unsafe void RegisterInstanceData(napi_env env, JSRuntime runtime) } ((nint*)instanceData)[s_instanceDataSlot] = ContextHandle; + + // Publish the process-wide runtime that FromEnv uses only after registration succeeds, so a + // failed GetInstanceData/SetInstanceData or a rejected duplicate slot can't repoint FromEnv at + // a runtime that never registered a context on this env. + s_instanceDataRuntime = runtime; } #if !UNMANAGED_DELEGATES diff --git a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs index 8f7367bb..5bfb61b4 100644 --- a/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs +++ b/src/NodeApi/Runtime/NodeEmbeddingNodeApiScope.cs @@ -45,11 +45,14 @@ public NodeEmbeddingNodeApiScope(NodeEmbeddingRuntime runtime) public void Dispose() { if (IsDisposed) return; - IsDisposed = true; + // Mark disposal only after both closes succeed: the value scope's LIFO/thread check can throw + // if a nested scope is still open, and marking first would leave that unretryable and leak the + // native Node-API scope. _valueScope.Dispose(); NodeEmbedding.JSRuntime.EmbeddingRuntimeCloseNodeApiScope( _runtime.Handle, _nodeApiScope) .ThrowIfFailed(); + IsDisposed = true; } } From 547f8a3fba83bbd8161c3bb436a6d87134a5301a Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 1 Sep 2026 18:10:07 -0700 Subject: [PATCH 37/38] Search the ancestor chain when deferring runtime-context disposal The deferred-disposal request only checked the immediate parent, so with alternating A -> B -> A scope nesting (reentrant cross-environment callbacks, which TryCreateRuntimeScope allows) the inner A scope's parent is B, so A was disposed while the outer A scope was still open -- leaving it able to use a torn-down environment and unable to unwind its handle scopes. Walk the ancestor chain for another scope belonging to the target context; dispose the target only when none remains, otherwise carry the request to the nearest such ancestor so the target is disposed when its own outermost scope closes. Added an alternating-nesting regression test. --- src/NodeApi/JSValueScope.cs | 19 +++++++++++++------ test/JSValueScopeTests.cs | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index f9d9ee23..cd43428c 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -379,19 +379,26 @@ public void Dispose() CurrentOrNull = _parentScope; - // Carry a deferred context-disposal request out to the parent, or dispose the target once this - // is its outermost open scope (parent is null or a different context). Tracking the target - // context -- not a flag -- avoids disposing a foreign context nested below on the stack, which - // TryCreateRuntimeScope allows, in the requested context's place. + // Dispose a deferred context-disposal request's target once this scope closes, but only when + // no ancestor still belongs to that context -- alternating nesting (A -> B -> A, which + // TryCreateRuntimeScope allows across environments) can leave an outer scope of the target + // below intervening scopes for other contexts. Otherwise carry the request to the nearest + // ancestor of the target context, so it is disposed only when its own outermost scope closes. if (_runtimeContextToDisposeOnClose is { } runtimeContext) { - if (_parentScope is null || _parentScope.RuntimeContext != runtimeContext) + JSValueScope? ancestor = _parentScope; + while (ancestor is not null && ancestor.RuntimeContext != runtimeContext) + { + ancestor = ancestor._parentScope; + } + + if (ancestor is null) { runtimeContext.Dispose(); } else { - _parentScope._runtimeContextToDisposeOnClose = runtimeContext; + ancestor._runtimeContextToDisposeOnClose = runtimeContext; } } } diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 79799fec..62507014 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -441,6 +441,44 @@ public void DisposeRuntimeContextWhenIdleDisposesRequestedContextUnderForeignNes contextA.Dispose(); } + [Fact] + public void DisposeRuntimeContextWhenIdleDefersPastAlternatingNesting() + { + napi_env env = new(Environment.CurrentManagedThreadId); + + // Separate runtimes so each context owns its own instance data, allowing alternating A -> B -> A + // scope nesting (reentrant cross-environment callbacks) on one thread. + var contextA = new JSRuntimeContext( + env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); + var contextB = new JSRuntimeContext( + env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); + + using (JSValueScope.CreateRuntimeScope(env, contextA)) + { + using (JSValueScope.CreateRuntimeScope(env, contextB)) + { + using (JSValueScope.CreateRuntimeScope(env, contextA)) + { + // Requesting A while the inner A scope is current must not dispose A when that + // scope closes: the outer A scope, below B on the stack, is still open. + JSValueScope.DisposeRuntimeContextWhenIdle(contextA); + Assert.False(contextA.IsDisposed); + } + + // The inner A scope closed, but the outer A scope keeps A alive. + Assert.False(contextA.IsDisposed); + } + + // B closed; the outer A scope still keeps A alive. + Assert.False(contextA.IsDisposed); + } + + // The outermost A scope closed, so A is disposed. + Assert.True(contextA.IsDisposed); + + contextB.Dispose(); + } + [Fact] public void RegisteringSecondContextOnEnvIsRejected() { From bedf377a52ef59889916fe4863d6b2d3c789785d Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 1 Sep 2026 19:12:25 -0700 Subject: [PATCH 38/38] Enforce one runtime context per thread scope stack A loaded module has its own thread-static scope stack, and Node associates one runtime context per environment per module, so every scope on a thread's stack shares one context -- separate modules (including the AOT native host and the CoreCLR managed host that share an env) have separate stacks that never mix. Enforce that invariant: a nested value scope must continue its parent's context, and creating one for a different context throws. This makes the cross-context scope nesting earlier changes hardened against (foreign-context and alternating A -> B -> A nesting) impossible rather than tolerated, so the deferred-disposal request is again a simple flag that disposes the one context when the outermost scope closes, and the callback scope factory inherits the current context (or FromEnv when no scope is open) without an env match. Updated the runtime-model doc and replaced the two cross-context tests with an invariant test. --- docs/concepts/runtime-model.md | 9 ++++++ src/NodeApi/JSValueScope.cs | 49 ++++++++++++++--------------- test/JSValueScopeTests.cs | 56 ++++------------------------------ 3 files changed, 38 insertions(+), 76 deletions(-) diff --git a/docs/concepts/runtime-model.md b/docs/concepts/runtime-model.md index 569e815e..904c506e 100644 --- a/docs/concepts/runtime-model.md +++ b/docs/concepts/runtime-model.md @@ -103,6 +103,15 @@ the surrounding context, so each loaded module resolves its own module instance 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 diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index cd43428c..0e226032 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -53,7 +53,7 @@ public sealed class JSValueScope : IDisposable #pragma warning restore IDE0032 private readonly SynchronizationContext? _previousSyncContext; private readonly nint _scopeHandle; - private JSRuntimeContext? _runtimeContextToDisposeOnClose; + private bool _disposeRuntimeContextOnClose; internal JSValueScopeType ScopeType { get; } @@ -160,12 +160,9 @@ public static JSValueScope CreateRuntimeScope( { try { - // Inherit the current scope's context only when it belongs to this env; a synchronous - // callback for a different env must resolve that env's context, not the active one. - JSValueScope? current = CurrentOrNull; - JSRuntimeContext? context = current is null || current.UncheckedEnvironmentHandle != env - ? JSRuntimeContext.FromEnv(env) - : current.RuntimeContext; + // Inherit the current scope's context -- every scope on a thread shares one context per + // environment -- or recover it from env instance data when no scope is open yet. + JSRuntimeContext? context = CurrentOrNull?.RuntimeContext ?? JSRuntimeContext.FromEnv(env); return context is { IsDisposed: false } ? new JSValueScope(env, context) : null; } catch (Exception) @@ -215,6 +212,15 @@ private JSValueScope(napi_env env, JSRuntimeContext? context, bool moduleBoundar ?? throw new InvalidOperationException( "A runtime context could not be resolved for the scope."); + // Every scope on a thread's stack shares one runtime context (one per environment per loaded + // module -- separate modules have separate thread-static scope stacks), so a nested scope must + // continue its parent's context. A mismatch is a caller error, not something to reconcile. + if (_parentScope is not null && context != _parentScope.RuntimeContext) + { + throw new InvalidOperationException( + "A nested value scope must use the same runtime context as its parent scope."); + } + // A disposed context's environment is torn down; entering it would call Node-API on a // dead env, which the scope's own unchecked handle would not catch. if (context.IsDisposed) @@ -320,15 +326,14 @@ private JSValueScope(JSValueScopeType scopeType) /// immediately if none is open. A dispose request can arrive through a native callback nested /// inside open value scopes (JS calling native calling JS…); disposing the context then would /// leave those scopes to close their napi handle scopes on a disposed context as they unwind, an - /// unbalanced close that Node-API rejects. The request records the target context on the innermost - /// open scope and moves outward as scopes close (LIFO); the outermost scope that belongs to the - /// target context disposes it, so a foreign context nested below on the stack is left untouched. + /// unbalanced close that Node-API rejects. Every scope on the stack shares this one context, so + /// the innermost open scope is flagged and the outermost disposes the context as it closes. /// internal static void DisposeRuntimeContextWhenIdle(JSRuntimeContext runtimeContext) { if (CurrentOrNull is { } scope) { - scope._runtimeContextToDisposeOnClose = runtimeContext; + scope._disposeRuntimeContextOnClose = true; } else { @@ -379,26 +384,18 @@ public void Dispose() CurrentOrNull = _parentScope; - // Dispose a deferred context-disposal request's target once this scope closes, but only when - // no ancestor still belongs to that context -- alternating nesting (A -> B -> A, which - // TryCreateRuntimeScope allows across environments) can leave an outer scope of the target - // below intervening scopes for other contexts. Otherwise carry the request to the nearest - // ancestor of the target context, so it is disposed only when its own outermost scope closes. - if (_runtimeContextToDisposeOnClose is { } runtimeContext) + // Carry a deferred context-disposal request (see DisposeRuntimeContextWhenIdle) out to the + // parent, or -- at the outermost scope, where none of the context's scopes remain open -- + // dispose the context now. + if (_disposeRuntimeContextOnClose) { - JSValueScope? ancestor = _parentScope; - while (ancestor is not null && ancestor.RuntimeContext != runtimeContext) - { - ancestor = ancestor._parentScope; - } - - if (ancestor is null) + if (_parentScope is null) { - runtimeContext.Dispose(); + RuntimeContext.Dispose(); } else { - ancestor._runtimeContextToDisposeOnClose = runtimeContext; + _parentScope._disposeRuntimeContextOnClose = true; } } } diff --git a/test/JSValueScopeTests.cs b/test/JSValueScopeTests.cs index 62507014..a2a62436 100644 --- a/test/JSValueScopeTests.cs +++ b/test/JSValueScopeTests.cs @@ -413,12 +413,11 @@ public void DisposeRuntimeContextWhenIdleDisposesImmediatelyWithNoScope() } [Fact] - public void DisposeRuntimeContextWhenIdleDisposesRequestedContextUnderForeignNesting() + public void NestedRuntimeScopeWithDifferentContextThrows() { napi_env env = new(Environment.CurrentManagedThreadId); - // Separate runtimes so each context owns its own instance data, letting a scope for one be - // nested under a scope for the other (as TryCreateRuntimeScope allows across environments). + // Separate runtimes so each context owns its own instance data. var contextA = new JSRuntimeContext( env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); var contextB = new JSRuntimeContext( @@ -426,56 +425,13 @@ public void DisposeRuntimeContextWhenIdleDisposesRequestedContextUnderForeignNes using (JSValueScope.CreateRuntimeScope(env, contextA)) { - using (JSValueScope.CreateRuntimeScope(env, contextB)) - { - // A scope for B is nested under a scope for A. The request must dispose B when B's - // scope closes, not propagate into A's scope and dispose A in its place. - JSValueScope.DisposeRuntimeContextWhenIdle(contextB); - Assert.False(contextB.IsDisposed); - } - - Assert.True(contextB.IsDisposed); - Assert.False(contextA.IsDisposed); + // Every scope on a thread's stack shares one runtime context, so nesting a scope for a + // different context is rejected. + Assert.Throws( + () => JSValueScope.CreateRuntimeScope(env, contextB)); } contextA.Dispose(); - } - - [Fact] - public void DisposeRuntimeContextWhenIdleDefersPastAlternatingNesting() - { - napi_env env = new(Environment.CurrentManagedThreadId); - - // Separate runtimes so each context owns its own instance data, allowing alternating A -> B -> A - // scope nesting (reentrant cross-environment callbacks) on one thread. - var contextA = new JSRuntimeContext( - env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); - var contextB = new JSRuntimeContext( - env, new MockJSRuntime(), new MockJSRuntime.SynchronizationContext()); - - using (JSValueScope.CreateRuntimeScope(env, contextA)) - { - using (JSValueScope.CreateRuntimeScope(env, contextB)) - { - using (JSValueScope.CreateRuntimeScope(env, contextA)) - { - // Requesting A while the inner A scope is current must not dispose A when that - // scope closes: the outer A scope, below B on the stack, is still open. - JSValueScope.DisposeRuntimeContextWhenIdle(contextA); - Assert.False(contextA.IsDisposed); - } - - // The inner A scope closed, but the outer A scope keeps A alive. - Assert.False(contextA.IsDisposed); - } - - // B closed; the outer A scope still keeps A alive. - Assert.False(contextA.IsDisposed); - } - - // The outermost A scope closed, so A is disposed. - Assert.True(contextA.IsDisposed); - contextB.Dispose(); }