diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 2af31d3de038e7..f654685a040ee8 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -462,7 +462,7 @@ typedef enum { napi_date_expected, napi_arraybuffer_expected, napi_detachable_arraybuffer_expected, - napi_would_deadlock, + napi_would_deadlock, /* unused */ } napi_status; ``` @@ -5124,18 +5124,9 @@ 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. +`napi_call_threadsafe_function()` should not be called with `napi_tsfn_blocking` +from a JavaScript thread, because, if the queue is full, it may cause the +JavaScript thread to 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 @@ -5276,6 +5267,10 @@ This API may be called from any thread which makes use of `func`. added: v10.6.0 napiVersion: 4 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/33453 + description: > + Support for `napi_would_deadlock` has been reverted. - version: v14.1.0 pr-url: https://github.com/nodejs/node/pull/32689 description: > @@ -5298,13 +5293,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 should not be called with `napi_tsfn_blocking` from a JavaScript +thread, because, if the queue is full, it may cause the JavaScript thread to +deadlock. 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 c32c71c4d39334..7254019dd78750 100644 --- a/src/js_native_api_types.h +++ b/src/js_native_api_types.h @@ -82,7 +82,7 @@ typedef enum { napi_date_expected, napi_arraybuffer_expected, napi_detachable_arraybuffer_expected, - napi_would_deadlock + napi_would_deadlock // unused } 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()' diff --git a/src/node_api.cc b/src/node_api.cc index 098c5e03219396..b87690752b3d9a 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -155,29 +155,6 @@ 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 9f2fa5f9bd21bc..c9c526153804c6 100644 --- a/test/node-api/test_threadsafe_function/binding.c +++ b/test/node-api/test_threadsafe_function/binding.c @@ -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, diff --git a/test/node-api/test_threadsafe_function/binding.gyp b/test/node-api/test_threadsafe_function/binding.gyp index 34587eed3dfb1f..b60352e05af103 100644 --- a/test/node-api/test_threadsafe_function/binding.gyp +++ b/test/node-api/test_threadsafe_function/binding.gyp @@ -2,10 +2,7 @@ 'targets': [ { 'target_name': 'binding', - 'sources': [ - 'binding.c', - '../../js-native-api/common.c' - ] + 'sources': ['binding.c'] } ] } diff --git a/test/node-api/test_threadsafe_function/test.js b/test/node-api/test_threadsafe_function/test.js index f5afe225f07624..3603d79ee6b5d3 100644 --- a/test/node-api/test_threadsafe_function/test.js +++ b/test/node-api/test_threadsafe_function/test.js @@ -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));