From 076ba3150dedbc02419a11d026df0fb371b2f74f Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 16 Apr 2020 07:23:31 -0700 Subject: [PATCH] Revert "n-api: detect deadlocks in thread-safe function" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit aeb7084fe6446350ec032e9819746126811bf44f. The solution creates incorrect behaviour on Windows. Re: https://github.com/nodejs/node-addon-api/pull/697#issuecomment-612993476 Signed-off-by: Gabriel Schulhof PR-URL: https://github.com/nodejs/node/pull/32880 Reviewed-By: Ben Noordhuis Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Gireesh Punathil Reviewed-By: Chengzhong Wu Reviewed-By: Gerhard Stöbich --- doc/api/n-api.md | 21 +------ src/js_native_api_types.h | 10 +--- src/js_native_api_v8.cc | 3 +- src/node_api.cc | 5 -- .../test_threadsafe_function/binding.c | 55 ------------------- .../test_threadsafe_function/binding.gyp | 5 +- .../node-api/test_threadsafe_function/test.js | 11 +--- 7 files changed, 10 insertions(+), 100 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index eda1d94f7289e1..a38ebbd6f8614b 100644 --- a/doc/api/n-api.md +++ b/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`. ```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`. diff --git a/src/js_native_api_types.h b/src/js_native_api_types.h index c32c71c4d39334..7a49fc9f719b30 100644 --- a/src/js_native_api_types.h +++ b/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); diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index ef25c92e060592..422eff6d7c5b68 100644 --- a/src/js_native_api_v8.cc +++ b/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, diff --git a/src/node_api.cc b/src/node_api.cc index 552538c6f05fcf..fad9cf72a972c2 100644 --- a/src/node_api.cc +++ b/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(¤t_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 ref; diff --git a/test/node-api/test_threadsafe_function/binding.c b/test/node-api/test_threadsafe_function/binding.c index 9f2fa5f9bd21bc..c9c526153804c6 100644 --- 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 index 34587eed3dfb1f..b60352e05af103 100644 --- 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 index f5afe225f07624..3603d79ee6b5d3 100644 --- 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));