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: get Node API version used by addon #45715

Closed
wants to merge 15 commits into from
Closed
81 changes: 47 additions & 34 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1639,25 +1639,36 @@ If it is called more than once an error will be returned.

This API can be called even if there is a pending JavaScript exception.

### References to objects with a lifespan longer than that of the native method
### References to values with a lifespan longer than that of the native method

In some cases an addon will need to be able to create and reference objects
In some cases, an addon will need to be able to create and reference values
with a lifespan longer than that of a single native method invocation. For
example, to create a constructor and later use that constructor
in a request to creates instances, it must be possible to reference
in a request to create instances, it must be possible to reference
the constructor object across many different instance creation requests. This
would not be possible with a normal handle returned as a `napi_value` as
described in the earlier section. The lifespan of a normal handle is
managed by scopes and all scopes must be closed before the end of a native
method.

Node-API provides methods to create persistent references to an object.
Each persistent reference has an associated count with a value of 0
or higher. The count determines if the reference will keep
the corresponding object live. References with a count of 0 do not
prevent the object from being collected and are often called 'weak'
references. Any count greater than 0 will prevent the object
from being collected.
Node-API provides methods to create persistent references to values.
Each reference has an associated count with a value of 0 or higher.
The count determines if the reference will keep the corresponding value alive.
References with a count of 0 do not prevent values from being collected.
Values of object (object, function, external) and symbol types are becoming
'weak' references and can still be accessed while they are not collected.
Values of other types are released when the count becomes 0
and cannot be accessed from the reference any more.
Any count greater than 0 will prevent the values from being collected.

The Symbol values have different flavors. The true weak reference behavior is
only supported by local symbols created with the `Symbol()` constructor call.
The globally registered symbols created with the `Symbol.for()` call remain
always strong references because the garbage collector does not collect them.
The same is true for well-known symbols such as `Symbol.iterator`. They are
also never collected by the garbage collector. JavaScript's `WeakRef` and
`WeakMap` types return an error when the registered symbols are used.
They succeed for local and well-known symbols.

References can be created with an initial reference count. The count can
then be modified through [`napi_reference_ref`][] and
Expand All @@ -1668,21 +1679,24 @@ will return `NULL` for the returned `napi_value`. An attempt to call
[`napi_reference_ref`][] for a reference whose object has been collected
results in an error.

In Node-API version 8 and before we can only create references for a limited
vmoroz marked this conversation as resolved.
Show resolved Hide resolved
set of value types: object, external, function, and symbol. In newer Node-API
versions the references can be created for any value type.

References must be deleted once they are no longer required by the addon. When
a reference is deleted, it will no longer prevent the corresponding object from
being collected. Failure to delete a persistent reference results in
a 'memory leak' with both the native memory for the persistent reference and
the corresponding object on the heap being retained forever.

There can be multiple persistent references created which refer to the same
object, each of which will either keep the object live or not based on its
individual count. Multiple persistent references to the same object
can result in unexpectedly keeping alive native memory. The native structures
for a persistent reference must be kept alive until finalizers for the
referenced object are executed. If a new persistent reference is created
for the same object, the finalizers for that object will not be
run and the native memory pointed by the earlier persistent reference
will not be freed. This can be avoided by calling
a reference is deleted, it will no longer prevent the corresponding value from
being collected. If you don't delete a persistent reference, it can cause a
'memory leak'. This means that both the native memory for the persistent
reference and the corresponding value on the heap will be retained forever.

You can create multiple persistent references that refer to the same value.
Each reference will keep the value alive or not based on its individual count.
If there are multiple persistent references to the same value, it can
unexpectedly keep alive native memory. The native structures for a persistent
reference must be kept alive until finalizers for the referenced value are
executed. If you create a new persistent reference for the same value, the
finalizers for that value will not be run and the native memory pointed by
the earlier persistent reference will not be freed. To avoid this, call
`napi_delete_reference` in addition to `napi_reference_unref` when possible.

#### `napi_create_reference`
Expand All @@ -1700,15 +1714,18 @@ NAPI_EXTERN napi_status napi_create_reference(napi_env env,
```

* `[in] env`: The environment that the API is invoked under.
* `[in] value`: `napi_value` representing the `Object` to which we want a
reference.
* `[in] value`: `napi_value` to which we want to create a reference.
vmoroz marked this conversation as resolved.
Show resolved Hide resolved
* `[in] initial_refcount`: Initial reference count for the new reference.
* `[out] result`: `napi_ref` pointing to the new reference.

Returns `napi_ok` if the API succeeded.

This API creates a new reference with the specified reference count
to the `Object` passed in.
to the value passed in.

In Node-API version 8 and earlier, you could only create a reference for
object, function, external, and symbol value types. In newer Node-API
versions, you can create a reference for any value type.

#### `napi_delete_reference`

Expand Down Expand Up @@ -1787,18 +1804,14 @@ NAPI_EXTERN napi_status napi_get_reference_value(napi_env env,
napi_value* result);
```

the `napi_value passed` in or out of these methods is a handle to the
object to which the reference is related.

* `[in] env`: The environment that the API is invoked under.
* `[in] ref`: `napi_ref` for which we requesting the corresponding `Object`.
* `[out] result`: The `napi_value` for the `Object` referenced by the
`napi_ref`.
* `[in] ref`: `napi_ref` for which we are requesting the corresponding value.
* `[out] result`: The `napi_value` referenced by the `napi_ref`.

Returns `napi_ok` if the API succeeded.

If still valid, this API returns the `napi_value` representing the
JavaScript `Object` associated with the `napi_ref`. Otherwise, result
JavaScript value associated with the `napi_ref`. Otherwise, result
will be `NULL`.

### Cleanup on exit of the current Node.js environment
Expand Down
30 changes: 11 additions & 19 deletions src/api/environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -872,26 +872,18 @@ void AddLinkedBinding(Environment* env,

void AddLinkedBinding(Environment* env,
const char* name,
napi_addon_register_func fn) {
napi_addon_register_func fn,
int32_t module_api_version) {
node_module mod = {
-1,
NM_F_LINKED,
nullptr, // nm_dso_handle
nullptr, // nm_filename
nullptr, // nm_register_func
[](v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context,
void* priv) {
napi_module_register_by_symbol(
exports,
module,
context,
reinterpret_cast<napi_addon_register_func>(priv));
},
name,
reinterpret_cast<void*>(fn),
nullptr // nm_link
-1, // nm_version for Node-API
NM_F_LINKED, // nm_flags
nullptr, // nm_dso_handle
nullptr, // nm_filename
nullptr, // nm_register_func
get_node_api_context_register_func(env, name, module_api_version),
name, // nm_modname
reinterpret_cast<void*>(fn), // nm_priv
nullptr // nm_link
};
AddLinkedBinding(env, mod);
}
Expand Down
31 changes: 25 additions & 6 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,18 @@ inline napi_status Wrap(napi_env env,
return GET_RETURN_STATUS(env);
}

// In JavaScript, weak references can be created for object types (Object,
// Function, and external Object) and for local symbols that are created with
// the `Symbol` function call. Global symbols created with the `Symbol.for`
// 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

// This matches the current Node-API behavior.
inline bool CanBeHeldWeakly(v8::Local<v8::Value> value) {
return value->IsObject() || value->IsSymbol();
}

} // end of anonymous namespace

void Finalizer::ResetFinalizer() {
Expand Down Expand Up @@ -551,7 +563,8 @@ void RefBase::Finalize() {
template <typename... Args>
Reference::Reference(napi_env env, v8::Local<v8::Value> value, Args&&... args)
: RefBase(env, std::forward<Args>(args)...),
persistent_(env->isolate, value) {
persistent_(env->isolate, value),
can_be_weak_(CanBeHeldWeakly(value)) {
if (RefCount() == 0) {
SetWeak();
}
Expand Down Expand Up @@ -585,7 +598,7 @@ uint32_t Reference::Ref() {
return 0;
}
uint32_t refcount = RefBase::Ref();
if (refcount == 1) {
if (refcount == 1 && can_be_weak_) {
persistent_.ClearWeak();
}
return refcount;
Expand Down Expand Up @@ -625,7 +638,11 @@ void Reference::Finalize() {
// Mark the reference as weak and eligible for collection
// by the gc.
void Reference::SetWeak() {
persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
if (can_be_weak_) {
persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
} else {
persistent_.Reset();
}
}

// The N-API finalizer callback may make calls into the engine. V8's heap is
Expand Down Expand Up @@ -2419,9 +2436,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->module_api_version <= 8) {
if (!(v8_value->IsObject() || v8_value->IsFunction() ||
v8_value->IsSymbol())) {
return napi_set_last_error(env, napi_invalid_arg);
}
}

v8impl::Reference* reference = v8impl::Reference::New(
Expand Down
9 changes: 7 additions & 2 deletions src/js_native_api_v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,11 @@ class Finalizer;
} // 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,
int32_t module_api_version)
: isolate(context->GetIsolate()),
context_persistent(isolate, context),
module_api_version(module_api_version) {
napi_clear_last_error(this);
}

Expand Down Expand Up @@ -144,6 +147,7 @@ struct napi_env__ {
int open_callback_scopes = 0;
int refs = 1;
void* instance_data = nullptr;
int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION;

protected:
// Should not be deleted directly. Delete with `napi_env__::DeleteMe()`
Expand Down Expand Up @@ -419,6 +423,7 @@ class Reference : public RefBase {
void SetWeak();

v8impl::Persistent<v8::Value> persistent_;
bool can_be_weak_;
};

} // end of namespace v8impl
Expand Down
8 changes: 5 additions & 3 deletions src/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -1237,9 +1237,11 @@ NODE_EXTERN void AddLinkedBinding(Environment* env,
const char* name,
addon_context_register_func fn,
void* priv);
NODE_EXTERN void AddLinkedBinding(Environment* env,
const char* name,
napi_addon_register_func fn);
NODE_EXTERN void AddLinkedBinding(
Environment* env,
const char* name,
napi_addon_register_func fn,
int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION);

/* Registers a callback with the passed-in Environment instance. The callback
* is called after the event loop exits, but before the VM is disposed.
Expand Down
76 changes: 70 additions & 6 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,
int32_t module_api_version)
: napi_env__(context, module_api_version), filename(module_filename) {
CHECK_NOT_NULL(node_env());
}

Expand Down Expand Up @@ -151,11 +152,36 @@ class BufferFinalizer : private Finalizer {
~BufferFinalizer() { env_->Unref(); }
};

void ThrowNodeApiVersionError(node::Environment* node_env,
const char* module_name,
int32_t module_api_version) {
std::string error_message;
error_message += module_name;
error_message += " requires Node-API version ";
error_message += std::to_string(module_api_version);
error_message += ", but this version of Node.js only supports version ";
error_message += NODE_STRINGIFY(NAPI_VERSION) " add-ons.";
node_env->ThrowError(error_message.c_str());
}

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

result = new node_napi_env__(context, module_filename);
// Validate module_api_version.
if (module_api_version < NODE_API_DEFAULT_MODULE_API_VERSION) {
module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION;
} else if (module_api_version > NAPI_VERSION &&
module_api_version != NAPI_VERSION_EXPERIMENTAL) {
node::Environment* node_env = node::Environment::GetCurrent(context);
CHECK_NOT_NULL(node_env);
ThrowNodeApiVersionError(
node_env, module_filename.c_str(), module_api_version);
return nullptr;
}

result = new node_napi_env__(context, module_filename, module_api_version);
// 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 @@ -623,10 +649,48 @@ static void napi_module_register_cb(v8::Local<v8::Object> exports,
static_cast<const napi_module*>(priv)->nm_register_func);
}

template <int32_t module_api_version>
static void node_api_context_register_func(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context,
void* priv) {
napi_module_register_by_symbol(
exports,
module,
context,
reinterpret_cast<napi_addon_register_func>(priv),
module_api_version);
}

// This function must be augmented for each new Node API version.
// The key role of this function is to encode module_api_version in the function
// pointer. We are not going to have many Node API versions and having one
// function per version is relatively cheap. It avoids dynamic memory
// allocations or implementing more expensive changes to module registration.
// Currently AddLinkedBinding is the only user of this function.
node::addon_context_register_func get_node_api_context_register_func(
node::Environment* node_env,
const char* module_name,
int32_t module_api_version) {
static_assert(
NAPI_VERSION == 8,
"New version of Node-API requires adding another else-if statement below "
"for the new version and updating this assert condition.");
if (module_api_version <= NODE_API_DEFAULT_MODULE_API_VERSION) {
vmoroz marked this conversation as resolved.
Show resolved Hide resolved
return node_api_context_register_func<NODE_API_DEFAULT_MODULE_API_VERSION>;
} else if (module_api_version == NAPI_VERSION_EXPERIMENTAL) {
return node_api_context_register_func<NAPI_VERSION_EXPERIMENTAL>;
} else {
v8impl::ThrowNodeApiVersionError(node_env, module_name, module_api_version);
return nullptr;
}
}

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_addon_register_func init,
int32_t module_api_version) {
node::Environment* node_env = node::Environment::GetCurrent(context);
std::string module_filename = "";
if (init == nullptr) {
Expand Down Expand Up @@ -654,7 +718,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, module_api_version);

napi_value _exports = nullptr;
env->CallIntoModule([&](napi_env env) {
Expand Down