-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Revert "n-api: detect deadlocks in thread-safe function" #32880
Revert "n-api: detect deadlocks in thread-safe function" #32880
Conversation
This reverts commit aeb7084. The solution creates incorrect behaviour on Windows. Re: nodejs/node-addon-api#697 (comment) Signed-off-by: Gabriel Schulhof <[email protected]>
Let's revert this until we come up with a better solution in #32860. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 fast-track?
aeb7084 went out in 13.13.0. Is the issue serious enough that this revert should be pulled into 14.0.0 or can it wait for the next patch release? cc FYI @BethGriggs |
@richardlau I would pull it into v14.0.0. As a revert, this is very low-risk. |
@richardlau the issue is pretty serious. On Windows a secondary thread will receive a heretofore unseen non-ok status message even though it would not have caused a deadlock. This is just plain wrong. Pulling this out of v14.0.0 is definitely warranted IMO. |
@richardlau to be clear: I strongly believe that this PR should be a part of v14.0.0. |
That's fine, I just wanted to make sure that @BethGriggs was aware of it if that was the case. |
This reverts commit aeb7084. The solution creates incorrect behaviour on Windows. Re: nodejs/node-addon-api#697 (comment) Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #32880 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
Landed in d3d5eca. |
This reverts commit aeb7084. The solution creates incorrect behaviour on Windows. Re: nodejs/node-addon-api#697 (comment) Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #32880 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
This reverts commit aeb7084. The solution creates incorrect behaviour on Windows. Re: nodejs/node-addon-api#697 (comment) Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: nodejs#32880 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
This reverts commit aeb7084. The solution creates incorrect behaviour on Windows. Re: nodejs/node-addon-api#697 (comment) Signed-off-by: Gabriel Schulhof <[email protected]> PR-URL: #32880 Backport-PR-URL: #32948 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
This reverts commit aeb7084.
The solution creates incorrect behaviour on Windows.
Re: nodejs/node-addon-api#697 (comment)
Signed-off-by: @gabrielschulhof
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes