OSSForks/node
1
0
Fork 0
mirror of https://github.com/zebrajr/node.git synced 2026-01-15 12:15:26 +00:00
Code Issues Actions 8 Packages Projects Releases Wiki Activity

Revert "n-api: detect deadlocks in thread-safe function"

Browse Source 
This reverts commit aeb7084fe6.

The solution creates incorrect behaviour on Windows.

Re: https://github.com/nodejs/node-addon-api/pull/697#issuecomment-612993476
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: https://github.com/nodejs/node/pull/32880
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
This commit is contained in:
Gabriel Schulhof
2020-04-16 07:23:31 -07:00
parent a9da65699a
commit d3d5eca657
7 changed files with 10 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
doc/api/n-api.md
View File

@@ -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`.
<!-- YAML
added: v10.6.0
napiVersion: 4
changes:
- version: v13.13.0
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
@@ -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`.

10
src/js_native_api_types.h
View File

@@ -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);

3
src/js_native_api_v8.cc
View File

@@ -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,

5
src/node_api.cc
View File

@@ -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(&current_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<v8::Function> ref;

55
test/node-api/test_threadsafe_function/binding.c
View File

@@ -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,

5
test/node-api/test_threadsafe_function/binding.gyp
View File

@@ -2,10 +2,7 @@
'targets': [
{
'target_name': 'binding',
'sources': [
'binding.c',
'../../js-native-api/common.c'
]
'sources': ['binding.c']
}
]
}

11
test/node-api/test_threadsafe_function/test.js
View File

@@ -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));