diff options
| -rw-r--r-- | doc/api/n-api.md | 21 | ||||
| -rw-r--r-- | src/js_native_api_types.h | 10 | ||||
| -rw-r--r-- | src/js_native_api_v8.cc | 3 | ||||
| -rw-r--r-- | src/node_api.cc | 5 | ||||
| -rw-r--r-- | test/node-api/test_threadsafe_function/binding.c | 55 | ||||
| -rw-r--r-- | test/node-api/test_threadsafe_function/binding.gyp | 5 | ||||
| -rw-r--r-- | test/node-api/test_threadsafe_function/test.js | 11 |
7 files changed, 100 insertions, 10 deletions
diff --git a/doc/api/n-api.md b/doc/api/n-api.md index a38ebbd6f8..eda1d94f72 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,12 @@ 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 @@ -5231,6 +5238,12 @@ 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 @@ -5248,9 +5261,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 7a49fc9f71..c32c71c4d3 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 422eff6d7c..ef25c92e06 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 fad9cf72a9..552538c6f0 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -129,6 +129,7 @@ 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_), @@ -148,12 +149,15 @@ 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); } @@ -434,6 +438,7 @@ 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; diff --git a/test/node-api/test_threadsafe_function/binding.c b/test/node-api/test_threadsafe_function/binding.c index c9c5261538..9f2fa5f9bd 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 b60352e05a..34587eed3d 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 3603d79ee6..f5afe225f0 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' +})); |