node-api: enable napi_ref for all value types by vmoroz · Pull Request #42557 · nodejs/node · GitHub

The issue

In the Node-API the napi_value exists only on the call stack. When the value needs to be persisted after the call stack is unwinded, we can use the napi_ref. Currently the napi_ref keeps napi_value only for napi_object, napi_function, napi_external, and napi_symbol types because napi_ref must support weak pointer semantic. Thus, there was a decision to not keep persistent values for other types such as napi_string or napi_number. Though, the napi_symbol cannot have the weak semantic, but we still can have a napi_ref for it.

Solution

In this PR we enable use of napi_ref for strong references to all napi_value types. Though, only references for napi_object, napi_external, and napi_function types could have the true weak reference semantic. These three types could be collected by GC when the ref count is 0, while other types cannot be collected because they are always string references. To keep the backward compatibility, this PR enables the new behavior under a special node_api_features bit flag node_api_feature_reference_all_types. The node_api_features are being introduced in this PR.

The new node_api_features allow changing internal behavior of existing Node-API functions.

We pass a node_api_features pointer to the napi_module struct in the NAPI_MODULE_X macro. This macro is used for the module registration. If the module is initialized without using this macro, then there will be no features selected and the module will use the node_api_feature_none.

Each Node-API version is going to define its own default set of features. For the current version it can be accessed using node_api_default_features. A module can override the set of its enabled features by adding NODE_API_CUSTOM_FEATURES definition to the .gyp file and then setting the value of the global node_api_module_features variable. To check enabled features, use the node_api_is_feature_enabled function.

For example, to disable node_api_feature_reference_all_types we can exclude its bit from the node_api_default_features:

node_api_features node_api_module_features =
    node_api_default_features & ~node_api_feature_reference_all_types;

Documentation

The n-api.md documentation is updated with the info about node_api_features enum and the node_api_is_feature_enabled function.

Testing

Added three new tests:

js-native-api/test_reference_all_types.
The test stores napi_value of different types and then retrieves them for the strong and weak napi_ref references.
node-api/test_init_reference_all_types. The same as the previous test, but it uses NAPI_MODULE_INIT macro to initialize module instead of NAPI_MODULE. This is to verify that both NAPI_MODULE and NAPI_MODULE_INIT macro support feature specification.
node-api/test_init_reference_obj_only. To test old references behavior and module initialization with NAPI_MODULE_INIT macro.

The test_reference and test_init_reference_obj_only disable the node_api_feature_reference_all_types feature and make sure that the old napi_ref behavior continues to work.

The NAPI_EXPERIMENTAL is added to common.h and entry_point.c in test/js-native-api folder to make sure that the js-native-api tests always use the latest Node-API version features.

Review requested:

@nodejs/gyp
@nodejs/node-api

This PR was discussed today in the Node-API meeting and the suggestion was to reuse the existing napi_ref instead of adding the new napi_persistent type. I am going to add new versions of functions related to napi_ref that will have a flag that will control behavior of napi_ref depending on whether it supports the weak ref semantic:

With the weak ref semantic it will continue to work as today.
Without the weak ref semantic it will behave as ref counted Persistent value that can keep value of any type.

The new functions are going to have the same name as the current function, but to have prefix node_api_ instead of napi_, and to have a new flags parameter.

Adding "blocked" label as #42557 (comment) said to prevent unexpected landing. Feel free to remove the label when it is not the case anymore.

Hi @nodejs/node-api , can we get a new review after @vmoroz 's implementation changes that address the blocked issue re. #42557 (comment) ? Thanks

Doc should indicated as experimental as well.

We discussed in the 20 May Node-API meeting that @vmoroz would like to change the enum names for node_api_reftype_strong_or_weak and node_api_reftype_strong. If anyone has some suggestions, please feel free to comment on the PR. Thanks!

@mhdawson, @legendecas, @NickNaso, per our discussion today at the Node-API meeting, I had updated the PR description and added a note to the doc that node_api_create_reference is targeting to replace the napi_create_reference. The PR is ready to be reviewed and merged.

Some thoughts on naming. Maybe these would work?

node_api_reftype_any
node_api_reftype_object

Where we can explain the difference being that

for node_api_reftype_any, they can point to any type of node_api value, however, the application must fully manage the creation/deletion through the ref count and that when the ref count hits 0 the reference will be deleted.
for node_api_reftype_object, they can only point to node_api values for and object or related like functions and in addition to being able to fully manage the creation/deletion there is the additional option of allowing the gc to clean up the object and it's related reference when the reference is made weak by lowering the reference count to 0.

In addition to the naming are we sure that all JavaScript engines will support making persistent references to all types or have what's needed for the node-api implementation for the engine to be able implement something that looks like a reference?

AFAICT, the main blocker for supporting arbitrary values in napi_create_reference is the concern on various engine supports for the feature. I take a superficial dig into engines on the edge like JerryScript and found that they all support creating references on primitive values (with reference counting): https://jerryscript.net/api-reference/#jerry_acquire_value, and QuickJS JSValue https://bellard.org/quickjs/quickjs.html.

However, node-api implementation of napi_create_reference in the iotjs, a Node.js implementation based on JerryScript, is not supporting weak references: https://github.com/jerryscript-project/iotjs/blob/master/src/napi/node_api_lifetime.c#L210

As the weak reference is not guaranteed to be consistent, as those values can be collected anytime, I believe the support of create strong references on primitive values can meet the criteria New API should be agnostic towards the underlying JavaScript VM and "A new API addition should be simultaneously implemented in at least one other VM implementation of Node.js".

I believe it would be beneficial to strive to extend the maximum capability of existing APIs and try not to introduce new APIs for differences that are hard for end-users to choose between.

I believe it would be beneficial to strive to extend the maximum capability of existing APIs and try not to introduce new APIs for differences that are hard for end-users to choose between.

@legendecas, do you think that instead of adding new API we should just change the implementation of the existing references to support all value types and just say in the docs that the weak references work only object-like values?
I would expect that all JS engines must support weak reference semantics for objects to implement JS WeakRef.

Some thoughts on naming. Maybe these would work?

node_api_reftype_any

node_api_reftype_object

@mhdawson, I have changed the names.

do you think that instead of adding new API we should just change the implementation of the existing references to support all value types and just say in the docs that the weak references work only object-like values?

Yes, we should try to extend the current API. For documentation, we should not distinguish types but instead say that the behavior of the weak reference is determined by the engine implementation, and is not guaranteed to be able to get the referenced value when the ref-count is reached 0.

Hi @nodejs/node-api ,

This PR introduces two changes: (1) allowing references to all values and (2) the ability for modules to request certain feature sets at module registration. @vmoroz has requested some feedback on both of these points if possible.

@mhdawson, @legendecas, I have updated the PR - please review.
It has the napi_ref updates, napi_features implementation, new unit tests, and updated documentation.

This can be a good start for node-api to get started with backward-compatible feature additions!

As you mentioned in the node-api meetings, this bit flag (an enum is an int size) may only represent 32 features at most. I'm wondering if it would be more extensible to save the module's defined NAPI_VERSION as nm_napi_version here like nm_version instead. In this way, node can refuse to load a node-api addon when an addon requires NAPI_VERSION 10, but the node is compiled as NAPI_VERSION 9.

The drawback of the alternative is that people have to pick up all feature changes with the new NAPI_VERSION, not part of it. But this could also be a relief that we don't need to maintain a long list of features, but a version support list instead.

What do you think?

I like the idea of passing Node-API version used for a module. This way we can enforce the version compatibility.
The only drawback could be that developers will not be able to choose functionality depending on the Node-API version at runtime. I am not sure if anyone does it though.

As for the 32-bit feature set limit, the proposal is to pass not the feature bits to the module, but rather the pointer to the feature set. This way we can use the first 31 bits normally. In case if we need more, then we can set the 32nd bit and it will mean that the feature set has the second 32bit number, and the feature set pointer becomes a pointer to the feature set array with two elements. We can extend this array to be as long as needed. Obviously, each new entry in this array will require its own enum type. In practice I doubt that we ever exceed the 31 bit limit, but if we do, then we can extend it using this approach.

We never allowed creating a reference on primitive values (except Symbols). So I believe there is no need to check the feature bit here: reference on primitive values can not be a weak reference.

Suggested change

      
                  _canBeWeak(!env->IsFeatureEnabled(napi_feature_reference_all_types) ||
          
                  _canBeWeak(

The intent here is to stop offering weak references for Symbols with the new flag and only do it for Objects and Functions to better match the JavaScript spec. But since currently we support weak references for Symbols and have unit tests in napi_references for them, I had to put the flag check here. This way the old code works without changes if the flag is not set.

I removed the _canBeWeak field completely to let V8 engine to decide on the weak references' behavior.
The only true weak references, as it used to be before, are object, external objects, and functions.
All other types are strong references when the ref count is 0.
This behavior can be seen in the new tests.
The only difference between what we used to have before and now is that we allow all types instead of only four (object, external object, function, and symbol) to be ref counted.

It almost feels like that allowing use of napi_ref for all value types may not need a special feature, but we should rather just extend the existing behavior as we did previously for Symbols.

For compatiblity I'm wondering if we should tie the defaults to a Node-API version versus the vesion of Node.js Unless the module author choses to opt-in, they behaviour should not change unless they have moved up to a newer Node-API vesion?

Looking more closely I think that is the approach that is described, but but nothing is guarded because its under experimental. Will make a comment in src/js_native_api_types.h

My intent is to make it per Node-API version.

I'm not sure if we should have this, I think we should guard the availabilty to be able to turn on experimental features with NAPI_EXPERIMENTAL, but I'm not sure if having a set different than the default for the Node-API version makes sense. Otherwise somebody who wants to use any experimental API is also forced into to new defaults.

In case if someone does not want to use the new experimental default, they can always override it per module.

Suggested change

      
            * `[in] feature`: The feature that we want to test.
          
            * `[in] feature`: The features that we want to test.

Changed. Though I am not sure why we need to use plural form, while the parameter is singular. Should I also rename the parameter?

I wonder if this should be in node_api.cc instead of this file. It is not tied to v8 right?

It is not tied to V8. I was thinking that we want to have features available for different JS engines.

Comments as before, I think this will end up setting to be specific to the Node.js version used to build versus the Node-API version selected. Should discuss this more.

I am not sure how to address this. Let's discuss it in the Node-API meeting. The concern as I understand it is that if we add more experimental features, we may fail to propagate them to the previous versions of Node.JS, and thus it may cause differences in the behavior between Node.JS versions, while we have the same version of Node-API, right?

Should we just create a new NAPI_MODULE_X_FEATURES which takes the features parameter?

It may be simpler - let me try.

Closing this PR because PR #45715 that was recently merged implements an alternative approach for changing existing behavior. It uses Node-API version instead of features. The PR #45715 also implements support for all value types supported by napi_ref.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 1, 2022

legendecas added the node-api Issues and PRs related to the Node-API. label Apr 1, 2022

jasnell approved these changes Apr 3, 2022

View reviewed changes

legendecas added the blocked PRs that are blocked by other issues or PRs. label Apr 6, 2022

vmoroz changed the title node-api: add napi_persistent to keep value of any type node-api: allow napi_ref to keep value of any type Apr 6, 2022

vmoroz force-pushed the PR/Add_napi_permanent branch from 516e36e to efd23fd Compare April 26, 2022 01:49

vmoroz changed the title node-api: allow napi_ref to keep value of any type node-api: add new napi_ref type for any napi_value Apr 26, 2022

mhdawson reviewed Apr 29, 2022

View reviewed changes

vmoroz force-pushed the PR/Add_napi_permanent branch from 3fdb722 to 4f8cbed Compare May 27, 2022 05:07

legendecas removed the blocked PRs that are blocked by other issues or PRs. label May 28, 2022

vmoroz force-pushed the PR/Add_napi_permanent branch from f43b537 to b320716 Compare June 3, 2022 14:00

vmoroz force-pushed the PR/Add_napi_permanent branch from ba396aa to bffedbb Compare September 2, 2022 14:56

vmoroz force-pushed the PR/Add_napi_permanent branch from bffedbb to b39f201 Compare September 9, 2022 14:51

vmoroz marked this pull request as draft September 9, 2022 14:52

vmoroz force-pushed the PR/Add_napi_permanent branch from b39f201 to 70a98de Compare September 30, 2022 14:56

KevinEady mentioned this pull request Oct 7, 2022

node-api: run finalizers directly from GC #42651

Closed

vmoroz force-pushed the PR/Add_napi_permanent branch from 70a98de to 6b1f36d Compare October 10, 2022 19:00

vmoroz changed the title node-api: add new napi_ref type for any napi_value node-api: enable napi_ref for all value types Oct 10, 2022

vmoroz marked this pull request as ready for review October 10, 2022 19:33

legendecas reviewed Oct 14, 2022

View reviewed changes

legendecas mentioned this pull request Oct 17, 2022

ThreadSafeFunction should handle exceptions in async callbacks identical to Node itself nodejs/node-addon-api#1119

Closed

vmoroz added 3 commits October 28, 2022 07:56

node-api: enable napi_ref for all value types

579d44c

attempt to fix lint issues

e108b5e

add two reference tests that use NAPI_MODULE_INIT

e629646

vmoroz force-pushed the PR/Add_napi_permanent branch from f8bbe4c to e629646 Compare October 28, 2022 14:58

remove _canBeWeak and change names

246e224

mhdawson reviewed Nov 3, 2022

View reviewed changes

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

mhdawson reviewed Nov 3, 2022

View reviewed changes

update spelling in docs based based on code review

c794f31

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

legendecas mentioned this pull request Nov 28, 2022

Node-API Runtime Semantics Versioning #45657

Closed

vmoroz mentioned this pull request Jan 12, 2023

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

Closed

vmoroz mentioned this pull request May 1, 2023

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

Closed

vmoroz closed this May 7, 2023

vmoroz deleted the PR/Add_napi_permanent branch May 7, 2023 20:09

+                // Not only objects, external objects, functions, and symbols as before.
+                node_api_feature_reference_all_types = 1 << 0,
+                // Each version of Node-API is going to have its own default set of features.
+                node_api_default_experimental_features = node_api_feature_reference_all_types,

	_canBeWeak(!env->IsFeatureEnabled(napi_feature_reference_all_types) \|\|
	_canBeWeak(

+                node_api_default_experimental_features = node_api_feature_reference_all_types,
+                 // version specific
+                node_api_default_features = node_api_default_experimental_features,

	* `[in] feature`: The feature that we want to test.
	* `[in] feature`: The features that we want to test.

                 return napi_clear_last_error(env);
               }
+              napi_status NAPI_CDECL node_api_is_feature_enabled(napi_env env,

+              #else  // NODE_API_CUSTOM_FEATURES
+              #define NODE_API_DEFINE_DEFAULT_FEATURES                                       \
+                static node_api_features node_api_module_features = node_api_default_features;

+              #define NODE_API_FEATURES_PTR
+              #endif  // NAPI_EXPERIMENTAL
               #define NAPI_MODULE_X(modname, regfunc, priv, flags)                           \

node-api: enable napi_ref for all value types by vmoroz · Pull Request #42557 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

node-api: enable napi_ref for all value types#42557vmoroz wants to merge 5 commits intonodejs:mainnodejs/node:mainfrom vmoroz:PR/Add_napi_permanentvmoroz/nodejs-node:PR/Add_napi_permanentCopy head branch name to clipboard

Conversation

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

Uh oh!

The issue

Solution

Documentation

Testing

Uh oh!

nodejs-github-bot commented Apr 1, 2022

Uh oh!

vmoroz commented Apr 1, 2022

Uh oh!

legendecas commented Apr 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinEady commented Apr 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KevinEady commented May 20, 2022

Uh oh!

vmoroz commented May 27, 2022

Uh oh!

mhdawson commented Jun 27, 2022

Uh oh!

legendecas commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmoroz commented Sep 2, 2022

Uh oh!

vmoroz commented Sep 2, 2022

Uh oh!

legendecas commented Sep 5, 2022

Uh oh!

KevinEady commented Sep 30, 2022

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

node-api: enable napi_ref for all value types#42557
vmoroz wants to merge 5 commits into
nodejs:mainfrom
vmoroz:PR/Add_napi_permanent

vmoroz commented Apr 1, 2022 •
edited
Loading

legendecas commented Apr 6, 2022 •
edited
Loading

legendecas commented Aug 9, 2022 •
edited
Loading

vmoroz commented Oct 10, 2022 •
edited
Loading