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

src: restore ability to run under NAPI_EXPERIMENTAL #1409

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gabrielschulhof
Copy link
Contributor

Since we made the default for Node.js core finalizers synchronous for users running with NAPI_EXPERIMENTAL and introduced env->CheckGCAccess() in Node.js core, we must now defer all finalizers in node-addon-api, because our users likely make non-gc-safe Node-API calls from existing finalizers. To that end,

  • Use the NAPI_VERSION environment variable to detect whether NAPI_EXPERIMENTAL should be on, and add it to the defines if NAPI_VERSION is set to NAPI_VERSION_EXPERIMENTAL, i.e. 2147483647.
  • When building with NAPI_EXPERIMENTAL,
    • render all finalizers asynchronous, and
    • expect napi_cannot_run_js instead of napi_exception_pending.

@mhdawson
Copy link
Member

I might have been thinking of another change but I thought we had made it opt-in with another define? @legendecas is that right?

@legendecas
Copy link
Member

because our users likely make non-gc-safe Node-API calls from existing finalizers

I'm afraid that I didn't understand this assumption. Is this assumption unique to node-addon-api, not the node-api c layer?

@gabrielschulhof gabrielschulhof force-pushed the restore-ability-to-run-with-NAPI_EXPERIMENTAL branch 2 times, most recently from 8f78a76 to 8b19089 Compare January 26, 2024 07:23
@gabrielschulhof gabrielschulhof force-pushed the restore-ability-to-run-with-NAPI_EXPERIMENTAL branch 2 times, most recently from 2c5853a to f89a09b Compare February 18, 2024 18:53
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented May 3, 2024

Core status of needed changes:

branch backport complete released
v22.x ✅ (v22.0.0)
v20.x ✅ (20.13.0)
v18.x ✅ (v18.20.3)

@KevinEady
Copy link
Contributor

Hi @gabrielschulhof ,

Looks like this is becoming of more importance. I can take a look at continuing this work? Judging by the latest CI failure, looks like the Napi::External<> object needs to implement a similar approach.

@gabrielschulhof gabrielschulhof force-pushed the restore-ability-to-run-with-NAPI_EXPERIMENTAL branch from f89a09b to a6db742 Compare May 17, 2024 14:30
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Project coverage is 64.89%. Comparing base (57ba3df) to head (1a11e3a).
Report is 1 commits behind head on main.

Files Patch % Lines
napi-inl.h 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
+ Coverage   64.71%   64.89%   +0.17%     
==========================================
  Files           3        3              
  Lines        1981     1991      +10     
  Branches      687      687              
==========================================
+ Hits         1282     1292      +10     
  Misses        138      138              
  Partials      561      561              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gabrielschulhof gabrielschulhof force-pushed the restore-ability-to-run-with-NAPI_EXPERIMENTAL branch 2 times, most recently from 390f607 to 08ce972 Compare May 25, 2024 03:07
@gabrielschulhof
Copy link
Contributor Author

v18.x fails because v18.20.3, which is the one where the thread-safe function revert-to-napi_env landed, is not yet available via github actions.

@gabrielschulhof
Copy link
Contributor Author

Removed v21.x from the matrix because the needed fixes were never backported to it and because it will be discontinued soon, per https://github.com/nodejs/Release/raw/main/schedule.svg

Since we made the default for Node.js core finalizers synchronous for
users running with `NAPI_EXPERIMENTAL` and introduced
`env->CheckGCAccess()` in Node.js core, we must now defer all
finalizers in node-addon-api, because our users likely make non-gc-safe
Node-API calls from existing finalizers. To that end,
  * Use the NAPI_VERSION environment variable to detect whether
    `NAPI_EXPERIMENTAL` should be on, and add it to the defines if
    `NAPI_VERSION` is set to `NAPI_VERSION_EXPERIMENTAL`, i.e.
    2147483647.
  * When building with `NAPI_EXPERIMENTAL`,
    * render all finalizers asynchronous, and
    * expect `napi_cannot_run_js` instead of `napi_exception_pending`.
@gabrielschulhof gabrielschulhof force-pushed the restore-ability-to-run-with-NAPI_EXPERIMENTAL branch from 7d97d9c to 84b5f8e Compare May 29, 2024 00:09
Copy link
Contributor

@KevinEady KevinEady left a comment

Choose a reason for hiding this comment

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

See comment for discussion.

@@ -5,6 +5,7 @@
},
'conditions': [
['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ],
['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in test/addon_build/tpl/binding.gyp as well? Discovered this when working on my nogc-types implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KevinEady propagated this to the add-on build test as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants