FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

n-api: detect deadlocks in thread-safe function · nodejs/node@861eb39 · GitHub

/ node Public

Commit 861eb39

Browse files
Gabriel Schulhof
authored and
BethGriggs
committed
n-api: detect deadlocks in thread-safe function
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>
1 parent f9b8988 commit 861eb39

7 files changed

Lines changed: 125 additions & 10 deletions

File tree

‎doc/api/n-api.md‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -458,6 +458,7 @@ typedef enum {
458458
napi_date_expected,
459459
napi_arraybuffer_expected,
460460
napi_detachable_arraybuffer_expected,
461+
napi_would_deadlock,
461462
} napi_status;
462463
```
463464

@@ -5096,6 +5097,19 @@ preventing data from being successfully added to the queue. If set to
50965097
`napi_call_threadsafe_function()` never blocks if the thread-safe function was
50975098
created with a maximum queue size of 0.
50985099

5100+
As a special case, when `napi_call_threadsafe_function()` is called from a
5101+
JavaScript thread, it will return `napi_would_deadlock` if the queue is full
5102+
and it was called with `napi_tsfn_blocking`. The reason for this is that the
5103+
JavaScript thread is responsible for removing items from the queue, thereby
5104+
reducing their number. Thus, if it waits for room to become available on the
5105+
queue, then it will deadlock.
5106+
5107+
`napi_call_threadsafe_function()` will also return `napi_would_deadlock` if the
5108+
thread-safe function created on one JavaScript thread is called from another
5109+
JavaScript thread. The reason for this is to prevent a deadlock arising from the
5110+
possibility that the two JavaScript threads end up waiting on one another,
5111+
thereby both deadlocking.
5112+
50995113
The actual call into JavaScript is controlled by the callback given via the
51005114
`call_js_cb` parameter. `call_js_cb` is invoked on the main thread once for each
51015115
value that was placed into the queue by a successful call to
@@ -5232,6 +5246,12 @@ This API may be called from any thread which makes use of `func`.
52325246
<!-- YAML
52335247
added: v10.6.0
52345248
napiVersion: 4
5249+
changes:
5250+
- version: REPLACEME
5251+
pr-url: https://github.com/nodejs/node/pull/32689
5252+
description: >
5253+
Return `napi_would_deadlock` when called with `napi_tsfn_blocking` from
5254+
the main thread or a worker thread and the queue is full.
52355255
-->
52365256

52375257
```C
@@ -5249,9 +5269,13 @@ napi_call_threadsafe_function(napi_threadsafe_function func,
52495269
`napi_tsfn_nonblocking` to indicate that the call should return immediately
52505270
with a status of `napi_queue_full` whenever the queue is full.
52515271

5272+
This API will return `napi_would_deadlock` if called with `napi_tsfn_blocking`
5273+
from the main thread and the queue is full.
5274+
52525275
This API will return `napi_closing` if `napi_release_threadsafe_function()` was
5253-
called with `abort` set to `napi_tsfn_abort` from any thread. The value is only
5254-
added to the queue if the API returns `napi_ok`.
5276+
called with `abort` set to `napi_tsfn_abort` from any thread.
5277+
5278+
The value is only added to the queue if the API returns `napi_ok`.
52555279

52565280
This API may be called from any thread which makes use of `func`.
52575281

‎src/js_native_api_types.h‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,15 @@ typedef enum {
8282
napi_date_expected,
8383
napi_arraybuffer_expected,
8484
napi_detachable_arraybuffer_expected,
85+
napi_would_deadlock
8586
} napi_status;
8687
// Note: when adding a new enum value to `napi_status`, please also update
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).
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).
9094

9195
typedef napi_value (*napi_callback)(napi_env env,
9296
napi_callback_info info);

‎src/js_native_api_v8.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -740,6 +740,7 @@ 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",
743744
};
744745

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

756757
static_assert(
757758
NAPI_ARRAYSIZE(error_messages) == last_status + 1,

‎src/node_api.cc‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,29 @@ class ThreadSafeFunction : public node::AsyncResource {
155155
if (mode == napi_tsfn_nonblocking) {
156156
return napi_queue_full;
157157
}
158+
159+
// Here we check if there is a Node.js event loop running on this thread.
160+
// If there is, and our queue is full, we return `napi_would_deadlock`. We
161+
// do this for two reasons:
162+
//
163+
// 1. If this is the thread on which our own event loop runs then we
164+
// cannot wait here because that will prevent our event loop from
165+
// running and emptying the very queue on which we are waiting.
166+
//
167+
// 2. If this is not the thread on which our own event loop runs then we
168+
// still cannot wait here because that allows the following sequence of
169+
// events:
170+
//
171+
// 1. JSThread1 calls JSThread2 and blocks while its queue is full and
172+
// because JSThread2's queue is also full.
173+
//
174+
// 2. JSThread2 calls JSThread1 before it's had a chance to remove an
175+
// item from its own queue and blocks because JSThread1's queue is
176+
// also full.
177+
v8::Isolate* isolate = v8::Isolate::GetCurrent();
178+
if (isolate != nullptr && node::GetCurrentEventLoop(isolate) != nullptr)
179+
return napi_would_deadlock;
180+
158181
cond->Wait(lock);
159182
}
160183

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,60 @@ 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+
270324
// Module init
271325
static napi_value Init(napi_env env, napi_value exports) {
272326
size_t index;
@@ -305,6 +359,7 @@ static napi_value Init(napi_env env, napi_value exports) {
305359
DECLARE_NAPI_PROPERTY("StopThread", StopThread),
306360
DECLARE_NAPI_PROPERTY("Unref", Unref),
307361
DECLARE_NAPI_PROPERTY("Release", Release),
362+
DECLARE_NAPI_PROPERTY("TestDeadlock", TestDeadlock),
308363
};
309364

310365
NAPI_CALL(env, napi_define_properties(env, exports,

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

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

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,13 @@ 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));
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+
}));

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL