Skip to content

Commit

Permalink
n-api: detect deadlocks in thread-safe function
Browse files Browse the repository at this point in the history
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.

PR-URL: nodejs#32689
Fixes: nodejs#32615
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
Gabriel Schulhof committed Apr 9, 2020
1 parent b82d72c commit aeb7084
Show file tree
Hide file tree
Showing 7 changed files with 100 additions and 10 deletions.
21 changes: 19 additions & 2 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ typedef enum {
napi_date_expected,
napi_arraybuffer_expected,
napi_detachable_arraybuffer_expected,
napi_would_deadlock,
} napi_status;
```

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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`.

Expand Down
10 changes: 7 additions & 3 deletions src/js_native_api_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_),
Expand All @@ -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(&current_thread, &main_thread)) {
return napi_would_deadlock;
}
cond->Wait(lock);
}
Expand Down Expand Up @@ -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;
Expand Down
55 changes: 55 additions & 0 deletions test/node-api/test_threadsafe_function/binding.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion test/node-api/test_threadsafe_function/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
'targets': [
{
'target_name': 'binding',
'sources': ['binding.c']
'sources': [
'binding.c',
'../../js-native-api/common.c'
]
}
]
}
11 changes: 8 additions & 3 deletions test/node-api/test_threadsafe_function/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
}));

0 comments on commit aeb7084

Please sign in to comment.