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

Revert "n-api: detect deadlocks in thread-safe function" · nodejs/node@4c235b0 · GitHub

/ node Public

Commit 4c235b0

Browse files
addaleax
authored and
codebytere
committed
Revert "n-api: detect deadlocks in thread-safe function"
This reverts commit d26ca06 because it breaks running the tests in debug mode, as `v8::Isolate::GetCurrent()` is not allowed if no `Isolate` is active on the current thread. Refs: #33276 Refs: #32860 PR-URL: #33453 Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
1 parent 23cf39a commit 4c235b0

6 files changed

Lines changed: 18 additions & 109 deletions

File tree

‎doc/api/n-api.md‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,7 @@ typedef enum {
462462
napi_date_expected,
463463
napi_arraybuffer_expected,
464464
napi_detachable_arraybuffer_expected,
465-
napi_would_deadlock,
465+
napi_would_deadlock, /* unused */
466466
} napi_status;
467467
```
468468

@@ -5124,18 +5124,9 @@ preventing data from being successfully added to the queue. If set to
51245124
`napi_call_threadsafe_function()` never blocks if the thread-safe function was
51255125
created with a maximum queue size of 0.
51265126

5127-
As a special case, when `napi_call_threadsafe_function()` is called from a
5128-
JavaScript thread, it will return `napi_would_deadlock` if the queue is full
5129-
and it was called with `napi_tsfn_blocking`. The reason for this is that the
5130-
JavaScript thread is responsible for removing items from the queue, thereby
5131-
reducing their number. Thus, if it waits for room to become available on the
5132-
queue, then it will deadlock.
5133-
5134-
`napi_call_threadsafe_function()` will also return `napi_would_deadlock` if the
5135-
thread-safe function created on one JavaScript thread is called from another
5136-
JavaScript thread. The reason for this is to prevent a deadlock arising from the
5137-
possibility that the two JavaScript threads end up waiting on one another,
5138-
thereby both deadlocking.
5127+
`napi_call_threadsafe_function()` should not be called with `napi_tsfn_blocking`
5128+
from a JavaScript thread, because, if the queue is full, it may cause the
5129+
JavaScript thread to deadlock.
51395130

51405131
The actual call into JavaScript is controlled by the callback given via the
51415132
`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`.
52765267
added: v10.6.0
52775268
napiVersion: 4
52785269
changes:
5270+
- version: REPLACEME
5271+
pr-url: https://github.com/nodejs/node/pull/33453
5272+
description: >
5273+
Support for `napi_would_deadlock` has been reverted.
52795274
- version: v14.1.0
52805275
pr-url: https://github.com/nodejs/node/pull/32689
52815276
description: >
@@ -5298,13 +5293,13 @@ napi_call_threadsafe_function(napi_threadsafe_function func,
52985293
`napi_tsfn_nonblocking` to indicate that the call should return immediately
52995294
with a status of `napi_queue_full` whenever the queue is full.
53005295

5301-
This API will return `napi_would_deadlock` if called with `napi_tsfn_blocking`
5302-
from the main thread and the queue is full.
5296+
This API should not be called with `napi_tsfn_blocking` from a JavaScript
5297+
thread, because, if the queue is full, it may cause the JavaScript thread to
5298+
deadlock.
53035299

53045300
This API will return `napi_closing` if `napi_release_threadsafe_function()` was
5305-
called with `abort` set to `napi_tsfn_abort` from any thread.
5306-
5307-
The value is only added to the queue if the API returns `napi_ok`.
5301+
called with `abort` set to `napi_tsfn_abort` from any thread. The value is only
5302+
added to the queue if the API returns `napi_ok`.
53085303

53095304
This API may be called from any thread which makes use of `func`.
53105305

‎src/js_native_api_types.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ typedef enum {
8282
napi_date_expected,
8383
napi_arraybuffer_expected,
8484
napi_detachable_arraybuffer_expected,
85-
napi_would_deadlock
85+
napi_would_deadlock // unused
8686
} napi_status;
8787
// Note: when adding a new enum value to `napi_status`, please also update
8888
// * `const int last_status` in the definition of `napi_get_last_error_info()'

‎src/node_api.cc‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -155,29 +155,6 @@ 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-
181158
cond->Wait(lock);
182159
}
183160

‎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)
Back | FazBrowse Home | New Git URL