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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2046,6 +2046,49 @@ the `napi_value` in question is of the JavaScript type expected by the API.

### Enum types

#### `node_api_features`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

```c
typedef enum {
node_api_feature_none = 0,
node_api_feature_reference_all_types = 1 << 0,

node_api_default_experimental_features = node_api_feature_reference_all_types,

// version specific
node_api_default_features = node_api_default_experimental_features,
Comment on lines +2062 to +2065
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

} node_api_features;
```

The `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 defines 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 defining 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 disables `node_api_feature_reference_all_types` feature we can
vmoroz marked this conversation as resolved.
Show resolved Hide resolved
exclude its bit from the `node_api_default_features` set:

```c
node_api_features node_api_module_features =
node_api_default_features & ~node_api_feature_reference_all_types;
```

#### `napi_key_collection_mode`

<!-- YAML
Expand Down Expand Up @@ -5700,6 +5743,32 @@ support it:
* If the function is not available, provide an alternate implementation
that does not use the function.

#### `node_api_is_feature_enabled`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

```c
NAPI_EXTERN napi_status node_api_is_feature_enabled(napi_env env,
node_api_features feature,
bool* result);
```

* `[in] env`: The environment that the API is invoked under.
* `[in] feature`: The feature that we want to test.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

* `[out] result`: Whether the feature or a set of features are enabled.

Returns `napi_ok` if the API succeeded.

The function checks enabled features for the module.
If `feature` parameter has multiple `node_api_features` bit flags, then the
function returns `true` only when all the requested fatures are enabled.

See `node_api_features` for more details about Node-API features.

## Memory management

### `napi_adjust_external_memory`
Expand Down
5 changes: 5 additions & 0 deletions src/js_native_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,11 @@ NAPI_EXTERN napi_status NAPI_CDECL napi_object_seal(napi_env env,
napi_value object);
#endif // NAPI_VERSION >= 8

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN napi_status NAPI_CDECL node_api_is_feature_enabled(
napi_env env, node_api_features feature, bool* result);
#endif // NAPI_EXPERIMENTAL

EXTERN_C_END

#endif // SRC_JS_NATIVE_API_H_
31 changes: 31 additions & 0 deletions src/js_native_api_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,37 @@ typedef enum {
// * the definition of `napi_status` in doc/api/n-api.md to reflect the newly
// added value(s).

#ifdef NAPI_EXPERIMENTAL
// 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 defines 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 defining 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 disables node_api_feature_reference_all_types:
// node_api_features node_api_module_features =
// node_api_default_features & ~node_api_feature_reference_all_types;
typedef enum {
// To be used when no features needs to be set.
node_api_feature_none = 0,
// Use napi_ref for all value types.
// 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,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// This variable must be conditionally set depending on the Node-API version.
node_api_default_features = node_api_default_experimental_features,
} node_api_features;
#endif

typedef napi_value(NAPI_CDECL* napi_callback)(napi_env env,
napi_callback_info info);
typedef void(NAPI_CDECL* napi_finalize)(napi_env env,
Expand Down
17 changes: 14 additions & 3 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2495,9 +2495,11 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env,
CHECK_ARG(env, result);

v8::Local<v8::Value> v8_value = v8impl::V8LocalValueFromJsValue(value);
if (!(v8_value->IsObject() || v8_value->IsFunction() ||
v8_value->IsSymbol())) {
return napi_set_last_error(env, napi_invalid_arg);
if (!env->IsFeatureEnabled(node_api_feature_reference_all_types)) {
if (!(v8_value->IsObject() || v8_value->IsFunction() ||
v8_value->IsSymbol())) {
return napi_set_last_error(env, napi_invalid_arg);
}
}

v8impl::Reference* reference =
Expand Down Expand Up @@ -3257,3 +3259,12 @@ napi_status NAPI_CDECL napi_is_detached_arraybuffer(napi_env env,

return napi_clear_last_error(env);
}

napi_status NAPI_CDECL node_api_is_feature_enabled(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

node_api_features feature,
bool* result) {
CHECK_ENV(env);
CHECK_ARG(env, result);
*result = env->IsFeatureEnabled(feature);
return napi_clear_last_error(env);
}
16 changes: 13 additions & 3 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,11 @@ class RefTracker {
} // end of namespace v8impl

struct napi_env__ {
explicit napi_env__(v8::Local<v8::Context> context)
: isolate(context->GetIsolate()), context_persistent(isolate, context) {
explicit napi_env__(v8::Local<v8::Context> context,
node_api_features* features)
: isolate(context->GetIsolate()),
context_persistent(isolate, context),
features(features == nullptr ? node_api_feature_none : *features) {
CHECK_EQ(isolate, context->GetIsolate());
napi_clear_last_error(this);
}
Expand Down Expand Up @@ -89,13 +92,19 @@ struct napi_env__ {
}
}

// This should be overridden to schedule the finalization to a properiate
// This should be overridden to schedule the finalization to appropriate
// timing, like next tick of the event loop.
virtual void CallFinalizer(napi_finalize cb, void* data, void* hint) {
v8::HandleScope handle_scope(isolate);
CallIntoModule([&](napi_env env) { cb(env, data, hint); });
}

bool IsFeatureEnabled(node_api_features feature) {
// By comparing results of `&` operation to the feature parameter
// we allow to test for multiple feature flags.
return (features & feature) == feature;
}

virtual void DeleteMe() {
// First we must finalize those references that have `napi_finalizer`
// callbacks. The reason is that addons might store other references which
Expand All @@ -122,6 +131,7 @@ struct napi_env__ {
int open_callback_scopes = 0;
int refs = 1;
void* instance_data = nullptr;
node_api_features features = node_api_feature_none;

protected:
// Should not be deleted directly. Delete with `napi_env__::DeleteMe()`
Expand Down
34 changes: 27 additions & 7 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@
#include <memory>

node_napi_env__::node_napi_env__(v8::Local<v8::Context> context,
const std::string& module_filename)
: napi_env__(context), filename(module_filename) {
const std::string& module_filename,
node_api_features* features)
: napi_env__(context, features), filename(module_filename) {
CHECK_NOT_NULL(node_env());
}

Expand Down Expand Up @@ -126,10 +127,11 @@ class BufferFinalizer : private Finalizer {
};

inline napi_env NewEnv(v8::Local<v8::Context> context,
const std::string& module_filename) {
const std::string& module_filename,
node_api_features* features) {
node_napi_env result;

result = new node_napi_env__(context, module_filename);
result = new node_napi_env__(context, module_filename, features);
// TODO(addaleax): There was previously code that tried to delete the
// napi_env when its v8::Context was garbage collected;
// However, as long as N-API addons using this napi_env are in place,
Expand Down Expand Up @@ -586,24 +588,42 @@ class AsyncContext {

} // end of namespace v8impl

void napi_module_register_by_symbol_with_features(
v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context,
napi_addon_register_func init,
node_api_features* features);

// Intercepts the Node-V8 module registration callback. Converts parameters
// to NAPI equivalents and then calls the registration callback specified
// by the NAPI module.
static void napi_module_register_cb(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context,
void* priv) {
napi_module_register_by_symbol(
napi_module_register_by_symbol_with_features(
exports,
module,
context,
static_cast<const napi_module*>(priv)->nm_register_func);
static_cast<const napi_module*>(priv)->nm_register_func,
static_cast<const napi_module*>(priv)->nm_features);
}

void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context,
napi_addon_register_func init) {
napi_module_register_by_symbol_with_features(
exports, module, context, init, nullptr);
}

void napi_module_register_by_symbol_with_features(
v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context,
napi_addon_register_func init,
node_api_features* features) {
node::Environment* node_env = node::Environment::GetCurrent(context);
std::string module_filename = "";
if (init == nullptr) {
Expand Down Expand Up @@ -631,7 +651,7 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
}

// Create a new napi_env for this specific module.
napi_env env = v8impl::NewEnv(context, module_filename);
napi_env env = v8impl::NewEnv(context, module_filename, features);

napi_value _exports;
env->CallIntoModule([&](napi_env env) {
Expand Down
30 changes: 29 additions & 1 deletion src/node_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ typedef struct napi_module {
napi_addon_register_func nm_register_func;
const char* nm_modname;
void* nm_priv;
#ifdef NAPI_EXPERIMENTAL
node_api_features* nm_features;
void* reserved[3];
#else
void* reserved[4];
#endif
} napi_module;

#define NAPI_MODULE_VERSION 1
Expand Down Expand Up @@ -73,16 +78,39 @@ typedef struct napi_module {
static void fn(void)
#endif

#ifdef NAPI_EXPERIMENTAL
#ifdef NODE_API_CUSTOM_FEATURES

// Define value of node_api_module_features variable in your module when
// NODE_API_CUSTOM_FEATURES is set in gyp file.
extern node_api_features node_api_module_features;
#define NODE_API_DEFINE_DEFAULT_FEATURES

#else // NODE_API_CUSTOM_FEATURES

#define NODE_API_DEFINE_DEFAULT_FEATURES \
static node_api_features node_api_module_features = node_api_default_features;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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?


#endif // NODE_API_CUSTOM_FEATURES

#define NODE_API_FEATURES_PTR /* NOLINT */ &node_api_module_features,

#else // NAPI_EXPERIMENTAL
#define NODE_API_DEFINE_DEFAULT_FEATURES
#define NODE_API_FEATURES_PTR
#endif // NAPI_EXPERIMENTAL

#define NAPI_MODULE_X(modname, regfunc, priv, flags) \
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It may be simpler - let me try.

EXTERN_C_START \
NODE_API_DEFINE_DEFAULT_FEATURES \
static napi_module _module = { \
NAPI_MODULE_VERSION, \
flags, \
__FILE__, \
regfunc, \
#modname, \
priv, \
{0}, \
NODE_API_FEATURES_PTR{0}, \
}; \
NAPI_C_CTOR(_register_##modname) { napi_module_register(&_module); } \
EXTERN_C_END
Expand Down
3 changes: 2 additions & 1 deletion src/node_api_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@

struct node_napi_env__ : public napi_env__ {
node_napi_env__(v8::Local<v8::Context> context,
const std::string& module_filename);
const std::string& module_filename,
node_api_features* features);

bool can_call_into_js() const override;
v8::Maybe<bool> mark_arraybuffer_as_untransferable(
Expand Down
1 change: 1 addition & 0 deletions test/js-native-api/common.h
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#define NAPI_EXPERIMENTAL
#include <js_native_api.h>

// Empty value so that macros here are able to return NULL or void
Expand Down
1 change: 1 addition & 0 deletions test/js-native-api/entry_point.c
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#define NAPI_EXPERIMENTAL
#include <node_api.h>

EXTERN_C_START
Expand Down
3 changes: 2 additions & 1 deletion test/js-native-api/test_reference/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
"sources": [
"../entry_point.c",
"test_reference.c"
]
],
'defines': [ 'NODE_API_CUSTOM_FEATURES' ]
}
]
}
5 changes: 5 additions & 0 deletions test/js-native-api/test_reference/test_reference.c
Original file line number Diff line number Diff line change
Expand Up @@ -283,4 +283,9 @@ napi_value Init(napi_env env, napi_value exports) {

return exports;
}

// Make sure that this test uses the old napi_ref behavior.
node_api_features node_api_module_features =
node_api_default_features & ~node_api_feature_reference_all_types;

EXTERN_C_END
11 changes: 11 additions & 0 deletions test/js-native-api/test_reference_all_types/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"targets": [
{
"target_name": "test_reference_all_types",
"sources": [
"../entry_point.c",
"test_reference_all_types.c"
]
}
]
}
Loading