n-api: detect deadlocks in thread-safe function

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>
nodejs · BridgeAR · Apr 29, 2020 · Apr 12, 2020 · Apr 16, 2020 · Apr 6, 2020
commit fd4320c49f0731ab8742cd2dbe871f20a7037ebb
diff --git a/doc/api/n-api.md b/doc/api/n-api.md
@@ -457,6 +457,7 @@ typedef enum {
   napi_date_expected,
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
+  napi_would_deadlock,
 } napi_status;
 ```
 
@@ -5095,6 +5096,19 @@ preventing data from being successfully added to the queue. If set to
 `napi_call_threadsafe_function()` never blocks if the thread-safe function was
 created with a maximum queue size of 0.
 
+As a special case, when `napi_call_threadsafe_function()` is called from a
+JavaScript thread, it will return `napi_would_deadlock` if the queue is full
+and it was called with `napi_tsfn_blocking`. The reason for this is that the
+JavaScript thread is responsible for removing items from the queue, thereby
+reducing their number. Thus, if it waits for room to become available on the
+queue, then it will deadlock.
+
+`napi_call_threadsafe_function()` will also return `napi_would_deadlock` if the
+thread-safe function created on one JavaScript thread is called from another
+JavaScript thread. The reason for this is to prevent a deadlock arising from the
+possibility that the two JavaScript threads end up waiting on one another,
+thereby both deadlocking.
+
 The actual call into JavaScript is controlled by the callback given via the
 `call_js_cb` parameter. `call_js_cb` is invoked on the main thread once for each
 value that was placed into the queue by a successful call to
@@ -5231,6 +5245,12 @@ This API may be called from any thread which makes use of `func`.
 <!-- YAML
 added: v10.6.0
 napiVersion: 4
+changes:
+  - version: v13.13.0
+    pr-url: https://github.com/nodejs/node/pull/32689
+    description: >
+      Return `napi_would_deadlock` when called with `napi_tsfn_blocking` from
+      the main thread or a worker thread and the queue is full.
 -->
 
 ```C
@@ -5248,9 +5268,13 @@ napi_call_threadsafe_function(napi_threadsafe_function func,
   `napi_tsfn_nonblocking` to indicate that the call should return immediately
   with a status of `napi_queue_full` whenever the queue is full.
 
+This API will return `napi_would_deadlock` if called with `napi_tsfn_blocking`
+from the main thread and the queue is full.
+
 This API will return `napi_closing` if `napi_release_threadsafe_function()` was
-called with `abort` set to `napi_tsfn_abort` from any thread. The value is only
-added to the queue if the API returns `napi_ok`.
+called with `abort` set to `napi_tsfn_abort` from any thread.
+
+The value is only added to the queue if the API returns `napi_ok`.
 
 This API may be called from any thread which makes use of `func`.
 

diff --git a/src/js_native_api_types.h b/src/js_native_api_types.h
@@ -82,11 +82,15 @@ typedef enum {
   napi_date_expected,
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
+  napi_would_deadlock
 } napi_status;
 // Note: when adding a new enum value to `napi_status`, please also update
-// `const int last_status` in `napi_get_last_error_info()' definition,
-// in file js_native_api_v8.cc. Please also update the definition of
-// `napi_status` in doc/api/n-api.md to reflect the newly added value(s).
+//   * `const int last_status` in the definition of `napi_get_last_error_info()'
+//     in file js_native_api_v8.cc.
+//   * `const char* error_messages[]` in file js_native_api_v8.cc with a brief
+//     message explaining the error.
+//   * the definition of `napi_status` in doc/api/n-api.md to reflect the newly
+//     added value(s).
 
 typedef napi_value (*napi_callback)(napi_env env,
                                     napi_callback_info info);

diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -740,6 +740,7 @@ const char* error_messages[] = {nullptr,
                                 "A date was expected",
                                 "An arraybuffer was expected",
                                 "A detachable arraybuffer was expected",
+                                "Main thread would deadlock",
 };
 
 napi_status napi_get_last_error_info(napi_env env,
@@ -751,7 +752,7 @@ napi_status napi_get_last_error_info(napi_env env,
   // message in the `napi_status` enum each time a new error message is added.
   // We don't have a napi_status_last as this would result in an ABI
   // change each time a message was added.
-  const int last_status = napi_detachable_arraybuffer_expected;
+  const int last_status = napi_would_deadlock;
 
   static_assert(
       NAPI_ARRAYSIZE(error_messages) == last_status + 1,

diff --git a/src/node_api.cc b/src/node_api.cc
@@ -155,6 +155,29 @@ class ThreadSafeFunction : public node::AsyncResource {
       if (mode == napi_tsfn_nonblocking) {
         return napi_queue_full;
       }
+
+      // Here we check if there is a Node.js event loop running on this thread.
+      // If there is, and our queue is full, we return `napi_would_deadlock`. We
+      // do this for two reasons:
+      //
+      // 1. If this is the thread on which our own event loop runs then we
+      //    cannot wait here because that will prevent our event loop from
+      //    running and emptying the very queue on which we are waiting.
+      //
+      // 2. If this is not the thread on which our own event loop runs then we
+      //    still cannot wait here because that allows the following sequence of
+      //    events:
+      //
+      //    1. JSThread1 calls JSThread2 and blocks while its queue is full and
+      //       because JSThread2's queue is also full.
+      //
+      //    2. JSThread2 calls JSThread1 before it's had a chance to remove an
+      //       item from its own queue and blocks because JSThread1's queue is
+      //       also full.
+      v8::Isolate* isolate = v8::Isolate::GetCurrent();
+      if (isolate != nullptr && node::GetCurrentEventLoop(isolate) != nullptr)
+        return napi_would_deadlock;
+
       cond->Wait(lock);
     }
 

diff --git a/test/node-api/test_threadsafe_function/binding.c b/test/node-api/test_threadsafe_function/binding.c
@@ -267,6 +267,60 @@ static napi_value StartThreadNoJsFunc(napi_env env, napi_callback_info info) {
     /** block_on_full */true, /** alt_ref_js_cb */true);
 }
 
+static void DeadlockTestDummyMarshaller(napi_env env,
+                                        napi_value empty0,
+                                        void* empty1,
+                                        void* empty2) {}
+
+static napi_value TestDeadlock(napi_env env, napi_callback_info info) {
+  napi_threadsafe_function tsfn;
+  napi_status status;
+  napi_value async_name;
+  napi_value return_value;
+
+  // Create an object to store the returned information.
+  NAPI_CALL(env, napi_create_object(env, &return_value));
+
+  // Create a string to be used with the thread-safe function.
+  NAPI_CALL(env, napi_create_string_utf8(env,
+                                     "N-API Thread-safe Function Deadlock Test",
+                                     NAPI_AUTO_LENGTH,
+                                     &async_name));
+
+  // Create the thread-safe function with a single queue slot and a single thread.
+  NAPI_CALL(env, napi_create_threadsafe_function(env,
+                                                 NULL,
+                                                 NULL,
+                                                 async_name,
+                                                 1,
+                                                 1,
+                                                 NULL,
+                                                 NULL,
+                                                 NULL,
+                                                 DeadlockTestDummyMarshaller,
+                                                 &tsfn));
+
+  // Call the threadsafe function. This should succeed and fill the queue.
+  NAPI_CALL(env, napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_blocking));
+
+  // Call the threadsafe function. This should not block, but return
+  // `napi_would_deadlock`. We save the resulting status in an object to be
+  // returned.
+  status = napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_blocking);
+  add_returned_status(env,
+                      "deadlockTest",
+                      return_value,
+                      "Main thread would deadlock",
+                      napi_would_deadlock,
+                      status);
+
+  // Clean up the thread-safe function before returning.
+  NAPI_CALL(env, napi_release_threadsafe_function(tsfn, napi_tsfn_release));
+
+  // Return the result.
+  return return_value;
+}
+
 // Module init
 static napi_value Init(napi_env env, napi_value exports) {
   size_t index;
@@ -305,6 +359,7 @@ static napi_value Init(napi_env env, napi_value exports) {
     DECLARE_NAPI_PROPERTY("StopThread", StopThread),
     DECLARE_NAPI_PROPERTY("Unref", Unref),
     DECLARE_NAPI_PROPERTY("Release", Release),
+    DECLARE_NAPI_PROPERTY("TestDeadlock", TestDeadlock),
   };
 
   NAPI_CALL(env, napi_define_properties(env, exports,

diff --git a/test/node-api/test_threadsafe_function/binding.gyp b/test/node-api/test_threadsafe_function/binding.gyp
@@ -2,7 +2,10 @@
   'targets': [
     {
       'target_name': 'binding',
-      'sources': ['binding.c']
+      'sources': [
+        'binding.c',
+        '../../js-native-api/common.c'
+      ]
     }
   ]
 }
diff --git a/test/node-api/test_threadsafe_function/test.js b/test/node-api/test_threadsafe_function/test.js
@@ -210,8 +210,13 @@ new Promise(function testWithoutJSMarshaller(resolve) {
 }))
 .then((result) => assert.strictEqual(result.indexOf(0), -1))
 
-// Start a child process to test rapid teardown
+// Start a child process to test rapid teardown.
 .then(() => testUnref(binding.MAX_QUEUE_SIZE))
 
-// Start a child process with an infinite queue to test rapid teardown
-.then(() => testUnref(0));
+// Start a child process with an infinite queue to test rapid teardown.
+.then(() => testUnref(0))
+
+// Test deadlock prevention.
+.then(() => assert.deepStrictEqual(binding.TestDeadlock(), {
+  deadlockTest: 'Main thread would deadlock'
+}));