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

[v13.x] Backport thread-safe function deadlock detection fix by gabrielschulhof · Pull Request #32948 · nodejs/node · GitHub

/ node Public

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

Closed
gabrielschulhof wants to merge 2 commits into
nodejs:v13.x-stagingfrom
gabrielschulhof:backport-deadlock-detection-to-v13.x
ConversationCommits (2)ChecksFiles changed
Closed

[v13.x] Backport thread-safe function deadlock detection fix#32948
gabrielschulhof wants to merge 2 commits into
nodejs:v13.x-stagingfrom
gabrielschulhof:backport-deadlock-detection-to-v13.x

Conversation

gabrielschulhof commented Apr 20, 2020

Copy link
Copy Markdown
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
Revert "n-api: detect deadlocks in thread-safe function"
62c953e 
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>
n-api: detect deadlocks in thread-safe function
cdd7f4f 
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 added node-api Issues and PRs related to the Node-API. v13.x labels Apr 20, 2020
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 20, 2020

gabrielschulhof commented Apr 20, 2020

Copy link
Copy Markdown
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 commented Apr 21, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 21, 2020

Copy link
Copy Markdown
Collaborator

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

gabrielschulhof commented Apr 21, 2020

Copy link
Copy Markdown
Contributor Author

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

gabrielschulhof mentioned this pull request Apr 23, 2020
Closed
mhdawson approved these changes Apr 24, 2020
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

mhdawson commented Apr 24, 2020

Copy link
Copy Markdown
Member

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

addaleax commented Apr 24, 2020

Copy link
Copy Markdown
Member

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

targos commented Apr 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

Edit: ref: nodejs/Release#487

gabrielschulhof commented Apr 26, 2020

Copy link
Copy Markdown
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
Revert "n-api: detect deadlocks in thread-safe function"
cc0395c 
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
n-api: detect deadlocks in thread-safe function
fd4320c 
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 commented Apr 28, 2020

Copy link
Copy Markdown
Member

Landed in ea85768...fd4320c

targos closed this Apr 28, 2020
gabrielschulhof deleted the backport-deadlock-detection-to-v13.x branch January 28, 2021 00:14
gabrielschulhof restored the backport-deadlock-detection-to-v13.x branch January 28, 2021 05:33
gabrielschulhof deleted the backport-deadlock-detection-to-v13.x branch February 3, 2021 07:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

legendecas legendecas approved these changes
addaleax Awaiting requested review from addaleax
cjihrig Awaiting requested review from cjihrig
+1 more reviewer
mhdawson mhdawson 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL