n-api: detect deadlocks in thread-safe function #32689

gabrielschulhof · 2020-04-06T17:45:45Z

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: @gabrielschulhof

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-04-06T17:48:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30503/

addaleax · 2020-04-06T17:51:12Z

src/node_api.cc

@@ -148,12 +149,15 @@ 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 the_thread = uv_thread_self();


Suggested change

uv_thread_t the_thread = uv_thread_self();

uv_thread_t current_thread = uv_thread_self();

addaleax · 2020-04-06T17:53:33Z

doc/api/n-api.md

+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.


Add a changes: entry for napi_create_threadsafe_function below?

gabrielschulhof · 2020-04-06T18:37:20Z

@addaleax I updated the variable name, added the changes: to napi_call_threadsafe_function() and re-worder the documentation about its return value a bit.

nodejs-github-bot · 2020-04-06T19:57:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30516/

nodejs-github-bot · 2020-04-06T20:55:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30517/ ✔️

legendecas · 2020-04-07T02:10:04Z

doc/api/n-api.md

+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/32689
+    description: Return `napi_would_deadlock` when called from the main thread.


Suggested change

description: Return `napi_would_deadlock` when called from the main thread.

description: Return `napi_would_deadlock` if called with `napi_tsfn_blocking` from the main thread and the queue is full.

Just another clarification...

mhdawson

LGTM

nodejs-github-bot · 2020-04-09T07:05:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30574/

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. PR-URL: nodejs#32689 Fixes: nodejs#32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

nodejs-github-bot · 2020-04-09T07:39:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30575/

nodejs-github-bot · 2020-04-09T08:33:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/30577/ ✔️

gabrielschulhof · 2020-04-09T09:44:34Z

Landed in aeb7084.

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. PR-URL: #32689 Fixes: #32615 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Notable changes: New file system APIs: * Added a new function, `fs.readv` (with sync and promisified versions). This function takes an array of `ArrayBufferView` elements and will write the data it reads sequentially to the buffers (Sk Sajidul Kadir). #32356 * A new overload is available for `fs.readSync`, which allows to optionally pass any of the `offset`, `length` and `position` parameters. #32460 Other changes: * dns: * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4 mapped IPv6 addresses (murgatroid99). #32183 * http: * The default maximum HTTP header size was changed from 8KB to 16KB (rosaxny). #32520 * n-api: * Calls to `napi_call_threadsafe_function` from the main thread can now return the `napi_would_deadlock` status in certain circumstances (Gabriel Schulhof). #32689 * util: * Added a new `maxStrLength` option to `util.inspect`, to control the maximum length of printed strings. Its default value is `Infinity` (rosaxny). #32392 * worker: * Added support for passing a `transferList` along with `workerData` to the `Worker` constructor (Juan José Arboleda). #32278 New core collaborators: With this release, we welcome three new Node.js core collaborators: * himself65. #32734 * flarna (Gerhard Stoebich). #32620 * mildsunrise (Alba Mendez). #32525 PR-URL: #32813

himself65 · 2020-04-15T06:47:30Z

just tip. downstream #32689 or #32860

gabrielschulhof added semver-minor PRs that contain new features and should be released in the next minor version. node-api Issues and PRs related to the Node-API. labels Apr 6, 2020

gabrielschulhof requested review from addaleax, legendecas and mhdawson April 6, 2020 17:45

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2020

gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 968f06d to f93f12e Compare April 6, 2020 17:47

gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from f93f12e to 7b258a1 Compare April 6, 2020 17:47

addaleax approved these changes Apr 6, 2020

View reviewed changes

jasnell approved these changes Apr 7, 2020

View reviewed changes

legendecas approved these changes Apr 7, 2020

View reviewed changes

devnexen approved these changes Apr 7, 2020

View reviewed changes

cjihrig approved these changes Apr 7, 2020

View reviewed changes

mhdawson approved these changes Apr 7, 2020

View reviewed changes

gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 439af5e to 6322b5d Compare April 9, 2020 07:04

gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 6322b5d to 929cdb4 Compare April 9, 2020 07:09

gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 929cdb4 to 75b2509 Compare April 9, 2020 07:10

gabrielschulhof force-pushed the tsfn-avoid-deadlock branch from 75b2509 to e5c61e8 Compare April 9, 2020 07:13

gabrielschulhof closed this Apr 9, 2020

targos mentioned this pull request Apr 13, 2020

v13.13.0 release proposal #32813

Merged

gabrielschulhof deleted the tsfn-avoid-deadlock branch April 13, 2020 18:34

gabrielschulhof mentioned this pull request Apr 15, 2020

n-api: detect deadlocks in thread-safe function (take two) #32860

Closed

2 tasks

targos added dont-land-on-v10.x labels Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: detect deadlocks in thread-safe function #32689

n-api: detect deadlocks in thread-safe function #32689

gabrielschulhof commented Apr 6, 2020 •

edited

Loading

nodejs-github-bot commented Apr 6, 2020

addaleax Apr 6, 2020

addaleax Apr 6, 2020

gabrielschulhof commented Apr 6, 2020

nodejs-github-bot commented Apr 6, 2020

nodejs-github-bot commented Apr 6, 2020 •

edited by gabrielschulhof

Loading

legendecas Apr 7, 2020

mhdawson left a comment

nodejs-github-bot commented Apr 9, 2020

nodejs-github-bot commented Apr 9, 2020

nodejs-github-bot commented Apr 9, 2020 •

edited by gabrielschulhof

Loading

gabrielschulhof commented Apr 9, 2020

himself65 commented Apr 15, 2020

	uv_thread_t the_thread = uv_thread_self();
	uv_thread_t current_thread = uv_thread_self();

	description: Return `napi_would_deadlock` when called from the main thread.
	description: Return `napi_would_deadlock` if called with `napi_tsfn_blocking` from the main thread and the queue is full.

n-api: detect deadlocks in thread-safe function #32689

n-api: detect deadlocks in thread-safe function #32689

Conversation

gabrielschulhof commented Apr 6, 2020 • edited Loading

Checklist

nodejs-github-bot commented Apr 6, 2020

addaleax Apr 6, 2020

Choose a reason for hiding this comment

addaleax Apr 6, 2020

Choose a reason for hiding this comment

gabrielschulhof commented Apr 6, 2020

nodejs-github-bot commented Apr 6, 2020

nodejs-github-bot commented Apr 6, 2020 • edited by gabrielschulhof Loading

legendecas Apr 7, 2020

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 9, 2020

nodejs-github-bot commented Apr 9, 2020

nodejs-github-bot commented Apr 9, 2020 • edited by gabrielschulhof Loading

gabrielschulhof commented Apr 9, 2020

himself65 commented Apr 15, 2020

gabrielschulhof commented Apr 6, 2020 •

edited

Loading

nodejs-github-bot commented Apr 6, 2020 •

edited by gabrielschulhof

Loading

nodejs-github-bot commented Apr 9, 2020 •

edited by gabrielschulhof

Loading