Fix no-context JSReference leak and TSFN post/release race (follow-ups to #492) - #495
Open
Saúl Ponce (GalaxiasKyklos) wants to merge 2 commits into
Open
Conversation
A JSReference created in a NoContext scope has no JSSynchronizationContext, so when its finalizer runs on the GC finalizer thread it cannot marshal DeleteReference back to the owning JS thread. Previously the finalizer simply skipped the delete, leaking the napi_ref (and any strongly-referenced JS value) until the JS environment was destroyed. Defer such deletions into an environment-scoped queue keyed by napi_env. The queue is drained on the JS thread the next time any scope for that environment is entered, and finally when the environment's Root/NoContext scope is disposed (which also removes the registry entry so it does not retain dead environments). A cheap interlocked counter gates the fast path, so the common no-pending case costs only a volatile read per scope entry. Deferred handles are always deleted while the environment is still alive, keeping the queued napi_ref valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7
JSTsfnSynchronizationContext.Post/Send checked IsDisposed and then called the thread-safe function via NonBlockingCall. The environment cleanup hook could run Dispose (releasing the TSFN) between the check and the call, causing a native use-after-release that a managed try/catch cannot guard. Introduce TsfnCallGate, an interlocked gate that reference-counts in-flight NonBlockingCall invocations. Post/Send wrap each native call in TryEnter/Exit; Dispose calls Close before releasing the TSFN. Close atomically rejects new entrants and then waits for in-flight calls to drain, guaranteeing no native call is in progress and none can start once the TSFN is released. The gate is held only around the native call, not around Send's completion wait, so TSFN release cannot deadlock against a callback running on the JS thread. Expose internals to the test assembly so the gate's concurrency behavior can be unit-tested directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7
Copilot started reviewing on behalf of
Saúl Ponce (GalaxiasKyklos)
August 15, 2026 00:30
View session
There was a problem hiding this comment.
Pull request overview
Addresses follow-up leaks and teardown races from #492.
Changes:
- Defers no-context
JSReferencedeletion to the JS thread. - Adds a gate around TSFN calls and release.
- Adds concurrency and reference-cleanup tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/NodeApi/JSReference.cs |
Queues deferred reference deletions. |
src/NodeApi/JSValueScope.cs |
Drains queued deletions. |
src/NodeApi/Interop/TsfnCallGate.cs |
Implements the TSFN call gate. |
src/NodeApi/Interop/JSSynchronizationContext.cs |
Coordinates TSFN calls with release. |
src/NodeApi/NodeApi.csproj |
Exposes internals to tests. |
test/JSReferenceTests.cs |
Tests deferred reference deletion. |
test/TsfnCallGateTests.cs |
Tests gate behavior and concurrency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+465
to
+467
| s_pendingFinalizerDeletions | ||
| .GetOrAdd(env, _ => new ConcurrentQueue<napi_ref>()) | ||
| .Enqueue(handle); |
Comment on lines
+374
to
+381
| 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); |
Comment on lines
+111
to
+114
| start.Set(); | ||
|
|
||
| // Close concurrently with the callers; it must wait for any in-flight call to exit. | ||
| gate.Close(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two follow-ups to #492 (which made the
JSReferencefinalizer thread-safe to fix the worker-teardown crash). #492 intentionally left two related issues for a separate change; this PR addresses both.Each fix is a separate commit.
1. Delete no-context
JSReferenceon the JS thread instead of leaking itA
JSReferencecreated in aNoContextscope has noJSSynchronizationContext, so when its finalizer runs on the GC finalizer thread it cannot marshalDeleteReferenceback to the owning JS thread. Previously the finalizer simply skipped the delete, leaking thenapi_ref(and any strongly-referenced JS value) until the JS environment was destroyed.This change defers such deletions into an environment-scoped queue keyed by
napi_env. The queue is drained on the JS thread:JSReference.DrainPendingDeletions, called from theJSValueScopeconstructor); andRoot/NoContextscope is disposed (JSReference.RemovePendingDeletions, which also removes the registry entry so it does not retain entries for environments that no longer exist).A process-wide interlocked counter gates the fast path, so the common (nothing-pending) case costs only a volatile read per scope entry. Deferred handles are always deleted while the environment is still alive, so the queued
napi_refremains valid.2. Close the TSFN post-then-release race in
JSSynchronizationContextJSTsfnSynchronizationContext.Post/SendcheckedIsDisposedand then called the thread-safe function viaNonBlockingCall. The environment cleanup hook could runDispose(releasing the TSFN) between the check and the call, a native use-after-release that a managedtry/catchcannot guard. This was pre-existing (the original context-pathDisposeused the same pattern); #492's deferred delete posts through the same path.This change adds
TsfnCallGate, an interlocked gate that reference-counts in-flightNonBlockingCallinvocations:Post/Sendwrap each native call inTryEnter()/Exit().DisposecallsClose()before_tsfn.Release().Close()atomically rejects new entrants and then waits for in-flight calls to drain.Once
Close()returns, no native call is in progress and none can start, soRelease()cannot race aNonBlockingCall. The gate is held only around the native call — not aroundSend's completion wait — so TSFN release cannot deadlock against a callback running on the JS thread.InternalsVisibleTois added so the gate's concurrency behavior can be unit-tested directly.Testing
TsfnCallGateconcurrency tests, including a deterministic "Close waits for in-flight call to exit" test and a concurrent enter/exit-vs-close stress test.JSReferenceTestsandJSValueScopeTestscontinue to pass.NodeApibuilds clean on all target frameworks with AOT-compatibility checks enabled;dotnet formatis clean.Related
Follow-ups to #492.
Fixes #496
Fixes #497