Fix deadlock releasing TSFN after environment shutdown#480
Open
tetra-fox wants to merge 1 commit into
Open
Conversation
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.
Since nodejs/node#55877 (appeared in Node 24.14.0), environment shutdown finalizes a TSFN that still has use counts by releasing its resources while holding the TSFN mutex. Dropping the env reference there runs napi finalizers synchronously. The instance data finalizer disposes
JSRuntimeContext, whoseJSTsfnSynchronizationContext.Dispose()callednapi_release_threadsafe_functionon the same TSFN and re-locked a mutex the thread already holds. The JS thread deadlocks against itself. On older Node the stale release was a silent use-after-free. In VRCX (Electron 40 / .NET 9 / Linux) every quit hung the process:thread_finalize_cbfires when Node.js destroys the TSFN, before the instance data finalizer runs. The fix tracks it and skips the release once it has fired. The normal dispose path is unchanged, and both the callback andDispose()run on the JS thread, so there is no race on the flag. The finalization docs describe this ordering and warn about use-after-free innapi_finalizecallbacks.Repro on Node.js >= 24.14.0 (Linux x64, stock
mcr.microsoft.com/dotnet/runtime:9.0container):npm install node-api-dotnet@0.9.21, thennode -e "require('node-api-dotnet/net9.0')"never exits, parked on the stack above. With this change it exits immediately. On Node.js <= 24.13.0 the one-liner exits normally. Quit in VRCX hung 3/3 times on stock and exits cleanly 3/3 on this patch, and the existing suite passes.Considered but left out: the same guard on the other TSFN entry points (
Ref/Unref,Post/Send), and disposingJSRuntimeContextfrom a cleanup hook registered after the TSFN is created, so it runs while the TSFN is still alive (the pattern those docs recommend). The latter changes disposal timing during shutdown, so it seemed like a separate discussion.