From 62c953e1ca2fe7c1ab55cdbebc27cd5421d16270 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 16 Apr 2020 07:23:31 -0700 Subject: [PATCH 1/2] 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 db3fb92cd439f9..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 1a01f2a29081ff..79e793b775e173 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)); From cdd7f4fca08ad75fc7edb568dbfd1d333d517fde Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 6 Apr 2020 10:16:15 -0700 Subject: [PATCH 2/2] 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: https://github.com/nodejs/node/issues/32615 Signed-off-by: Gabriel Schulhof PR-URL: https://github.com/nodejs/node/pull/32860 Reviewed-By: Ben Noordhuis Reviewed-By: Michael Dawson Reviewed-By: Zeyu Yang --- doc/api/n-api.md | 28 +++++++++- src/js_native_api_types.h | 10 +++- src/js_native_api_v8.cc | 3 +- src/node_api.cc | 23 ++++++++ .../test_threadsafe_function/binding.c | 55 +++++++++++++++++++ .../test_threadsafe_function/binding.gyp | 5 +- .../node-api/test_threadsafe_function/test.js | 11 +++- 7 files changed, 125 insertions(+), 10 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index a38ebbd6f8614b..0e07ed8f0fbb4d 100644 --- 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`. ```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 index 7a49fc9f719b30..c32c71c4d39334 100644 --- 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 index 79e793b775e173..1a01f2a29081ff 100644 --- 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 index fad9cf72a972c2..cb8bd4b482365e 100644 --- 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 index c9c526153804c6..9f2fa5f9bd21bc 100644 --- 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 index b60352e05af103..34587eed3dfb1f 100644 --- 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 index 3603d79ee6b5d3..f5afe225f07624 100644 --- 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' +}));