node icon indicating copy to clipboard operation
node copied to clipboard

node-api: add new napi_ref type for any napi_value

Open vmoroz opened this issue 2 years ago • 8 comments

The issue

In the Node-API the napi_value only exists on the call stack. When the value needs to be persisted after the call stack unwinded, we can use the napi_ref. But the napi_ref can keep only napi_value only of napi_object and napi_function types because napi_ref must support weak pointer semantic. Thus, today it is not possible to keep persistent values of other types such as napi_string or napi_number.

Solution

In this PR we extend the napi_ref to support two types of references:

  • node_api_reftype_strong - the new reference type that can keep values of any type as strong ref-counted references. When the ref count goes down to zero, a node_api_reftype_strong reference is deleted.
  • node_api_reftype_maybe_weak - the current references that can store only napi_object and napi_function values and can be strong or weak references. When the reference count goes down to zero the reference becomes 'weak' and can be collected by GC. Developers must use explicit napi_delete_reference call to delete the reference unless the reference is created internally without returning napi_ref to the user code.

To support the new napi_ref sematic two new functions are added:

  • node_api_create_reference - a replacement for napi_create_reference that accepts a new parameter reftype.
  • node_api_get_reference_type - to get type of napi_ref.

New test_strong test is added to the test/js-native-api folder. The test stores napi_value of different types and then retrieves them.

The n-api.md documentation is updated to reflect that napi_ref can be of two different types.

vmoroz avatar Apr 01 '22 04:04 vmoroz

Review requested:

  • [ ] @nodejs/gyp
  • [ ] @nodejs/node-api

nodejs-github-bot avatar Apr 01 '22 04:04 nodejs-github-bot

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.

vmoroz avatar Apr 01 '22 17:04 vmoroz

Adding "blocked" label as https://github.com/nodejs/node/pull/42557#issuecomment-1086156130 said to prevent unexpected landing. Feel free to remove the label when it is not the case anymore.

legendecas avatar Apr 06 '22 07:04 legendecas

Hi @nodejs/node-api , can we get a new review after @vmoroz 's implementation changes that address the blocked issue re. https://github.com/nodejs/node/pull/42557#issuecomment-1086156130 ? Thanks

KevinEady avatar Apr 22 '22 15:04 KevinEady

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!

KevinEady avatar May 20 '22 15:05 KevinEady

@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.

vmoroz avatar May 27 '22 17:05 vmoroz

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?

mhdawson avatar Jun 27 '22 20:06 mhdawson

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.

legendecas avatar Aug 09 '22 16:08 legendecas

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.

vmoroz avatar Sep 02 '22 14:09 vmoroz

Some thoughts on naming. Maybe these would work?

  • node_api_reftype_any
  • node_api_reftype_object

@mhdawson, I have changed the names.

vmoroz avatar Sep 02 '22 14:09 vmoroz

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.

legendecas avatar Sep 05 '22 04:09 legendecas

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.

KevinEady avatar Sep 30 '22 15:09 KevinEady

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

vmoroz avatar Oct 10 '22 19:10 vmoroz

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.

vmoroz avatar May 07 '23 20:05 vmoroz