Skip to content

node-api: generalize finalizer second pass callback #44141

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.

Sign up for GitHub

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

Closed
legendecas wants to merge 2 commits into from
Closed

node-api: generalize finalizer second pass callback #44141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57a58d1
node-api: generalize finalizer second pass callback
legendecas Aug 5, 2022
344493a
fixup! verify dangling napi_ref in napi_wrap
legendecas Oct 14, 2022
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
Next Next commit
node-api: generalize finalizer second pass callback 
Generalize the finalizer's second pass callback to make it
cancellable and simplify the code around the second pass callback.

With this change, it is determined that Reference::Finalize or
RefBase::Finalize are called once, either from the env's shutdown,
or from the env's second pass callback.

All existing node-api js tests should pass without a touch. The
js_native_api cctest is no longer applicable with this change,
just removing it.
  • Loading branch information
@legendecas
legendecas committed Nov 18, 2022
commit 57a58d1bb8cb452139cdf590c6b8de90bf28a8db
1 change: 0 additions & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,6 @@
'test/cctest/test_base_object_ptr.cc',
'test/cctest/test_node_postmortem_metadata.cc',
'test/cctest/test_environment.cc',
'test/cctest/test_js_native_api_v8.cc',
'test/cctest/test_linked_binding.cc',
'test/cctest/test_node_api.cc',
'test/cctest/test_per_process.cc',
Expand Down
243 changes: 75 additions & 168 deletions src/js_native_api_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ inline napi_status Unwrap(napi_env env,
if (action == RemoveWrap) {
CHECK(obj->DeletePrivate(context, NAPI_PRIVATE_KEY(context, wrapper))
.FromJust());
Reference::Delete(reference);
delete reference;
}

return GET_RETURN_STATUS(env);
Expand Down Expand Up @@ -464,11 +464,20 @@ RefBase::RefBase(napi_env env,
void* finalize_data,
void* finalize_hint)
: Finalizer(env, finalize_callback, finalize_data, finalize_hint),
_refcount(initial_refcount),
_delete_self(delete_self) {
refcount_(initial_refcount),
delete_self_(delete_self) {
Link(finalize_callback == nullptr ? &env->reflist : &env->finalizing_reflist);
}

// When a RefBase is being deleted, it may have been queued to call its
// finalizer.
RefBase::~RefBase() {
// Remove from the env's tracked list.
Unlink();
// Try to remove the finalizer from the scheduled second pass callback.
env_->DequeueFinalizer(this);
}

RefBase* RefBase::New(napi_env env,
uint32_t initial_refcount,
bool delete_self,
Expand All @@ -483,105 +492,65 @@ RefBase* RefBase::New(napi_env env,
finalize_hint);
}

RefBase::~RefBase() {
Unlink();
}

void* RefBase::Data() {
return _finalize_data;
}

// Delete is called in 2 ways. Either from the finalizer or
// from one of Unwrap or napi_delete_reference.
//
// When it is called from Unwrap or napi_delete_reference we only
// want to do the delete if the finalizer has already run or
// cannot have been queued to run (ie the reference count is > 0),
// otherwise we may crash when the finalizer does run.
// If the finalizer may have been queued and has not already run
// delay the delete until the finalizer runs by not doing the delete
// and setting _delete_self to true so that the finalizer will
// delete it when it runs.
//
// The second way this is called is from
// the finalizer and _delete_self is set. In this case we
// know we need to do the deletion so just do it.
void RefBase::Delete(RefBase* reference) {
if ((reference->RefCount() != 0) || (reference->_delete_self) ||
(reference->_finalize_ran)) {
delete reference;
} else {
// defer until finalizer runs as
// it may already be queued
reference->_delete_self = true;
}
return finalize_data_;
}

uint32_t RefBase::Ref() {
return ++_refcount;
return ++refcount_;
}

uint32_t RefBase::Unref() {
if (_refcount == 0) {
if (refcount_ == 0) {
return 0;
}
return --_refcount;
return --refcount_;
}

uint32_t RefBase::RefCount() {
return _refcount;
}

void RefBase::Finalize(bool is_env_teardown) {
// In addition to being called during environment teardown, this method is
// also the entry point for the garbage collector. During environment
// teardown we have to remove the garbage collector's reference to this
// method so that, if, as part of the user's callback, JS gets executed,
// resulting in a garbage collection pass, this method is not re-entered as
// part of that pass, because that'll cause a double free (as seen in
// https://github.com/nodejs/node/issues/37236).
//
// Since this class does not have access to the V8 persistent reference,
// this method is overridden in the `Reference` class below. Therein the
// weak callback is removed, ensuring that the garbage collector does not
// re-enter this method, and the method chains up to continue the process of
// environment-teardown-induced finalization.

// During environment teardown we have to convert a strong reference to
// a weak reference to force the deferring behavior if the user's finalizer
// happens to delete this reference so that the code in this function that
// follows the call to the user's finalizer may safely access variables from
// this instance.
if (is_env_teardown && RefCount() > 0) _refcount = 0;

if (_finalize_callback != nullptr) {
// This ensures that we never call the finalizer twice.
napi_finalize fini = _finalize_callback;
_finalize_callback = nullptr;
_env->CallFinalizer(fini, _finalize_data, _finalize_hint);
}

// this is safe because if a request to delete the reference
// is made in the finalize_callback it will defer deletion
// to this block and set _delete_self to true
if (_delete_self || is_env_teardown) {
Delete(this);
} else {
_finalize_ran = true;
return refcount_;
}

void RefBase::Finalize() {
bool delete_self = delete_self_;
// Swap out the field finalize_callback so that it can not be accidentally
// called more than once.
napi_finalize finalize_callback = finalize_callback_;
Comment thread
legendecas marked this conversation as resolved.
finalize_callback_ = nullptr;

// Either the RefBase is going to be deleted in the finalize_callback or not,
// it should be removed from the tracked list.
Unlink();
// 1. If the finalize_callback is present, it should either delete the
// RefBase, or set the flag `delete_self`.
Copy link
Copy Markdown
Member

@vmoroz vmoroz Aug 18, 2022

Choose a reason for hiding this comment

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

It seems that we do not set the delete_self flag anymore in our code. It is only set during Reference construction. Should we change this comment?

Copy link
Copy Markdown
Member Author

@legendecas legendecas Aug 19, 2022

Choose a reason for hiding this comment

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

The flag delete_self_ is still used in apis like napi_set_instance_data, which transfers user data ownership to the implementation of node-api.

// 2. If the finalizer is not present, the RefBase can be deleted after the
// call.
if (finalize_callback != nullptr) {
env_->CallFinalizer(finalize_callback, finalize_data_, finalize_hint_);
// No access to `this` after finalize_callback is called.
}

// If the RefBase is not self-destructive, userland code should delete it.
// Now delete it if it is self-destructive.
if (delete_self) {
delete this;
Copy link
Copy Markdown
Member

@vmoroz vmoroz Aug 19, 2022

Choose a reason for hiding this comment

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

Is it possible to get to a situation when we delete the object at the same address twice? What if some other object allocated at the same address after the first delete?
It feels that it would be safer to avoid Reference deletion while we are in its Finalizer callback.
E.g. add a flag that we are in a finalizer, and if so, then just record the deletion intent by setting the delete_self_ field. Then, after exiting the finalizer we know that the Reference is still alive and act on its delete_self_ field instead of the locally stored delete_self variable.

Copy link
Copy Markdown
Member Author

@legendecas legendecas Aug 19, 2022

Choose a reason for hiding this comment

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

delete_self_ flag is used to indicate that the data is owned by node-api and node-api should delete the RefBase once the data has been finalized.

}
}

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),
_secondPassParameter(new SecondPassCallParameterRef(this)),
_secondPassScheduled(false) {
persistent_(env->isolate, value) {
if (RefCount() == 0) {
SetWeak();
}
}

Reference::~Reference() {
// Reset the handle. And no weak callback will be invoked.
persistent_.Reset();
}

Reference* Reference::New(napi_env env,
v8::Local<v8::Value> value,
uint32_t initial_refcount,
Expand All @@ -598,24 +567,25 @@ Reference* Reference::New(napi_env env,
finalize_hint);
}

Reference::~Reference() {
// If the second pass callback is scheduled, it will delete the
// parameter passed to it, otherwise it will never be scheduled
// and we need to delete it here.
if (!_secondPassScheduled) {
delete _secondPassParameter;
}
}

uint32_t Reference::Ref() {
// When the persistent_ is cleared in the WeakCallback, and a second pass
// callback is pending, return 0 unconditionally.
if (persistent_.IsEmpty()) {
return 0;
}
uint32_t refcount = RefBase::Ref();
if (refcount == 1) {
ClearWeak();
persistent_.ClearWeak();
}
return refcount;
}

uint32_t Reference::Unref() {
// When the persistent_ is cleared in the WeakCallback, and a second pass
// callback is pending, return 0 unconditionally.
if (persistent_.IsEmpty()) {
return 0;
}
uint32_t old_refcount = RefCount();
uint32_t refcount = RefBase::Unref();
if (old_refcount == 1 && refcount == 0) {
Expand All @@ -625,99 +595,36 @@ uint32_t Reference::Unref() {
}

v8::Local<v8::Value> Reference::Get() {
if (_persistent.IsEmpty()) {
if (persistent_.IsEmpty()) {
return v8::Local<v8::Value>();
} else {
return v8::Local<v8::Value>::New(_env->isolate, _persistent);
return v8::Local<v8::Value>::New(env_->isolate, persistent_);
}
}

void Reference::Finalize(bool is_env_teardown) {
// During env teardown, `~napi_env()` alone is responsible for finalizing.
// Thus, we don't want any stray gc passes to trigger a second call to
// `RefBase::Finalize()`. ClearWeak will ensure that even if the
// gc is in progress no Finalization will be run for this Reference
// by the gc.
if (is_env_teardown) {
ClearWeak();
}
void Reference::Finalize() {
// Unconditionally reset the persistent handle so that no weak callback will
// be invoked again.
persistent_.Reset();

// Chain up to perform the rest of the finalization.
RefBase::Finalize(is_env_teardown);
}

// ClearWeak is marking the Reference so that the gc should not
// collect it, but it is possible that a second pass callback
// may have been scheduled already if we are in shutdown. We clear
// the secondPassParameter so that even if it has been
// scheduled no Finalization will be run.
void Reference::ClearWeak() {
if (!_persistent.IsEmpty()) {
_persistent.ClearWeak();
}
if (_secondPassParameter != nullptr) {
*_secondPassParameter = nullptr;
}
RefBase::Finalize();
}

// Mark the reference as weak and eligible for collection
// by the gc.
void Reference::SetWeak() {
if (_secondPassParameter == nullptr) {
// This means that the Reference has already been processed
// by the second pass callback, so its already been Finalized, do
// nothing
return;
}
_persistent.SetWeak(
_secondPassParameter, FinalizeCallback, v8::WeakCallbackType::kParameter);
*_secondPassParameter = this;
persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
Comment thread
legendecas marked this conversation as resolved.
}

// The N-API finalizer callback may make calls into the engine. V8's heap is
// not in a consistent state during the weak callback, and therefore it does
// not support calls back into it. However, it provides a mechanism for adding
// a finalizer which may make calls back into the engine by allowing us to
// attach such a second-pass finalizer from the first pass finalizer. Thus,
// we do that here to ensure that the N-API finalizer callback is free to call
// into the engine.
void Reference::FinalizeCallback(
const v8::WeakCallbackInfo<SecondPassCallParameterRef>& data) {
SecondPassCallParameterRef* parameter = data.GetParameter();
Reference* reference = *parameter;
if (reference == nullptr) {
return;
}

// The reference must be reset during the first pass.
reference->_persistent.Reset();
// Mark the parameter not delete-able until the second pass callback is
// invoked.
reference->_secondPassScheduled = true;

data.SetSecondPassCallback(SecondPassCallback);
}

// Second pass callbacks are scheduled with platform tasks. At env teardown,
// the tasks may have already be scheduled and we are unable to cancel the
// second pass callback task. We have to make sure that parameter is kept
// alive until the second pass callback is been invoked. In order to do
// this and still allow our code to Finalize/delete the Reference during
// shutdown we have to use a separately allocated parameter instead
// of a parameter within the Reference object itself. This is what
// is stored in _secondPassParameter and it is allocated in the
// constructor for the Reference.
void Reference::SecondPassCallback(
const v8::WeakCallbackInfo<SecondPassCallParameterRef>& data) {
SecondPassCallParameterRef* parameter = data.GetParameter();
Reference* reference = *parameter;
delete parameter;
if (reference == nullptr) {
// the reference itself has already been deleted so nothing to do
return;
}
reference->_secondPassParameter = nullptr;
reference->Finalize();
// not support calls back into it. Enqueue the invocation of the finalizer.
void Reference::WeakCallback(const v8::WeakCallbackInfo<Reference>& data) {
Reference* reference = data.GetParameter();
Comment thread
legendecas marked this conversation as resolved.
// The reference must be reset during the weak callback as the API protocol.
reference->persistent_.Reset();
reference->env_->EnqueueFinalizer(reference);
}

} // end of namespace v8impl
Expand Down Expand Up @@ -2516,7 +2423,7 @@ napi_status NAPI_CDECL napi_delete_reference(napi_env env, napi_ref ref) {
CHECK_ENV(env);
CHECK_ARG(env, ref);

v8impl::Reference::Delete(reinterpret_cast<v8impl::Reference*>(ref));
delete reinterpret_cast<v8impl::Reference*>(ref);

return napi_clear_last_error(env);
}
Expand Down Expand Up @@ -3206,7 +3113,7 @@ napi_status NAPI_CDECL napi_set_instance_data(napi_env env,
if (old_data != nullptr) {
// Our contract so far has been to not finalize any old data there may be.
// So we simply delete it.
v8impl::RefBase::Delete(old_data);
delete old_data;
}

env->instance_data =
Expand Down
Loading