Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Apr 6, 2020 •
edited
Loading

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

@gabrielschulhof 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
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 6, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 6, 2020

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

addaleax
addaleax approved these changes Apr 6, 2020
View reviewed changes
src/node_api.cc Outdated
Copy link
Member

@addaleax addaleax Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/n-api.md Outdated
Copy link
Member

@addaleax addaleax Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a changes: entry for napi_create_threadsafe_function below?

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 6, 2020

@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
Copy link
Collaborator

nodejs-github-bot commented Apr 6, 2020

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

@nodejs-github-bot
Copy link
Collaborator

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

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

legendecas
legendecas approved these changes Apr 7, 2020
View reviewed changes
doc/api/n-api.md Outdated
Copy link
Member

@legendecas legendecas Apr 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
mhdawson approved these changes Apr 7, 2020
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 9, 2020

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

n-api: detect deadlocks in thread-safe function
e5c61e8 
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
Copy link
Collaborator

nodejs-github-bot commented Apr 9, 2020

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

@nodejs-github-bot
Copy link
Collaborator

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

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

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 9, 2020

Landed in aeb7084.

gabrielschulhof pushed a commit that referenced this pull request Apr 9, 2020
n-api: detect deadlocks in thread-safe function
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>
targos pushed a commit that referenced this pull request Apr 12, 2020
@targos
n-api: detect deadlocks in thread-safe function
3907de7 
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>
@targos targos mentioned this pull request Apr 13, 2020
Merged
@gabrielschulhof gabrielschulhof deleted the tsfn-avoid-deadlock branch April 13, 2020 18:34
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
@BethGriggs
n-api: detect deadlocks in thread-safe function
45676be 
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>
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
4ca9571 
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
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
8130521 
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
targos added a commit that referenced this pull request Apr 14, 2020
@targos
2020-04-14, Version 13.13.0 (Current)
cf4c332 
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
Copy link
Member

himself65 commented Apr 15, 2020

just tip. downstream #32689 or #32860

@gabrielschulhof gabrielschulhof mentioned this pull request Apr 15, 2020
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@legendecas legendecas legendecas approved these changes

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@devnexen devnexen devnexen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

napi_call_threadsafe_function should error if called from the main thread

10 participants

@gabrielschulhof @nodejs-github-bot @himself65 @jasnell @addaleax @cjihrig @devnexen @legendecas @mhdawson @targos