node-api: avoid TSFN mutex reentry during teardown - #65941
Conversation
ThreadSafeFunction releases its Node-API environment reference while finalizing its loop-owned resources. That operation can synchronously invoke an external finalizer, which may release a still-valid TSFN ownership. Do not hold the TSFN mutex across this reentrant path. Use an intermediate resource-cleanup state to retain the TSFN until resource cleanup completes, then permit the loop or final native owner to destroy it. Fixes: nodejs#65100 Signed-off-by: Jimmy Miller <james.miller@vercel.com> Assisted-by: OpenCode
|
Review requested:
|
nodejs-github-bot
added
c++
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65941 +/- ##
==========================================
+ Coverage 90.16% 90.18% +0.02%
==========================================
Files 771 771
Lines 265445 265450 +5
Branches 50455 50460 +5
==========================================
+ Hits 239329 239392 +63
+ Misses 17056 16995 -61
- Partials 9060 9063 +3
🚀 New features to boost your workflow:
|
| size_t thread_count; | ||
| State state; | ||
|
|
||
| bool resources_released; |
There was a problem hiding this comment.
I don't find this new flag resources_released, and the enum state kResourceCleanup to be necessary. This makes the TSFN harder to maintain.
I think the existing state is sufficient to fix the issue: #65967.
There was a problem hiding this comment.
@legendecas So much simpler! Don't know why I didn't think of that. Thanks. Closing mine.
|
closed in favor of #65967 |
github-project-automation
Bot
moved this from Need Triage
to Done
in Node-API Team Project
3 participants
I did use ai to help write some this. But I have reviewed and testing things myself. From everything I can tell this, fixes the reentry problem without introducing other issues. I tried to keep the code minimal. Happy to add/change things as needed.
I did have a test over on this branch that adds a test main...jimmyhmiller:node:jimmym/tsfn-teardown-fix-test
Happy to include it if you'd like it.
I also had codex play with making a tla+ formal model over on my fork. I can't pretend I fully reviewed that. But it does lend at least some credence as the old setup recreates the bug and the new doesn't seem to have counter examples (obviously grains of salt here)
I did test the original reproducing case and it is now fixed.
Fixes: #65100