Skip to content
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.

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

[v13.x] Backport thread-safe function deadlock detection fix #32948

Conversation

gabrielschulhof
Copy link
Contributor

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

Gabriel Schulhof added 2 commits April 20, 2020 08:36
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: nodejs#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>
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: nodejs#32615
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: nodejs#32860
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@gabrielschulhof gabrielschulhof added node-api Issues and PRs related to the Node-API. v13.x labels Apr 20, 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 20, 2020
@gabrielschulhof
Copy link
Contributor Author

The backport would normally apply cleanly, but the resulting doc entry would have a REPLACEME tag for the `napi_call_threadsafe_function() entry indicating the change that brings deadlock detection, whereas the deadlock detection was not reverted out of the v13.x branch like it was in master, so that tag has to be manually updated to stay as "v13.13.0".

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof
Copy link
Contributor Author

This PR is also not semver-minor for that reason.

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

@mhdawson
Copy link
Member

I'm not sure at this point if there will be a another 13.x release.

@addaleax
Copy link
Member

Right, any further v13.x Release would be a maintenance release, where I believe this wouldn’t fit in.

@targos
Copy link
Member

targos commented Apr 24, 2020

There will be one last regular v13.x release next week.

Edit: ref: nodejs/Release#487

@gabrielschulhof
Copy link
Contributor Author

@targos it would be good if this made it into the final v13.x release because, as things stand, v13.x is buggy on Windows.

targos pushed a commit that referenced this pull request Apr 28, 2020
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
Backport-PR-URL: #32948
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>
targos pushed a commit that referenced this pull request Apr 28, 2020
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
Backport-PR-URL: #32948
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
@targos
Copy link
Member

targos commented Apr 28, 2020

Landed in ea85768...fd4320c

@targos targos closed this Apr 28, 2020
@gabrielschulhof gabrielschulhof deleted the backport-deadlock-detection-to-v13.x branch January 28, 2021 00:14
@gabrielschulhof gabrielschulhof restored the backport-deadlock-detection-to-v13.x branch January 28, 2021 05:33
@gabrielschulhof gabrielschulhof deleted the backport-deadlock-detection-to-v13.x branch February 3, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants