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.

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

n-api: make thread-safe-function calls properly #28606

Closed
wants to merge 1 commit into from
Closed

n-api: make thread-safe-function calls properly #28606

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Use NapiCallIntoModuleThrow() to execute the call into JavaScript and
the finalizer for consistency with the rest of the calls into the N-API
addon.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

n-api: make thread-safe-function calls properly
24a96bc 
Use `NapiCallIntoModuleThrow()` to execute the call into JavaScript and
the finalizer for consistency with the rest of the calls into the N-API
addon.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jul 9, 2019
addaleax
addaleax approved these changes Jul 9, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I assume it would be possible to add tests for this?

@gabrielschulhof
Copy link
Contributor Author

@addaleax I ran into this as I was working on the napi_env splitting. Doing the splitting involves going over every place in the code that calls into the N-API addon and making sure that both the shared and the correct local environment are available before control enters the addon. NapiCallIntoModule(Throw)? indicates such a place, but at the end, I realized that I hadn't visited any TSFN call sites. The tests still passed, because the shared environment remains correct, but were the tsfn test to make calls to the new APIs (which I'm calling napi_(get|set)_instance_data those would fail because the local environment would be set incorrectly.

I guess we need a test suite for NapiCallIntoModule - for example, we could open handle scopes and not close them. The problem is that, when we add a new way of calling into the addon, if we do not remember to use NapiCallIntoModule – the mistake I made with TSFN – then the test won't capture that new "entry point".

Either way I'm finding it increasingly important that we test the off-nominal operation of N-API.

mhdawson
mhdawson approved these changes Jul 9, 2019
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

mhdawson
mhdawson approved these changes Jul 9, 2019
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 Jul 10, 2019 •
edited by gabrielschulhof
Loading

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

  • build was yellow.

@gabrielschulhof
Copy link
Contributor Author

Landed in f5b40b2.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jul 11, 2019
n-api: make thread-safe-function calls properly
f5b40b2 
Use `NapiCallIntoModuleThrow()` to execute the call into JavaScript and
the finalizer for consistency with the rest of the calls into the N-API
addon.

PR-URL: nodejs#28606
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof gabrielschulhof deleted the n-api-tsfn-call-properly branch July 11, 2019 07:33
targos pushed a commit that referenced this pull request Jul 20, 2019
@targos
n-api: make thread-safe-function calls properly
44de431 
Use `NapiCallIntoModuleThrow()` to execute the call into JavaScript and
the finalizer for consistency with the rest of the calls into the N-API
addon.

PR-URL: #28606
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This was referenced Jul 23, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

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
@gabrielschulhof @nodejs-github-bot @bnoordhuis @addaleax @cjihrig @mhdawson