node-api: get Node API version used by addon by vmoroz · Pull Request #45715 · nodejs/node · GitHub

This is to address the issue #45657.

The main goal of the PR is to establish a new mechanism to implement Node-API changes that affect not only the API shape, but also its behavior.
We are going to associate Node-API functions and new Node-API behavior with new Node-API version.
Node-API modules can choose which version of Node-API they target.
By default, they target the current Node-API version 8.
To target newer versions of Node-API they must explicitly opt-in by defining napi_module_get_api_version_v1 export function that returns targeted version.

In this PR, the NAPI_MODULE and NAPI_MODULE_INITIALIZER macros are changed to define the napi_module_get_api_version_v1 function that reports the value of the NAPI_VERSION macro. C++ developers can control the targeted Node-API version by defining the NAPI_VERSION macro as they did before. The key difference is that now this macro not only affects the set of functions that module uses but also affects the Node-API implementation behavior.

Node.js is going to validate the version requested by the addon.

If the version is below version 8, then it will be internally switched to version 8 because we did not implement the behavior change before version 8.
If the version is above the NAPI_VERSION the Node.js compiled for and it is not NAPI_VERSION_EXPERIMENTAL, then Node.js will throw an error on module initialization.

Note that modules targeting NAPI_VERSION_EXPERIMENTAL are compatible with any Node-API version the Node.js was compiled with. Node-API in that case will use the latest functions and behaviors. Such module should call napi_get_version function to get the latest stable Node-API version offered by current Node.js instance. Then it must adapt for the different behavior based on the version.

In this PR, we change the behavior of the napi_create_reference function for Node-API versions higher than 8. The new behavior allows the creation of an napi_ref for any napi_valuetype instead of only napi_object, napi_function, napi_external, and napi_symbol. One of the main goals is to enable the use of napi_ref for napi_string.

The napi_ref for the new primitive types immediately releases them when the ref count becomes 0 because they cannot be weak references. For the previously supported object and symbol types, the behavior did not change: these values become weak references and can be collected at any time. Note that the globally registered symbols created with the Symbol.for() function call and well-known symbols such as Symbol.iterator are always kept alive because they are never collected by the garbage collector.

Review requested:

@nodejs/node-api

We discussed this draft's approach in the 9 Dec 2022 Node-API meeting. One question that was brought up is discussion about using two functions for the register-by-symbol method (one for getting version and one for initializing the module) and if this is a good approach versus just using a single method. It was determined that using two methods would be more beneficial because we need to know the targeted runtime version before the module initialization code runs, since this code can run arbitrary JavaScript.

Thank you for working on this! I know this is still a draft but I'll continue reviewing.

Mostly LGTM. Can we combine the two tests into a single test that uses two addons? So, like,

{
  "targets": [
    {
      "target_name": "test_reference_all_types",
      "sources": [ "test_reference_by_node_api_version.c" ],
      "defines": [ "NAPI_EXPERIMENTAL" ],
    },
    {
      "target_name": "test_reference_obj_only",
      "sources": [ "test_reference_by_node_api_version.c" ],
      "defines": [ "NAPI_VERSION=8" ],
    }
  ]
}

... and then do some #ifdef-ing in the source file? The test JS can then require() each add-on and perform a fixed set of tests with two different sets of expectations as passed-in parameters.

Mostly LGTM. Can we combine the two tests into a single test that uses two addons? So, like,

Let me see if it can be done.

Mostly LGTM. Can we combine the two tests into a single test that uses two addons? So, like,

Let me see if it can be done.

It may also be possible to distill common bits into one source file which is listed in the sources section of both targets and then have two source files, each of which has the version-specific changes, one for each target. That should reduce the need for #ifdefs.

Mostly LGTM. Can we combine the two tests into a single test that uses two addons? So, like,

Let me see if it can be done.

It may also be possible to distill common bits into one source file which is listed in the sources section of both targets and then have two source files, each of which has the version-specific changes, one for each target. That should reduce the need for #ifdefs.

I combined both tests and found that the C code between them was exactly the same. So, there was no need for #ifdefs. I also noticed that the JS code was mostly the same and I combined them too with minimal changes. Now the test looks pretty cool because it shows how we can create two different addons from the same C code that have different behavior depending on the targeted Node-API version. Then we use these addons in the same JS code.

@legendecas , @mhdawson , the PR is ready to be merged. Could you please take a look at it?

I got partway through a review. Will look further.

This does match current behaviour but also means that we'll have a leak right? Since we can change behaviour in a new Node-API version without affecting existing applications maybe the right thing to do is to consider all symbols as not being week so that the leak does not occur?

@mhdawson, there are no memory leaks. The only side effect is that for the global and well-known symbols we always return a valid symbol because they are global and never collected by GC (it is by design). The local symbols are behaving the same way as weak references to objects.

We say that when the napi_ref ref count goes to zero we are converting values to weak references if they are supported, otherwise we release them. Symbols support weak references, but they do it partially: for some of them the weak reference will always return "live" value because GC never collects them.

There is also a difference in the JavaScript WeakRef behavior. It throws an exception if a global symbol is given to it but succeeds for local and well-known symbols. We can align with this behavior in future when V8 API lets us to see the symbol type. Though we should probably avoid throwing an exception and thus resetting the global symbols instead of keeping them could be not that big of a deal.

In any case to avoid native memory leaks we require to call napi_delete_reference. Changing the ref count was never controlling the memory lifetime as we see it in COM IUnknown or in std::shared_ptr. If the napi_delete_reference is not called explicitly, then the native memory will be removed on napi_env finalization. So, technically it is also not a memory leak, but rather delays the memory release.

@vmoroz thanks for the clarification. I was thinking about the auto-delete we have in RefBase but looking more closely I see that won't be related to direct use of references.

@vmoroz just one question, otherwise all looks good to me. Will wait to add approval until we agree on the answer to that question.

LGTM

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

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

Sorry for going in circles about the PR. Thank you for pushing this forward!

PR-URL: #45715 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

@vmoroz, thanks for all of the hard on this, landed in c542d3a

PR-URL: #45715 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#45715 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

vmoroz marked this pull request as draft December 2, 2022 16:22

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 2, 2022

vmoroz changed the title node-api: get Node API version used by addon node-api: (DRAFT) get Node API version used by addon Dec 3, 2022

vmoroz force-pushed the PR/NodeApiModuleVersion branch from 87acd9b to 09b349b Compare December 16, 2022 15:58

vmoroz marked this pull request as ready for review December 16, 2022 16:02

legendecas added node-api Issues and PRs related to the Node-API. node-api-semver-major semver-major changes that need to be considered for N-API labels Jan 6, 2023

legendecas reviewed Jan 6, 2023

View reviewed changes

Comment thread src/node_api.cc Outdated

Comment thread src/node_api.h Outdated

vmoroz mentioned this pull request Jan 12, 2023

Expose a faster way to create objects with properties for Node-API #45905

Closed

vmoroz force-pushed the PR/NodeApiModuleVersion branch from f5805cf to 21c61e8 Compare January 20, 2023 14:35

vmoroz mentioned this pull request Jan 23, 2023

node-api: deprecate napi_module_register based registration #46319

Closed

vmoroz force-pushed the PR/NodeApiModuleVersion branch from b327ee2 to 7273e06 Compare February 3, 2023 05:46

vmoroz force-pushed the PR/NodeApiModuleVersion branch from 7273e06 to 722dbc5 Compare April 7, 2023 14:40

vmoroz changed the title node-api: (DRAFT) get Node API version used by addon node-api: get Node API version used by addon Apr 7, 2023

gabrielschulhof reviewed Apr 7, 2023

View reviewed changes

Comment thread src/node_api.h Outdated

This was referenced Apr 8, 2023

Node-API v9 nodejs/abi-stable-node#436

Closed

node-api: Add API to check safety of calling JS #43805

Closed

vmoroz force-pushed the PR/NodeApiModuleVersion branch from 830a9c0 to a90d822 Compare April 14, 2023 14:00

gabrielschulhof approved these changes Apr 14, 2023

View reviewed changes

Comment thread doc/api/n-api.md Outdated

Comment thread src/node_binding.cc Outdated

Comment thread test/node-api/test_reference_by_node_api_version/test.js Outdated

gabrielschulhof mentioned this pull request Apr 14, 2023

src: handle failure during error wrap of primitive nodejs/node-addon-api#1310

Closed

vmoroz force-pushed the PR/NodeApiModuleVersion branch from a90d822 to 0cd1b0d Compare April 18, 2023 15:05

github-actions Bot mentioned this pull request May 3, 2023

CI Reliability 2023-05-03 nodejs/reliability#554

Open

33 tasks

gabrielschulhof reviewed May 3, 2023

View reviewed changes

Comment thread doc/api/n-api.md Outdated

Comment thread doc/api/n-api.md Outdated

gabrielschulhof reviewed May 3, 2023

View reviewed changes

Comment thread test/node-api/test_reference_by_node_api_version/test.js

avoid using pronouns in the docs

38d5f0f

github-actions Bot mentioned this pull request May 4, 2023

CI Reliability 2023-05-04 nodejs/reliability#555

Open

28 tasks

mhdawson reviewed May 4, 2023

View reviewed changes

mhdawson approved these changes May 4, 2023

View reviewed changes

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2023

legendecas approved these changes May 5, 2023

View reviewed changes

mhdawson pushed a commit that referenced this pull request May 5, 2023

node-api: get Node API version used by addon

c542d3a

PR-URL: #45715 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

mhdawson closed this May 5, 2023

gabrielschulhof mentioned this pull request May 5, 2023

Create a Node-API release checklist #47664

Closed

legendecas mentioned this pull request May 5, 2023

Node-API Runtime Semantics Versioning #45657

Closed

vmoroz deleted the PR/NodeApiModuleVersion branch May 5, 2023 22:14

toyobayashi mentioned this pull request May 6, 2023

get Node API version used by addon toyobayashi/emnapi#64

Merged

vmoroz mentioned this pull request May 7, 2023

node-api: enable napi_ref for all value types #42557

Closed

legendecas mentioned this pull request May 12, 2023

doc: document node-api version process #47972

Merged

targos pushed a commit that referenced this pull request May 12, 2023

node-api: get Node API version used by addon

f489c67

PR-URL: #45715 Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

vmoroz mentioned this pull request May 12, 2023

node-api: make napi_ref for all value types to be experimental #47975

Closed

targos mentioned this pull request May 15, 2023

v20.2.0 release proposal #48020

Merged

gabrielschulhof mentioned this pull request Jun 5, 2023

Allow napi_add_finalizer to work on primitive types #48311

Closed

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

+              // method cannot be weak references because they are never collected.
+              //
+              // Currently, V8 has no API to detect if a symbol is local or global.
+              // Until we have a V8 API for it, we consider that all symbols can be weak.

node-api: get Node API version used by addon by vmoroz · Pull Request #45715 · nodejs/node · GitHub

Uh oh!

Uh oh!

node-api: get Node API version used by addon - #45715

node-api: get Node API version used by addon#45715vmoroz wants to merge 15 commits intonodejs:mainnodejs/node:mainfrom vmoroz:PR/NodeApiModuleVersionvmoroz/nodejs-node:PR/NodeApiModuleVersionCopy head branch name to clipboard

Conversation

vmoroz commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 2, 2022

Uh oh!

KevinEady commented Dec 9, 2022

Uh oh!

Uh oh!

Uh oh!

legendecas commented Jan 6, 2023

Uh oh!

gabrielschulhof left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vmoroz commented Apr 7, 2023

Uh oh!

gabrielschulhof commented Apr 7, 2023

Uh oh!

vmoroz commented Apr 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vmoroz commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhdawson May 4, 2023

Choose a reason for hiding this comment

Uh oh!

vmoroz May 4, 2023

Choose a reason for hiding this comment

Uh oh!

mhdawson May 4, 2023

Choose a reason for hiding this comment

Uh oh!

mhdawson commented May 4, 2023

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 4, 2023

Uh oh!

nodejs-github-bot commented May 4, 2023

Uh oh!

legendecas commented May 5, 2023

Uh oh!

mhdawson commented May 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

node-api: get Node API version used by addon#45715
vmoroz wants to merge 15 commits into
nodejs:mainfrom
vmoroz:PR/NodeApiModuleVersion

vmoroz commented Dec 2, 2022 •
edited
Loading

gabrielschulhof left a comment •
edited
Loading

vmoroz commented Apr 20, 2023 •
edited
Loading