src: use uv_async_t for WeakRefs

addaleax · addaleax · commit 3b8b241cc28a · 2019-11-24T00:34:36.000+01:00
Schedule a task on the main event loop, similar to what the HTML spec recommends for browsers. Alternative to nodejs#30198
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -1118,6 +1118,7 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) {
 inline void Environment::RegisterFinalizationGroupForCleanup(
     v8::Local<v8::FinalizationGroup> group) {
   cleanup_finalization_groups_.emplace_back(isolate(), group);
+  uv_async_send(&cleanup_finalization_groups_async_);
 }
 
 size_t CleanupHookCallback::Hash::operator()(
diff --git a/src/env.cc b/src/env.cc
@@ -460,8 +460,18 @@ void Environment::InitializeLibuv(bool start_profiler_idle_notifier) {
   // will be recorded with state=IDLE.
   uv_prepare_init(event_loop(), &idle_prepare_handle_);
   uv_check_init(event_loop(), &idle_check_handle_);
+  uv_async_init(
+      event_loop(),
+      &cleanup_finalization_groups_async_,
+      [](uv_async_t* async) {
+        Environment* env = ContainerOf(
+            &Environment::cleanup_finalization_groups_async_, async);
+        env->CleanupFinalizationGroups();
+      }
+     );
   uv_unref(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
   uv_unref(reinterpret_cast<uv_handle_t*>(&idle_check_handle_));
+  uv_unref(reinterpret_cast<uv_handle_t*>(&cleanup_finalization_groups_async_));
 
   thread_stopper()->Install(
     this, static_cast<void*>(this), [](uv_async_t* handle) {
@@ -524,6 +534,10 @@ void Environment::RegisterHandleCleanups() {
       reinterpret_cast<uv_handle_t*>(&idle_check_handle_),
       close_and_finish,
       nullptr);
+  RegisterHandleCleanup(
+      reinterpret_cast<uv_handle_t*>(&cleanup_finalization_groups_async_),
+      close_and_finish,
+      nullptr);
 }
 
 void Environment::CleanupHandles() {
@@ -1040,19 +1054,27 @@ char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
   return new_data;
 }
 
-bool Environment::RunWeakRefCleanup() {
+void Environment::RunWeakRefCleanup() {
   isolate()->ClearKeptObjects();
+}
 
-  while (!cleanup_finalization_groups_.empty()) {
+void Environment::CleanupFinalizationGroups() {
+  HandleScope handle_scope(isolate());
+  Context::Scope context_scope(context());
+  TryCatchScope try_catch(this);
+
+  while (!cleanup_finalization_groups_.empty() && can_call_into_js()) {
     Local<FinalizationGroup> fg =
         cleanup_finalization_groups_.front().Get(isolate());
     cleanup_finalization_groups_.pop_front();
     if (!FinalizationGroup::Cleanup(fg).FromMaybe(false)) {
-      return false;
+      if (try_catch.HasCaught() && !try_catch.HasTerminated())
+        errors::TriggerUncaughtException(isolate(), try_catch);
+      // Re-schedule the execution of the remainder of the queue.
+      uv_async_send(&cleanup_finalization_groups_async_);
+      return;
     }
   }
-
-  return true;
 }
 
 void AsyncRequest::Install(Environment* env, void* data, uv_async_cb target) {
diff --git a/src/env.h b/src/env.h
@@ -1128,7 +1128,8 @@ class Environment : public MemoryRetainer {
   void RunAtExitCallbacks();
 
   void RegisterFinalizationGroupForCleanup(v8::Local<v8::FinalizationGroup> fg);
-  bool RunWeakRefCleanup();
+  void RunWeakRefCleanup();
+  void CleanupFinalizationGroups();
 
   // Strings and private symbols are shared across shared contexts
   // The getters simply proxy to the per-isolate primitive.
@@ -1270,6 +1271,7 @@ class Environment : public MemoryRetainer {
   uv_idle_t immediate_idle_handle_;
   uv_prepare_t idle_prepare_handle_;
   uv_check_t idle_check_handle_;
+  uv_async_t cleanup_finalization_groups_async_;
   bool profiler_idle_notifier_started_ = false;
 
   AsyncHooks async_hooks_;
diff --git a/test/parallel/test-finalization-group-regular-gc.js b/test/parallel/test-finalization-group-regular-gc.js
@@ -0,0 +1,25 @@
+// Flags: --harmony-weak-refs
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+// Test that finalization callbacks do not crash when caused through a regular
+// GC (not global.gc()).
+
+const start = Date.now();
+const g = new globalThis.FinalizationGroup(() => {
+  const diff = Date.now() - start;
+  assert(diff < 10000, `${diff} >= 10000`);
+});
+g.register({}, 42);
+
+setImmediate(() => {
+  const arr = [];
+  // Build up enough memory usage to hopefully trigger a platform task but not
+  // enough to trigger GC as an interrupt.
+  while (arr.length < 1000000) arr.push([]);
+
+  setTimeout(() => {
+    g;  // Keep reference alive.
+  }, 200000).unref();
+});

Original file line number	Diff line number	Diff line change
`@@ -1118,6 +1118,7 @@ void Environment::RemoveCleanupHook(void (fn)(void), void* arg) {`
`1118`	`1118`	`inline void Environment::RegisterFinalizationGroupForCleanup(`
`1119`	`1119`	`v8::Local<v8::FinalizationGroup> group) {`
`1120`	`1120`	`cleanup_finalization_groups_.emplace_back(isolate(), group);`
	`1121`	`+ uv_async_send(&cleanup_finalization_groups_async_);`
`1121`	`1122`	`}`
`1122`	`1123`
`1123`	`1124`	`size_t CleanupHookCallback::Hash::operator()(`