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

[gabrielschulhof]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[gabrielschulhof]

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

[himself65]

LGTM if CI passes

[bnoordhuis]

This is what I meant, yes. :-)

[addaleax]

@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.

[gabrielschulhof]

@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.

[gabrielschulhof]

@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.

[nodejs-github-bot]

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

[addaleax]

@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.

[gabrielschulhof]

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

[gabrielschulhof]

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

[gabrielschulhof]

@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.

[gabrielschulhof]

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.

[gabrielschulhof]

@bnoordhuis @himself65 @addaleax please take another look!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[gabrielschulhof]

@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.

[gabrielschulhof]

Landed in d26ca06.