Skip to content

Commit d3d5eca

Browse files
author
Gabriel Schulhof
committed
Revert "n-api: detect deadlocks in thread-safe function"
This reverts commit aeb7084. The solution creates incorrect behaviour on Windows. Re: nodejs/node-addon-api#697 (comment) Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> PR-URL: #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>
1 parent a9da656 commit d3d5eca

7 files changed

Lines changed: 10 additions & 100 deletions

File tree

‎doc/api/n-api.md‎

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -457,7 +457,6 @@ typedef enum {
457457
napi_date_expected,
458458
napi_arraybuffer_expected,
459459
napi_detachable_arraybuffer_expected,
460-
napi_would_deadlock,
461460
} napi_status;
462461
```
463462

@@ -5096,12 +5095,6 @@ preventing data from being successfully added to the queue. If set to
50965095
`napi_call_threadsafe_function()` never blocks if the thread-safe function was
50975096
created with a maximum queue size of 0.
50985097

5099-
As a special case, when `napi_call_threadsafe_function()` is called from the
5100-
main thread, it will return `napi_would_deadlock` if the queue is full and it
5101-
was called with `napi_tsfn_blocking`. The reason for this is that the main
5102-
thread is responsible for reducing the number of items in the queue so, if it
5103-
waits for room to become available on the queue, then it will deadlock.
5104-
51055098
The actual call into JavaScript is controlled by the callback given via the
51065099
`call_js_cb` parameter. `call_js_cb` is invoked on the main thread once for each
51075100
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`.
52385231
<!-- YAML
52395232
added: v10.6.0
52405233
napiVersion: 4
5241-
changes:
5242-
- version: v13.13.0
5243-
pr-url: https://github.com/nodejs/node/pull/32689
5244-
description: >
5245-
Return `napi_would_deadlock` when called with `napi_tsfn_blocking` from
5246-
the main thread and the queue is full.
52475234
-->
52485235

52495236
```C
@@ -5261,13 +5248,9 @@ napi_call_threadsafe_function(napi_threadsafe_function func,
52615248
`napi_tsfn_nonblocking` to indicate that the call should return immediately
52625249
with a status of `napi_queue_full` whenever the queue is full.
52635250

5264-
This API will return `napi_would_deadlock` if called with `napi_tsfn_blocking`
5265-
from the main thread and the queue is full.
5266-
52675251
This API will return `napi_closing` if `napi_release_threadsafe_function()` was
5268-
called with `abort` set to `napi_tsfn_abort` from any thread.
5269-
5270-
The value is only added to the queue if the API returns `napi_ok`.
5252+
called with `abort` set to `napi_tsfn_abort` from any thread. The value is only
5253+
added to the queue if the API returns `napi_ok`.
52715254

52725255
This API may be called from any thread which makes use of `func`.
52735256

‎src/js_native_api_types.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,11 @@ typedef enum {
8282
napi_date_expected,
8383
napi_arraybuffer_expected,
8484
napi_detachable_arraybuffer_expected,
85-
napi_would_deadlock
8685
} napi_status;
8786
// Note: when adding a new enum value to `napi_status`, please also update
88-
// * `const int last_status` in the definition of `napi_get_last_error_info()'
89-
// in file js_native_api_v8.cc.
90-
// * `const char* error_messages[]` in file js_native_api_v8.cc with a brief
91-
// message explaining the error.
92-
// * the definition of `napi_status` in doc/api/n-api.md to reflect the newly
93-
// added value(s).
87+
// `const int last_status` in `napi_get_last_error_info()' definition,
88+
// in file js_native_api_v8.cc. Please also update the definition of
89+
// `napi_status` in doc/api/n-api.md to reflect the newly added value(s).
9490

9591
typedef napi_value (*napi_callback)(napi_env env,
9692
napi_callback_info info);

‎src/js_native_api_v8.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -740,7 +740,6 @@ const char* error_messages[] = {nullptr,
740740
"A date was expected",
741741
"An arraybuffer was expected",
742742
"A detachable arraybuffer was expected",
743-
"Main thread would deadlock",
744743
};
745744

746745
napi_status napi_get_last_error_info(napi_env env,
@@ -752,7 +751,7 @@ napi_status napi_get_last_error_info(napi_env env,
752751
// message in the `napi_status` enum each time a new error message is added.
753752
// We don't have a napi_status_last as this would result in an ABI
754753
// change each time a message was added.
755-
const int last_status = napi_would_deadlock;
754+
const int last_status = napi_detachable_arraybuffer_expected;
756755

757756
static_assert(
758757
NAPI_ARRAYSIZE(error_messages) == last_status + 1,

‎src/node_api.cc‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,6 @@ class ThreadSafeFunction : public node::AsyncResource {
129129
is_closing(false),
130130
context(context_),
131131
max_queue_size(max_queue_size_),
132-
main_thread(uv_thread_self()),
133132
env(env_),
134133
finalize_data(finalize_data_),
135134
finalize_cb(finalize_cb_),
@@ -149,15 +148,12 @@ class ThreadSafeFunction : public node::AsyncResource {
149148

150149
napi_status Push(void* data, napi_threadsafe_function_call_mode mode) {
151150
node::Mutex::ScopedLock lock(this->mutex);
152-
uv_thread_t current_thread = uv_thread_self();
153151

154152
while (queue.size() >= max_queue_size &&
155153
max_queue_size > 0 &&
156154
!is_closing) {
157155
if (mode == napi_tsfn_nonblocking) {
158156
return napi_queue_full;
159-
} else if (uv_thread_equal(&current_thread, &main_thread)) {
160-
return napi_would_deadlock;
161157
}
162158
cond->Wait(lock);
163159
}
@@ -438,7 +434,6 @@ class ThreadSafeFunction : public node::AsyncResource {
438434
// means we don't need the mutex to read them.
439435
void* context;
440436
size_t max_queue_size;
441-
uv_thread_t main_thread;
442437

443438
// These are variables accessed only from the loop thread.
444439
v8impl::Persistent<v8::Function> ref;

‎test/node-api/test_threadsafe_function/binding.c‎

Lines changed: 0 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -267,60 +267,6 @@ static napi_value StartThreadNoJsFunc(napi_env env, napi_callback_info info) {
267267
/** block_on_full */true, /** alt_ref_js_cb */true);
268268
}
269269

270-
static void DeadlockTestDummyMarshaller(napi_env env,
271-
napi_value empty0,
272-
void* empty1,
273-
void* empty2) {}
274-
275-
static napi_value TestDeadlock(napi_env env, napi_callback_info info) {
276-
napi_threadsafe_function tsfn;
277-
napi_status status;
278-
napi_value async_name;
279-
napi_value return_value;
280-
281-
// Create an object to store the returned information.
282-
NAPI_CALL(env, napi_create_object(env, &return_value));
283-
284-
// Create a string to be used with the thread-safe function.
285-
NAPI_CALL(env, napi_create_string_utf8(env,
286-
"N-API Thread-safe Function Deadlock Test",
287-
NAPI_AUTO_LENGTH,
288-
&async_name));
289-
290-
// Create the thread-safe function with a single queue slot and a single thread.
291-
NAPI_CALL(env, napi_create_threadsafe_function(env,
292-
NULL,
293-
NULL,
294-
async_name,
295-
1,
296-
1,
297-
NULL,
298-
NULL,
299-
NULL,
300-
DeadlockTestDummyMarshaller,
301-
&tsfn));
302-
303-
// Call the threadsafe function. This should succeed and fill the queue.
304-
NAPI_CALL(env, napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_blocking));
305-
306-
// Call the threadsafe function. This should not block, but return
307-
// `napi_would_deadlock`. We save the resulting status in an object to be
308-
// returned.
309-
status = napi_call_threadsafe_function(tsfn, NULL, napi_tsfn_blocking);
310-
add_returned_status(env,
311-
"deadlockTest",
312-
return_value,
313-
"Main thread would deadlock",
314-
napi_would_deadlock,
315-
status);
316-
317-
// Clean up the thread-safe function before returning.
318-
NAPI_CALL(env, napi_release_threadsafe_function(tsfn, napi_tsfn_release));
319-
320-
// Return the result.
321-
return return_value;
322-
}
323-
324270
// Module init
325271
static napi_value Init(napi_env env, napi_value exports) {
326272
size_t index;
@@ -359,7 +305,6 @@ static napi_value Init(napi_env env, napi_value exports) {
359305
DECLARE_NAPI_PROPERTY("StopThread", StopThread),
360306
DECLARE_NAPI_PROPERTY("Unref", Unref),
361307
DECLARE_NAPI_PROPERTY("Release", Release),
362-
DECLARE_NAPI_PROPERTY("TestDeadlock", TestDeadlock),
363308
};
364309

365310
NAPI_CALL(env, napi_define_properties(env, exports,

‎test/node-api/test_threadsafe_function/binding.gyp‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,7 @@
22
'targets': [
33
{
44
'target_name': 'binding',
5-
'sources': [
6-
'binding.c',
7-
'../../js-native-api/common.c'
8-
]
5+
'sources': ['binding.c']
96
}
107
]
118
}

‎test/node-api/test_threadsafe_function/test.js‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -210,13 +210,8 @@ new Promise(function testWithoutJSMarshaller(resolve) {
210210
}))
211211
.then((result) => assert.strictEqual(result.indexOf(0), -1))
212212

213-
// Start a child process to test rapid teardown.
213+
// Start a child process to test rapid teardown
214214
.then(() => testUnref(binding.MAX_QUEUE_SIZE))
215215

216-
// Start a child process with an infinite queue to test rapid teardown.
217-
.then(() => testUnref(0))
218-
219-
// Test deadlock prevention.
220-
.then(() => assert.deepStrictEqual(binding.TestDeadlock(), {
221-
deadlockTest: 'Main thread would deadlock'
222-
}));
216+
// Start a child process with an infinite queue to test rapid teardown
217+
.then(() => testUnref(0));

0 commit comments

Comments
 (0)