From d26ca06c16f497ffa5ac4845a27922d5058a9318 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 6 Apr 2020 10:16:15 -0700 Subject: [PATCH] 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 c31eb17eeb7126..de588519849dde 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -458,6 +458,7 @@ typedef enum { napi_date_expected, napi_arraybuffer_expected, napi_detachable_arraybuffer_expected, + napi_would_deadlock, } napi_status; ``` @@ -5096,6 +5097,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 @@ -5232,6 +5246,12 @@ This API may be called from any thread which makes use of `func`. ```C @@ -5249,9 +5269,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 422eff6d7c5b68..ef25c92e060592 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' +}));