node-api: fix crash on re-entering TSFN finalization during env shutdown - #65967
Open
legendecas wants to merge 1 commit into
Open
node-api: fix crash on re-entering TSFN finalization during env shutdown#65967legendecas wants to merge 1 commit into
legendecas wants to merge 1 commit into
Conversation
Collaborator
|
Review requested:
|
legendecas
force-pushed
the
node-api-tsfn-reentry
branch
from
September 10, 2026 20:41
66f198e to
b3c624f
Compare
Signed-off-by: Chengzhong Wu <cwu631@bloomberg.net>
legendecas
force-pushed
the
node-api-tsfn-reentry
branch
from
September 10, 2026 21:12
b3c624f to
043d783
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65967 +/- ##
=======================================
Coverage 90.17% 90.17%
=======================================
Files 771 771
Lines 265489 265471 -18
Branches 50471 50470 -1
=======================================
- Hits 239405 239397 -8
Misses 17042 17042
+ Partials 9042 9032 -10
🚀 New features to boost your workflow:
|
KevinEady
reviewed
Sep 11, 2026
|
|
||
| // Runs on JS thread. | ||
| void MaybeDelete() { | ||
| CHECK_EQ(state, kClosing); |
Contributor
There was a problem hiding this comment.
We discussed this PR in the 11 Sep 2026 Node-API meeting, specifically regarding of this MaybeDelete() can be called where the state is already kClosed (ie. if this MaybeDelete() gets called multiple times).
We discussed that it might be beneficial to add more test cases to verify that this CHECK_EQ is safe, by adding a test case with >1 for initial_thread_count and using the multiple pathways to "finalize" the TSFN:
- all threads call
napi_release_threadsafe_functionwithnapi_tsfn_releasemode. - one thread calls
napi_release_threadsafe_functionwithnapi_tsfn_abort, and other threads callnapi_call_threadsafe_functionwhich returnsnapi_closing.
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.
Fix the TSFN finalization re-entrance.
state = kClosedmarks a TSFN as ready to bedeleted, so a TSFN should not set this state before calling into user code synchronously,
which could call into
napi_release_threadsafe_function, and delete the TSFNsynchronously in the middle of
ThreadSafeFunction::MaybeDelete.Fixes: #65100