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

This reverts commit aeb7084.

The solution creates incorrect behaviour on Windows.

Re: nodejs/node-addon-api#697 (comment)
Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #32880
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>

doc/api/n-api.md

@@ -457,7 +457,6 @@ typedef enum {
   napi_date_expected,
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
-  napi_would_deadlock,
 } napi_status;
 ```
 
@@ -5096,12 +5095,6 @@ 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 the
-main 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 main
-thread is responsible for reducing the number of items in the queue so, if it
-waits for room to become available on the queue, then it will deadlock.
-
 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
@@ -5238,12 +5231,6 @@ This API may be called from any thread which makes use of `func`.
 <!-- YAML
 added: v10.6.0
 napiVersion: 4
-changes:
-  - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/32689
-    description: >
-      Return `napi_would_deadlock` when called with `napi_tsfn_blocking` from
-      the main thread and the queue is full.
 -->
 
 ```C
@@ -5261,13 +5248,9 @@ 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`.
 

src/js_native_api_types.h

@@ -82,15 +82,11 @@ 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 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).
+// `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).
 
 typedef napi_value (*napi_callback)(napi_env env,
                                     napi_callback_info info);

src/js_native_api_v8.cc

@@ -740,7 +740,6 @@ 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,
@@ -752,7 +751,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_would_deadlock;
+  const int last_status = napi_detachable_arraybuffer_expected;
 
   static_assert(
       NAPI_ARRAYSIZE(error_messages) == last_status + 1,

src/node_api.cc

@@ -129,7 +129,6 @@ class ThreadSafeFunction : public node::AsyncResource {
       is_closing(false),
       context(context_),
       max_queue_size(max_queue_size_),
-      main_thread(uv_thread_self()),
       env(env_),
       finalize_data(finalize_data_),
       finalize_cb(finalize_cb_),
@@ -149,15 +148,12 @@ class ThreadSafeFunction : public node::AsyncResource {
 
   napi_status Push(void* data, napi_threadsafe_function_call_mode mode) {
     node::Mutex::ScopedLock lock(this->mutex);
-    uv_thread_t current_thread = uv_thread_self();
 
     while (queue.size() >= max_queue_size &&
         max_queue_size > 0 &&
         !is_closing) {
       if (mode == napi_tsfn_nonblocking) {
         return napi_queue_full;
-      } else if (uv_thread_equal(&current_thread, &main_thread)) {
-        return napi_would_deadlock;
       }
       cond->Wait(lock);
     }
@@ -438,7 +434,6 @@ class ThreadSafeFunction : public node::AsyncResource {
   // means we don't need the mutex to read them.
   void* context;
   size_t max_queue_size;
-  uv_thread_t main_thread;
 
   // These are variables accessed only from the loop thread.
   v8impl::Persistent<v8::Function> ref;

test/node-api/test_threadsafe_function/binding.c

@@ -267,60 +267,6 @@ 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;
@@ -359,7 +305,6 @@ 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,

test/node-api/test_threadsafe_function/binding.gyp

@@ -2,10 +2,7 @@
   'targets': [
     {
       'target_name': 'binding',
-      'sources': [
-        'binding.c',
-        '../../js-native-api/common.c'
-      ]
+      'sources': ['binding.c']
     }
   ]
 }

test/node-api/test_threadsafe_function/test.js

@@ -210,13 +210,8 @@ 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))
-
-// Test deadlock prevention.
-.then(() => assert.deepStrictEqual(binding.TestDeadlock(), {
-  deadlockTest: 'Main thread would deadlock'
-}));
+// Start a child process with an infinite queue to test rapid teardown
+.then(() => testUnref(0));