src: refactor weakref cleanup

Author: devsnek

src/api/callback.cc

@@ -101,7 +101,7 @@ void InternalCallbackScope::Close() {
 
   if (!env_->can_call_into_js()) return;
 
-  OnScopeLeave weakref_cleanup([&]() { env_->RunWeakRefCleanup(); });
+  OnScopeLeave weakref_cleanup([&]() { env_->isolate()->ClearKeptObjects(); });
 
   if (!tick_info->has_tick_scheduled()) {
     MicrotasksScope::PerformCheckpoint(env_->isolate());

src/api/environment.cc

@@ -14,6 +14,7 @@ using v8::Context;
 using v8::EscapableHandleScope;
 using v8::FinalizationGroup;
 using v8::Function;
+using v8::Global;
 using v8::HandleScope;
 using v8::Isolate;
 using v8::Local;
@@ -77,13 +78,39 @@ static MaybeLocal<Value> PrepareStackTraceCallback(Local<Context> context,
   return result;
 }
 
+class FinalizationGroupCleanupTask final {
+ public:
+  FinalizationGroupCleanupTask(Environment* env, Local<FinalizationGroup> group)
+      : env_(env), group_(env->isolate(), group) {}
+
+  inline Environment* env() { return env_; }
+  inline Local<FinalizationGroup> group() {
+    return group_.Get(env_->isolate());
+  }
+
+ private:
+  Environment* env_;
+  Global<FinalizationGroup> group_;
+};
+
+void HostCleanupFinalizationGroupMicrotask(void* data) {
+  FinalizationGroupCleanupTask* t =
+      reinterpret_cast<FinalizationGroupCleanupTask*>(data);
+  TryCatchScope try_catch(t->env());
+  if (!FinalizationGroup::Cleanup(t->group()).FromMaybe(false)) {
+    TriggerUncaughtException(t->env()->isolate(), try_catch);
+  }
+}
+
 static void HostCleanupFinalizationGroupCallback(
     Local<Context> context, Local<FinalizationGroup> group) {
   Environment* env = Environment::GetCurrent(context);
   if (env == nullptr) {
     return;
   }
-  env->RegisterFinalizationGroupForCleanup(group);
+  env->isolate()->EnqueueMicrotask(
+      HostCleanupFinalizationGroupMicrotask,
+      new FinalizationGroupCleanupTask(env, group));
 }
 
 void* NodeArrayBufferAllocator::Allocate(size_t size) {

src/env-inl.h

@@ -1115,11 +1115,6 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) {
   cleanup_hooks_.erase(search);
 }
 
-inline void Environment::RegisterFinalizationGroupForCleanup(
-    v8::Local<v8::FinalizationGroup> group) {
-  cleanup_finalization_groups_.emplace_back(isolate(), group);
-}
-
 size_t CleanupHookCallback::Hash::operator()(
     const CleanupHookCallback& cb) const {
   return std::hash<void*>()(cb.arg_);

src/env.cc

@@ -29,7 +29,6 @@ using v8::ArrayBuffer;
 using v8::Boolean;
 using v8::Context;
 using v8::EmbedderGraph;
-using v8::FinalizationGroup;
 using v8::Function;
 using v8::FunctionTemplate;
 using v8::HandleScope;
@@ -1052,21 +1051,6 @@ void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
   keep_alive_allocators_->insert(allocator);
 }
 
-bool Environment::RunWeakRefCleanup() {
-  isolate()->ClearKeptObjects();
-
-  while (!cleanup_finalization_groups_.empty()) {
-    Local<FinalizationGroup> fg =
-        cleanup_finalization_groups_.front().Get(isolate());
-    cleanup_finalization_groups_.pop_front();
-    if (!FinalizationGroup::Cleanup(fg).FromMaybe(false)) {
-      return false;
-    }
-  }
-
-  return true;
-}
-
 void AsyncRequest::Install(Environment* env, void* data, uv_async_cb target) {
   CHECK_NULL(async_);
   env_ = env;

src/env.h

@@ -1132,9 +1132,6 @@ class Environment : public MemoryRetainer {
   void AtExit(void (*cb)(void* arg), void* arg);
   void RunAtExitCallbacks();
 
-  void RegisterFinalizationGroupForCleanup(v8::Local<v8::FinalizationGroup> fg);
-  bool RunWeakRefCleanup();
-
   // Strings and private symbols are shared across shared contexts
   // The getters simply proxy to the per-isolate primitive.
 #define VP(PropertyName, StringValue) V(v8::Private, PropertyName)
@@ -1341,8 +1338,6 @@ class Environment : public MemoryRetainer {
   uint64_t thread_id_;
   std::unordered_set<worker::Worker*> sub_worker_contexts_;
 
-  std::deque<v8::Global<v8::FinalizationGroup>> cleanup_finalization_groups_;
-
   static void* const kNodeContextTagPtr;
   static int const kNodeContextTag;
 

src/node_task_queue.cc

@@ -43,7 +43,7 @@ static void EnqueueMicrotask(const FunctionCallbackInfo<Value>& args) {
 
 // Should be in sync with runNextTicks in internal/process/task_queues.js
 bool RunNextTicksNative(Environment* env) {
-  OnScopeLeave weakref_cleanup([&]() { env->RunWeakRefCleanup(); });
+  OnScopeLeave weakref_cleanup([&]() { env->isolate()->ClearKeptObjects(); });
 
   TickInfo* tick_info = env->tick_info();
   if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn())