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.

Fixes: #32615
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #32860
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
  • Loading branch information
Gabriel Schulhof authored and BethGriggs committed Apr 27, 2020
1 parent f9b8988 commit 861eb39
Show file tree
Hide file tree
Showing 7 changed files with 125 additions and 10 deletions.
28 changes: 26 additions & 2 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ typedef enum {
napi_date_expected,
napi_arraybuffer_expected,
napi_detachable_arraybuffer_expected,
napi_would_deadlock,
} napi_status;
```

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -5232,6 +5246,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 or a worker thread and the queue is full.
-->

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

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
23 changes: 23 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

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 861eb39

Please sign in to comment.