Skip to content

node-api: fix crash on re-entering TSFN finalization during env shutdown - #65967

Open
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:node-api-tsfn-reentry
Open

node-api: fix crash on re-entering TSFN finalization during env shutdown#65967
legendecas wants to merge 1 commit into
nodejs:mainfrom
legendecas:node-api-tsfn-reentry

Conversation

@legendecas

Copy link
Copy Markdown
Member

Fix the TSFN finalization re-entrance. state = kClosed marks a TSFN as ready to be
deleted, 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 TSFN
synchronously in the middle of ThreadSafeFunction::MaybeDelete.

Fixes: #65100

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API. labels Sep 10, 2026
@legendecas
legendecas force-pushed the node-api-tsfn-reentry branch from 66f198e to b3c624f Compare September 10, 2026 20:41
Signed-off-by: Chengzhong Wu <cwu631@bloomberg.net>
@legendecas
legendecas force-pushed the node-api-tsfn-reentry branch from b3c624f to 043d783 Compare September 10, 2026 21:12
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.17%. Comparing base (b805fb5) to head (043d783).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/node_api.cc 66.66% 0 Missing and 1 partial ⚠️
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     
Files with missing lines Coverage Δ
src/node_api.cc 75.48% <66.66%> (+0.15%) ⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Sep 11, 2026
Comment thread src/node_api.cc

// Runs on JS thread.
void MaybeDelete() {
CHECK_EQ(state, kClosing);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_function with napi_tsfn_release mode.
  • one thread calls napi_release_threadsafe_function with napi_tsfn_abort, and other threads call napi_call_threadsafe_function which returns napi_closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to Node-API.

Projects

Status: In Progress

3 participants