-
Notifications
You must be signed in to change notification settings - Fork 465
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
src: restore ability to run under NAPI_EXPERIMENTAL #1409
Conversation
I might have been thinking of another change but I thought we had made it opt-in with another define? @legendecas is that right? |
I'm afraid that I didn't understand this assumption. Is this assumption unique to node-addon-api, not the node-api c layer? |
8f78a76
to
8b19089
Compare
2c5853a
to
f89a09b
Compare
Core status of needed changes:
|
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 |
f89a09b
to
a6db742
Compare
Codecov ReportAttention: Patch coverage is
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. |
390f607
to
08ce972
Compare
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. |
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`.
7d97d9c
to
84b5f8e
Compare
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.
See comment for discussion.
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.
See comments.
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.
LGTM!
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.
LGTM
@vmoroz noticed on my PR that i am not testing experimental on Windows, which is also the case on this PR. Should Windows experimental also be tested? ref: #1514 (comment) |
@KevinEady let's land this and add experimental on Windows in a different PR. I filed an issue so we don't forget. |
Based on discussion in the team meeting today, removed the SemVer-major lable as we believe it should be patch. |
Since we made the default for Node.js core finalizers synchronous for users running with
NAPI_EXPERIMENTAL
and introducedenv->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,NAPI_EXPERIMENTAL
should be on, and add it to the defines ifNAPI_VERSION
is set toNAPI_VERSION_EXPERIMENTAL
, i.e. 2147483647.NAPI_EXPERIMENTAL
,napi_cannot_run_js
instead ofnapi_exception_pending
.BEGIN_COMMIT_OVERRIDE
fix: restore ability to run under NAPI_EXPERIMENTAL (#1409)
END_COMMIT_OVERRIDE