Skip to content

node-api: run Node-API finalizers in GC second pass #42208

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
vmoroz wants to merge 9 commits into from
Closed

node-api: run Node-API finalizers in GC second pass #42208

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bfc9dd0
Implement finalizer_queue to run finalizers earlier
vmoroz Mar 4, 2022
63532d7
Fix C++ lint issues
vmoroz Mar 4, 2022
0a5c446
node-api: implement finalizer_queue to run finalizers earlier
vmoroz Mar 4, 2022
832409b
Fix C++ lint issues
vmoroz Mar 4, 2022
aaf43e2
Merge branch 'FinalizerQueue' of https://github.com/vmoroz/node into …
vmoroz Mar 4, 2022
d004b75
add public node_api_call_finalizers and unit tests
vmoroz Mar 11, 2022
518482f
implement node_api_set_finalizer_error_handler
vmoroz Mar 18, 2022
623d5a6
fix lint-cpp and lint-js issues
vmoroz Mar 18, 2022
66b25ac
fix lint-md issues
vmoroz Mar 18, 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
Prev Previous commit
Next Next commit
node-api: implement finalizer_queue to run finalizers earlier
  • Loading branch information
@vmoroz
vmoroz committed Mar 4, 2022
commit 0a5c446380771a07d6f2dc928925bf422a6ad1ea
9 changes: 8 additions & 1 deletion src/js_native_api_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,7 @@ void RefBase::Finalize(bool is_env_teardown) {
Delete(this);
} else {
_finalize_ran = true;
Unlink();
}
}

Expand Down Expand Up @@ -602,6 +603,8 @@ Reference::~Reference() {
// and we need to delete it here.
if (!_secondPassScheduled) {
delete _secondPassParameter;
} else if (_secondPassParameter) {
*_secondPassParameter = nullptr;
}
}

Expand Down Expand Up @@ -694,6 +697,10 @@ void Reference::FinalizeCallback(
reference->_secondPassScheduled = true;

data.SetSecondPassCallback(SecondPassCallback);

// Add the reference to the finalizing_queue
reference->Unlink();
reference->Link(&reference->_env->finalizing_queue);
}

// Second pass callbacks are scheduled with platform tasks. At env teardown,
Expand All @@ -715,7 +722,7 @@ void Reference::SecondPassCallback(
return;
}
reference->_secondPassParameter = nullptr;
reference->Finalize();
v8impl::RefTracker::FinalizeAll(&reference->_env->finalizing_queue, /*isEnvTeardown:*/false);
}

} // end of namespace v8impl
Expand Down
10 changes: 7 additions & 3 deletions src/js_native_api_v8.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ class RefTracker {
next_ = nullptr;
}

static void FinalizeAll(RefList* list) {
static void FinalizeAll(RefList* list, bool isEnvTeardown = true) {
while (list->next_ != nullptr) {
list->next_->Finalize(true);
list->next_->Finalize(isEnvTeardown);
}
}

Expand All @@ -64,6 +64,7 @@ struct napi_env__ {
// they delete during their `napi_finalizer` callbacks. If we deleted such
// references here first, they would be doubly deleted when the
// `napi_finalizer` deleted them subsequently.
v8impl::RefTracker::FinalizeAll(&finalizing_queue);
v8impl::RefTracker::FinalizeAll(&finalizing_reflist);
v8impl::RefTracker::FinalizeAll(&reflist);
}
Expand Down Expand Up @@ -116,6 +117,7 @@ struct napi_env__ {
// have such a callback. See `~napi_env__()` above for details.
v8impl::RefTracker::RefList reflist;
v8impl::RefTracker::RefList finalizing_reflist;
v8impl::RefTracker::RefList finalizing_queue;
napi_extended_error_info last_error;
int open_handle_scopes = 0;
int open_callback_scopes = 0;
Expand Down Expand Up @@ -361,6 +363,8 @@ class TryCatch : public v8::TryCatch {
~TryCatch() {
if (HasCaught()) {
_env->last_exception.Reset(_env->isolate, Exception());
} else {
v8impl::RefTracker::FinalizeAll(&_env->finalizing_queue, /*isEnvTeardown:*/false);
}
}

Expand All @@ -369,7 +373,7 @@ class TryCatch : public v8::TryCatch {
};

// Wrapper around v8impl::Persistent that implements reference counting.
class RefBase : protected Finalizer, RefTracker {
class RefBase : protected Finalizer, protected RefTracker {
protected:
RefBase(napi_env env,
uint32_t initial_refcount,
Expand Down