-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
node-api: get Node API version used by addon #45715
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
Changes from 14 commits
bbb0fb5
c8c6f56
4c11c92
272ba92
f297f67
b4ae2a6
fe46dee
3748a4e
bd38aaf
ac53d6b
f10f20f
bb3cc12
4b709c1
ad50859
38d5f0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There is also a difference in the JavaScript In any case to avoid native memory leaks we require to call
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() { | ||
|
|
@@ -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(); | ||
| } | ||
|
|
@@ -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; | ||
|
|
@@ -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 | ||
|
|
@@ -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( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.