diff --git a/src/NodeApi/Interop/JSSynchronizationContext.cs b/src/NodeApi/Interop/JSSynchronizationContext.cs index 48e2463b..4a729708 100644 --- a/src/NodeApi/Interop/JSSynchronizationContext.cs +++ b/src/NodeApi/Interop/JSSynchronizationContext.cs @@ -254,6 +254,10 @@ internal sealed unsafe class JSTsfnSynchronizationContext : JSSynchronizationCon private readonly JSRuntime _runtime; private readonly napi_env _env; private readonly JSThreadSafeFunction _tsfn; + + // Coordinates in-flight NonBlockingCall invocations with TSFN release during teardown, so that + // a poster can never call into the TSFN after (or while) it is being released. + private readonly TsfnCallGate _callGate = new(); private GCHandle _cleanupHandle; public JSTsfnSynchronizationContext() @@ -298,6 +302,11 @@ public override void Dispose() base.Dispose(); + // Prevent any new NonBlockingCall from starting and wait for in-flight calls to finish + // before releasing the TSFN. This closes the post-then-release race in which a poster could + // call into the TSFN after the environment cleanup hook has released it. + _callGate.Close(); + // Destroy TSFN by releasing last thread use count. // TSFN is deleted after this point and must not be used. _tsfn.Release(); @@ -348,9 +357,16 @@ public override void CloseAsyncScope() public override void Post(SendOrPostCallback callback, object? state) { - if (IsDisposed) return; + if (!_callGate.TryEnter()) return; - _tsfn.NonBlockingCall(() => callback(state)); + try + { + _tsfn.NonBlockingCall(() => callback(state)); + } + finally + { + _callGate.Exit(); + } } public override void Send(SendOrPostCallback callback, object? state) @@ -361,14 +377,25 @@ public override void Send(SendOrPostCallback callback, object? state) return; } - if (IsDisposed) return; - using ManualResetEvent syncEvent = new(false); - _tsfn.NonBlockingCall(() => + + // The gate is held only around the native NonBlockingCall, not around the wait below: the + // posted callback runs on the JS thread and could otherwise deadlock TSFN release. + if (!_callGate.TryEnter()) return; + + try { - callback(state); - syncEvent.Set(); - }); + _tsfn.NonBlockingCall(() => + { + callback(state); + syncEvent.Set(); + }); + } + finally + { + _callGate.Exit(); + } + syncEvent.WaitOne(); } } diff --git a/src/NodeApi/Interop/TsfnCallGate.cs b/src/NodeApi/Interop/TsfnCallGate.cs new file mode 100644 index 00000000..0ba64da0 --- /dev/null +++ b/src/NodeApi/Interop/TsfnCallGate.cs @@ -0,0 +1,97 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Threading; + +namespace Microsoft.JavaScript.NodeApi.Interop; + +/// +/// Coordinates in-flight native thread-safe-function (TSFN) calls with TSFN release, so that a +/// caller cannot invoke the TSFN after (or while) it is being released. +/// +/// +/// +/// posts work to the JS thread by calling into a native +/// TSFN, and releases that TSFN from an environment cleanup hook during teardown. Without +/// coordination there is a race: a poster can observe the context as not-yet-disposed, then the +/// cleanup hook can release the TSFN, and then the poster calls into the released TSFN — a +/// native use-after-release that a managed try/catch cannot guard. +/// +/// +/// This gate closes that window. A caller wraps each native TSFN call in +/// /; the owner calls before releasing +/// the TSFN. atomically prevents any further from +/// succeeding and then waits for all in-flight calls to finish, guaranteeing that no native call +/// is in progress and none can start once returns. +/// +/// +internal sealed class TsfnCallGate +{ + // High bit marks the gate as closed; the remaining bits count in-flight calls. Because Enter + // never increments once the closed bit is set, and Exit is only called after a successful + // Enter, the count part never borrows into the closed bit. + private const int ClosedFlag = unchecked((int)0x80000000); + private const int CountMask = 0x7FFFFFFF; + + private int _state; + + /// + /// Gets a value indicating whether the gate has been closed. + /// + public bool IsClosed => (Volatile.Read(ref _state) & ClosedFlag) != 0; + + /// + /// Attempts to enter the gate for a single native call. When this returns true the caller must + /// pair it with exactly one call to once the native call has returned. + /// + /// True if the call may proceed; false if the gate is closed and the call must be + /// skipped. + public bool TryEnter() + { + int state = Volatile.Read(ref _state); + while ((state & ClosedFlag) == 0) + { + int updated = Interlocked.CompareExchange(ref _state, state + 1, state); + if (updated == state) + { + return true; + } + + state = updated; + } + + return false; + } + + /// + /// Marks completion of a native call previously admitted by . + /// + public void Exit() => Interlocked.Decrement(ref _state); + + /// + /// Closes the gate so that no further calls can enter, then blocks until all in-flight calls + /// have exited. After this returns it is safe to release the underlying TSFN. Calling this more + /// than once is safe. + /// + public void Close() + { + int state = Volatile.Read(ref _state); + while ((state & ClosedFlag) == 0) + { + int updated = Interlocked.CompareExchange( + ref _state, state | ClosedFlag, state); + if (updated == state) + { + break; + } + + state = updated; + } + + SpinWait spin = default; + while ((Volatile.Read(ref _state) & CountMask) != 0) + { + spin.SpinOnce(); + } + } +} diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 250b0286..d0c6e862 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -2,9 +2,11 @@ // Licensed under the MIT License. using System; +using System.Collections.Concurrent; 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; @@ -32,6 +34,30 @@ public class JSReference : IDisposable private readonly napi_env _env; private readonly JSRuntimeContext? _context; + /// + /// Environment-scoped queue of no-context references whose finalizers could not release them + /// on the JS thread, keyed by the owning environment handle. + /// + /// + /// A no-context reference has no , so when its finalizer + /// runs on the GC finalizer thread (which has no JS scope) it cannot marshal + /// DeleteReference back to the owning JS thread. Rather than leaking the + /// napi_ref until the environment is destroyed, the finalizer enqueues the handle here. + /// The deletion is then performed the next time a scope for the same environment is active on + /// its JS thread (see ), and a final drain runs when that + /// environment's root scope is disposed. Deferred handles are always drained while the + /// environment is still alive, so the queued napi_ref remains valid. + /// + private static readonly ConcurrentDictionary> + s_pendingFinalizerDeletions = new(); + + /// + /// Total number of deferred deletions pending across all environments. Used as a cheap + /// fast-path gate so that the common case (nothing pending) avoids a dictionary lookup on + /// every scope entry. + /// + private static int s_pendingFinalizerDeletionCount; + /// /// Creates a new instance of a that holds a strong or weak /// reference to a JS value. @@ -388,17 +414,21 @@ private void DisposeFromFinalizer() { // 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). + // GC finalizer thread it is null and the inline delete below is skipped; the + // handle is instead deferred to be deleted the next time a scope for this + // environment is active on its JS thread (see JSValueScope). The guarded delete + // still runs directly if Dispose(disposing: false) is ever invoked on the owning + // JS thread. This releases the napi_ref promptly instead of leaking it until the + // JS environment is destroyed. JSValueScope? scope = JSValueScope.CurrentOrNull; if (scope != null && scope.UncheckedEnvironmentHandle == _env) { scope.Runtime.DeleteReference(_env, _handle); } + else + { + EnqueuePendingDeletion(_env, _handle); + } } else { @@ -425,6 +455,78 @@ private void DisposeFromFinalizer() } } + /// + /// Records a no-context reference handle whose deletion was deferred because its finalizer + /// could not run on the owning JS thread. The handle is deleted later by + /// or . + /// + private static void EnqueuePendingDeletion(napi_env env, napi_ref handle) + { + s_pendingFinalizerDeletions + .GetOrAdd(env, _ => new ConcurrentQueue()) + .Enqueue(handle); + Interlocked.Increment(ref s_pendingFinalizerDeletionCount); + } + + /// + /// Deletes any no-context reference handles that were deferred by a finalizer for the given + /// environment. Must be called on the JS thread that owns with an + /// active scope, so that DeleteReference is valid. + /// + /// The environment whose deferred deletions should be released. + /// The JS runtime for the environment. + internal static void DrainPendingDeletions(napi_env env, JSRuntime runtime) + { + // Fast path: avoid a dictionary lookup on every scope entry when nothing is pending. + if (Volatile.Read(ref s_pendingFinalizerDeletionCount) == 0) + { + return; + } + + if (!s_pendingFinalizerDeletions.TryGetValue( + env, out ConcurrentQueue? queue)) + { + return; + } + + DeletePendingHandles(env, runtime, queue); + } + + /// + /// Performs a final drain of deferred deletions for an environment that is being torn down and + /// removes its queue, so the registry does not retain entries for environments that no longer + /// exist. Must be called on the owning JS thread while the environment is still alive. + /// + /// The environment being torn down. + /// The JS runtime for the environment. + internal static void RemovePendingDeletions(napi_env env, JSRuntime runtime) + { + if (!s_pendingFinalizerDeletions.TryRemove( + env, out ConcurrentQueue? queue)) + { + return; + } + + DeletePendingHandles(env, runtime, queue); + } + + private static void DeletePendingHandles( + napi_env env, JSRuntime runtime, ConcurrentQueue queue) + { + while (queue.TryDequeue(out napi_ref handle)) + { + Interlocked.Decrement(ref s_pendingFinalizerDeletionCount); + try + { + runtime.DeleteReference(env, handle); + } + catch + { + // Best effort: the environment may already be gone. + } + } + } + ~JSReference() { // An exception escaping a finalizer terminates the process. Dispose(bool) is virtual, so a diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 2aabc8e2..7e938a8e 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -359,6 +359,11 @@ public JSValueScope( CurrentOrNull = previousScope; throw; } + + // Release any no-context reference deletions that a finalizer had to defer because it could + // not run on the JS thread. This scope is active on the owning JS thread, so deleting the + // deferred handles here is valid. The call is a cheap no-op when nothing is pending. + JSReference.DrainPendingDeletions(_env, Runtime); } public void Dispose() @@ -366,6 +371,16 @@ public void Dispose() if (IsDisposed) return; IsDisposed = true; + if (ScopeType == JSValueScopeType.Root || ScopeType == JSValueScopeType.NoContext) + { + // This environment-scoped scope is going away, so perform a final drain of any deferred + // no-context reference deletions and remove the environment's queue, preventing the + // registry from retaining entries for environments that no longer exist. The scope is + // still current here, so deletion runs on the owning JS thread while the environment is + // alive. + JSReference.RemovePendingDeletions(_env, Runtime); + } + if (ScopeType != JSValueScopeType.NoContext) { napi_env env = RuntimeContext.EnvironmentHandle; diff --git a/src/NodeApi/NodeApi.csproj b/src/NodeApi/NodeApi.csproj index b6aa76a6..3727e35e 100644 --- a/src/NodeApi/NodeApi.csproj +++ b/src/NodeApi/NodeApi.csproj @@ -28,6 +28,10 @@ + + + + diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index ed994865..fb2cbffd 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -133,7 +133,66 @@ public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow() Assert.True(reference.IsDisposed); } - // A reference with a runtime context posts its cleanup to the JS thread instead of deleting it + // A no-context reference finalized off the JS thread cannot be deleted inline (there is no + // synchronization context to marshal the delete back to the JS thread). Instead of leaking the + // napi_ref until the environment is destroyed, the finalizer defers the deletion, which is then + // performed the next time a scope for the same environment is active on its JS thread. + [Fact] + public void FinalizeNoContextReferenceDefersDeletionUntilNextScope() + { + napi_env env = new(Environment.CurrentManagedThreadId); + using JSValueScope noContextScope = new( + JSValueScopeType.NoContext, env, _mockRuntime, + new MockJSRuntime.SynchronizationContext()); + + JSValue value = JSValue.CreateObject(); + var reference = new FinalizerTestReference(value); + napi_ref handle = reference.Handle; + Assert.True(_mockRuntime.HasReference(handle)); + + // Simulate the GC finalizer thread: no current scope, so the delete cannot run inline. + TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); + Assert.True(reference.IsDisposed); + + // The delete is deferred, not run on the finalizer thread, so the reference still exists. + Assert.True(_mockRuntime.HasReference(handle)); + + // Entering a scope for the same environment on the JS thread drains the deferred deletion. + using (JSValueScope drainScope = new(JSValueScopeType.NoContext)) + { + } + + Assert.False(_mockRuntime.HasReference(handle)); + } + + // When the environment-scoped scope that owns a deferred no-context deletion is disposed, a + // final drain runs so the napi_ref is released even if no further scope is entered. + [Fact] + public void DisposingScopeDrainsDeferredNoContextDeletion() + { + napi_env env = new(Environment.CurrentManagedThreadId); + napi_ref handle; + + var noContextScope = new JSValueScope( + JSValueScopeType.NoContext, env, _mockRuntime, + new MockJSRuntime.SynchronizationContext()); + try + { + JSValue value = JSValue.CreateObject(); + var reference = new FinalizerTestReference(value); + handle = reference.Handle; + + TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); + Assert.True(_mockRuntime.HasReference(handle)); + } + finally + { + // Disposing the environment-scoped scope performs the final drain. + noContextScope.Dispose(); + } + + Assert.False(_mockRuntime.HasReference(handle)); + } // 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. [Fact] diff --git a/test/TsfnCallGateTests.cs b/test/TsfnCallGateTests.cs new file mode 100644 index 00000000..6b6d362d --- /dev/null +++ b/test/TsfnCallGateTests.cs @@ -0,0 +1,121 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.JavaScript.NodeApi.Interop; +using Xunit; + +namespace Microsoft.JavaScript.NodeApi.Test; + +public class TsfnCallGateTests +{ + [Fact] + public void EnterAndExitWhileOpen() + { + var gate = new TsfnCallGate(); + + Assert.True(gate.TryEnter()); + gate.Exit(); + + Assert.False(gate.IsClosed); + } + + [Fact] + public void CloseRejectsFurtherEntry() + { + var gate = new TsfnCallGate(); + + gate.Close(); + + Assert.True(gate.IsClosed); + Assert.False(gate.TryEnter()); + } + + [Fact] + public void CloseIsIdempotent() + { + var gate = new TsfnCallGate(); + + gate.Close(); + gate.Close(); + + Assert.True(gate.IsClosed); + Assert.False(gate.TryEnter()); + } + + // Close must not return while a call is in flight: it has to wait for the corresponding Exit, + // guaranteeing no native TSFN call is in progress once the TSFN is released. + [Fact] + public void CloseWaitsForInFlightCallToExit() + { + var gate = new TsfnCallGate(); + + // Simulate an in-flight native call that has entered but not yet exited. + Assert.True(gate.TryEnter()); + + Task closeTask = Task.Run(() => gate.Close()); + + // Close cannot complete while the call is in flight. + Assert.False(closeTask.Wait(TimeSpan.FromMilliseconds(200))); + + // Once the in-flight call exits, Close completes. + gate.Exit(); + Assert.True(closeTask.Wait(TimeSpan.FromSeconds(5))); + + // After closing, no further calls are admitted. + Assert.False(gate.TryEnter()); + } + + // Once Close has set the closed flag, an entry that races with it must be rejected, so the + // in-flight count cannot rise again after Close begins draining. + [Fact] + public void EntryDoesNotSucceedAfterCloseFlagSet() + { + var gate = new TsfnCallGate(); + + gate.Close(); + + // A burst of concurrent entry attempts after Close must all fail. + Parallel.For(0, 1000, _ => Assert.False(gate.TryEnter())); + } + + // Stress the gate with concurrent enter/exit callers while another thread closes it, then + // assert the invariant that after Close returns no caller is inside the gate and none can + // enter. + [Fact] + public void ConcurrentCallersDrainBeforeCloseCompletes() + { + var gate = new TsfnCallGate(); + using var start = new ManualResetEventSlim(false); + + Task[] callers = new Task[8]; + for (int i = 0; i < callers.Length; i++) + { + callers[i] = Task.Run(() => + { + start.Wait(); + for (int j = 0; j < 5000; j++) + { + if (gate.TryEnter()) + { + // Represents a brief native call. + Thread.SpinWait(10); + gate.Exit(); + } + } + }); + } + + start.Set(); + + // Close concurrently with the callers; it must wait for any in-flight call to exit. + gate.Close(); + + // After Close returns, no caller may enter and the callers finish without error. + Assert.False(gate.TryEnter()); + Assert.True(Task.WaitAll(callers, TimeSpan.FromSeconds(30))); + Assert.True(gate.IsClosed); + } +}