n-api: detect deadlocks in thread-safe function (take two) by gabrielschulhof · Pull Request #32860 · nodejs/node · GitHub

uv_thread_self() works on Windows only for threads created using
uv_thread_start() because libuv does not use GetCurrentThreadId()
for threads that were created otherwise.

Thus we must use a thread-local static boolean which we set to true
only on JS threads to distinguish JS threads from secondary threads,
thereby preventing deadlocks.

Signed-off-by: Gabriel Schulhof gabriel.schulhof@intel.com

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

CI: https://ci.nodejs.org/job/node-test-pull-request/30747/

CI: https://ci.nodejs.org/job/node-test-pull-request/30748/

CI: https://ci.nodejs.org/job/node-test-pull-request/30749/

@bnoordhuis @addaleax @gireeshpunathil @himself65 this is an alternative to #32823 based on #32823 (comment).

LGTM if CI passes

This is what I meant, yes. :-)

@gabrielschulhof Doesn’t this potentially do the wrong thing? We don’t want to know whether this is a JS thread, we want to know whether it’s the thread on which the loop runs to which this ThreadSafeFunction is associated with? I can see how it’s unlikely that that would pose a problem in practice, but still.

I definitely have a preference for #32823 here, and to me it seems like Ben’s original request for something that doesn’t create “invalid” uv_thread_ts has already been addressed.

@addaleax with this PR, the TSFN would be overly cautious. If you called a TSFN created on one JS thread from another JS thread, it would return napi_would_deadlock, which is incorrect, because one JS thread can definitely wait on another without deadlocking, however undesirable that may be. OTOH, if we allow one JS thread to wait on the other, you could get into a deadly embrace between two JS threads as they wait on one another. So, in a sense, we may want to return napi_would_deadlock when calling from any JS thread to any JS thread, because we cannot eliminate the probability, however unlikely, that a deadlock between at least two threads would ensue.

Of course, the correct solution is to not call napi_call_threadsafe_function() with napi_tsfn_blocking from any JS thread.

@addaleax also, in practice, as you also point out, the napi_would_deadlock is very unlikely to be returned for a JS-thread-to-another-JS-thread call. Most often by far (AFAICT) it will be returned for a JS-thread-to-the-same-JS-thread call. So, from a certain perspective, this modification is actually safer, because it detects the deadlock as reported in #32615 but it also prevents deadlocks resulting from JS-thread-to-another-JS-thread calls.

I will add this observation to the documentation surrounding napi_would_deadlock.

CI: https://ci.nodejs.org/job/node-test-pull-request/30763/

@gabrielschulhof Yeah, I guess my position is more based on the fact that thread_local variables usually are code smells.

What if an embedder decides to move running one Isolate/event loop combination from one thread to another? That probably not a common scenario, but it is for example something that was brought up in discussions that I have participated in.

Tbh, if you want to figure out whether you’re currently on a JS thread, I’d check Isolate::GetCurrent() != nullptr, similar to what we do in LowMemoryNotification() in util.h.

@addaleax checking v8::Isolate::GetCurrent() != nullptr sounds good. I'll make the change.

@addaleax ... until we can have a v8::Isolate on one thread, and the rest of the environment on another 😕 Well, hopefully not anytime soon 🤞

@addaleax if environments can migrate between threads as per the discussion in which you participated then, with #32823 behaviour would return to the pre-deadlock-check code whenever the environment was migrated. So, this PR would be the only solution.

Using v8::Isolate::GetCurrent() actually makes for the nicest diff of them all. The deadlock detection now uses @bnoordhuis' idea for a thread-local variable but via v8::Isolate, and is reduced to

$ git diff 96eceb7 -- src/node_api.cc

diff --git a/src/node_api.cc b/src/node_api.cc
index fad9cf72a9..89ec03b088 100644
--- a/src/node_api.cc
+++ b/src/node_api.cc
@@ -154,6 +154,14 @@ class ThreadSafeFunction : public node::AsyncResource {
         !is_closing) {
       if (mode == napi_tsfn_nonblocking) {
         return napi_queue_full;
+      } else if (v8::Isolate::GetCurrent() != nullptr) {
+        // Return `napi_would_deadlock` because waiting on the queue from a JS
+        // thread will prevent that same thread from removing items from the
+        // queue thereby causing deadlock. Deadlock can also be caused by one JS
+        // thread calling the thread-safe function of another JS thread because
+        // if two JS threads call each other's thread-safe functions blockingly
+        // when both their queues are full they will both deadlock.
+        return napi_would_deadlock;
       }
       cond->Wait(lock);
     }

where 96eceb7 is the commit before the deadlock detection was introduced.

@bnoordhuis @himself65 @addaleax please take another look!

CI: https://ci.nodejs.org/job/node-test-pull-request/30769/

CI: https://ci.nodejs.org/job/node-test-pull-request/30814/

@addaleax I also updated the comment accompanying the return of napi_would_deadlock to explain the two kinds of deadlock we're seeking to avoid.

We introduce status `napi_would_deadlock` to be used as a return status by `napi_call_threadsafe_function` if the call is made with `napi_tsfn_blocking` on the main thread and the queue is full. Fixes: #32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: #32860 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

Landed in d26ca06.

We introduce status `napi_would_deadlock` to be used as a return status by `napi_call_threadsafe_function` if the call is made with `napi_tsfn_blocking` on the main thread and the queue is full. Fixes: nodejs#32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: nodejs#32860 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

We introduce status `napi_would_deadlock` to be used as a return status by `napi_call_threadsafe_function` if the call is made with `napi_tsfn_blocking` on the main thread and the queue is full. Fixes: #32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: #32860 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

We introduce status `napi_would_deadlock` to be used as a return status by `napi_call_threadsafe_function` if the call is made with `napi_tsfn_blocking` on the main thread and the queue is full. Fixes: #32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: #32860 Backport-PR-URL: #32948 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

Notable changes: - deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha) [#32971](#32971) - doc: add juanarbol as collaborator (Juan José Arboleda) [#32906](#32906) - http: doc deprecate abort and improve docs (Robert Nagy) [#32807](#32807) - module: do not warn when accessing `__esModule` of unfinished exports (Anna Henningsen) [#33048](#33048) - n-api: detect deadlocks in thread-safe function (Gabriel Schulhof) [#32860](#32860) - src: deprecate embedder APIs with replacements (Anna Henningsen) [#32858](#32858) - stream: - don't emit end after close (Robert Nagy) [#33076](#33076) - don't wait for close on legacy streams (Robert Nagy) [#33058](#33058) - pipeline should only destroy un-finished streams (Robert Nagy) [#32968](#32968) - vm: add importModuleDynamically option to compileFunction (Gus Caplan) [#32985](#32985) PR-URL: #33103

We introduce status `napi_would_deadlock` to be used as a return status by `napi_call_threadsafe_function` if the call is made with `napi_tsfn_blocking` on the main thread and the queue is full. Fixes: #32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: #32860 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: nodejs#33276 Refs: nodejs#32860

This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: nodejs#33276 Refs: nodejs#32860 PR-URL: nodejs#33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: #33276 Refs: #32860 PR-URL: #33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Apr 15, 2020

gabrielschulhof marked this pull request as draft April 15, 2020 04:04

gabrielschulhof mentioned this pull request Apr 15, 2020

n-api: fix win32 main thread detection #32823

Closed

2 tasks

gabrielschulhof force-pushed the tsfn-fix-win32-thread-self-thread_local branch from e9f9d39 to 6792324 Compare April 15, 2020 04:08

gabrielschulhof marked this pull request as ready for review April 15, 2020 06:15

gabrielschulhof requested review from addaleax, bnoordhuis, gireeshpunathil and himself65 April 15, 2020 06:15

himself65 mentioned this pull request Apr 15, 2020

n-api: detect deadlocks in thread-safe function #32689

Closed

4 tasks

himself65 approved these changes Apr 15, 2020

View reviewed changes

Comment thread src/node_api.cc Outdated

bnoordhuis approved these changes Apr 15, 2020

View reviewed changes

gabrielschulhof requested review from bnoordhuis and himself65 April 15, 2020 17:40

gabrielschulhof closed this Apr 19, 2020

gabrielschulhof deleted the tsfn-fix-win32-thread-self-thread_local branch April 21, 2020 02:58

BethGriggs mentioned this pull request Apr 27, 2020

v14.1.0 proposal #33103

Merged

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

addaleax mentioned this pull request May 12, 2020

test: skip test_threadsafe_function for debug build #33276

Closed

2 tasks

targos added dont-land-on-v10.x labels May 13, 2020

addaleax mentioned this pull request May 18, 2020

Revert "n-api: detect deadlocks in thread-safe function" #33453

Closed

3 tasks

n-api: detect deadlocks in thread-safe function (take two) by gabrielschulhof · Pull Request #32860 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: detect deadlocks in thread-safe function (take two) - #32860

n-api: detect deadlocks in thread-safe function (take two)#32860gabrielschulhof wants to merge 2 commits intonodejs:masternodejs/node:masterfrom gabrielschulhof:tsfn-fix-win32-thread-self-thread_localgabrielschulhof/node:tsfn-fix-win32-thread-self-thread_localCopy head branch name to clipboard

Conversation

gabrielschulhof commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Apr 15, 2020

Uh oh!

nodejs-github-bot commented Apr 15, 2020

Uh oh!

nodejs-github-bot commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

himself65 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

nodejs-github-bot commented Apr 15, 2020

Uh oh!

addaleax commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

gabrielschulhof commented Apr 15, 2020

Uh oh!

nodejs-github-bot commented Apr 15, 2020

Uh oh!

nodejs-github-bot commented Apr 17, 2020

Uh oh!

gabrielschulhof commented Apr 17, 2020

Uh oh!

gabrielschulhof commented Apr 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

n-api: detect deadlocks in thread-safe function (take two)#32860
gabrielschulhof wants to merge 2 commits into
nodejs:masterfrom
gabrielschulhof:tsfn-fix-win32-thread-self-thread_local

gabrielschulhof commented Apr 15, 2020 •
edited
Loading