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

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: David Carlier <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
nodejs · Jun 30, 2020 · f4cfe94 · f4cfe94
1 parent 1e6b600
commit f4cfe94
Show file tree

Hide file tree

Showing 6 changed files with 18 additions and 109 deletions.
diff --git a/doc/api/n-api.md b/doc/api/n-api.md
@@ -462,7 +462,7 @@ typedef enum {
   napi_date_expected,
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
-  napi_would_deadlock,
+  napi_would_deadlock,  /* unused */
 } napi_status;
 ```
 
@@ -5124,18 +5124,9 @@ 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.
+`napi_call_threadsafe_function()` should not be called with `napi_tsfn_blocking`
+from a JavaScript thread, because, if the queue is full, it may cause the
+JavaScript thread to 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
@@ -5276,6 +5267,10 @@ This API may be called from any thread which makes use of `func`.
 added: v10.6.0
 napiVersion: 4
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/33453
+    description: >
+      Support for `napi_would_deadlock` has been reverted.
   - version: v14.1.0
     pr-url: https://github.com/nodejs/node/pull/32689
     description: >
@@ -5298,13 +5293,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 should not be called with `napi_tsfn_blocking` from a JavaScript
+thread, because, if the queue is full, it may cause the JavaScript thread to
+deadlock.
 
 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,7 +82,7 @@ typedef enum {
   napi_date_expected,
   napi_arraybuffer_expected,
   napi_detachable_arraybuffer_expected,
-  napi_would_deadlock
+  napi_would_deadlock  // unused
 } 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()'

diff --git a/src/node_api.cc b/src/node_api.cc
@@ -155,29 +155,6 @@ 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,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,

diff --git a/test/node-api/test_threadsafe_function/binding.gyp b/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']
     }
   ]
 }
diff --git a/test/node-api/test_threadsafe_function/test.js b/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));