From 9e9e48bf7e20a425d02c2538da106ae97c0688c8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 24 Nov 2019 00:34:36 +0100 Subject: [PATCH] src: use uv_async_t for WeakRefs Schedule a task on the main event loop, similar to what the HTML spec recommends for browsers. Alternative to https://github.com/nodejs/node/pull/30198 PR-URL: https://github.com/nodejs/node/pull/30616 Reviewed-By: Gus Caplan Reviewed-By: Ben Noordhuis --- src/env-inl.h | 1 + src/env.cc | 31 ++++++++++++++++--- src/env.h | 4 ++- .../parallel/test-finalization-group-error.js | 4 +++ .../test-finalization-group-regular-gc.js | 25 +++++++++++++++ 5 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 test/parallel/test-finalization-group-regular-gc.js diff --git a/src/env-inl.h b/src/env-inl.h index 15b5010deb7c90..d34bd03c7c6896 100644 --- 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 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 index 5f9a6acb461f97..d907f65c27b892 100644 --- a/src/env.cc +++ b/src/env.cc @@ -460,8 +460,17 @@ 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(&idle_prepare_handle_)); uv_unref(reinterpret_cast(&idle_check_handle_)); + uv_unref(reinterpret_cast(&cleanup_finalization_groups_async_)); thread_stopper()->Install( this, static_cast(this), [](uv_async_t* handle) { @@ -524,6 +533,10 @@ void Environment::RegisterHandleCleanups() { reinterpret_cast(&idle_check_handle_), close_and_finish, nullptr); + RegisterHandleCleanup( + reinterpret_cast(&cleanup_finalization_groups_async_), + close_and_finish, + nullptr); } void Environment::CleanupHandles() { @@ -1040,19 +1053,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 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 index 495d92471a336f..027f5c0bacfba1 100644 --- a/src/env.h +++ b/src/env.h @@ -1128,7 +1128,8 @@ class Environment : public MemoryRetainer { void RunAtExitCallbacks(); void RegisterFinalizationGroupForCleanup(v8::Local 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-error.js b/test/parallel/test-finalization-group-error.js index 0754370f2d4bfa..0685811f55b7f8 100644 --- a/test/parallel/test-finalization-group-error.js +++ b/test/parallel/test-finalization-group-error.js @@ -18,6 +18,10 @@ setTimeout(() => { name: 'Error', message: 'test', }); + + // Give the callbacks scheduled by global.gc() time to run, as the underlying + // uv_async_t is unref’ed. + setTimeout(() => {}, 200); }, 200); process.on('uncaughtException', common.mustCall()); diff --git a/test/parallel/test-finalization-group-regular-gc.js b/test/parallel/test-finalization-group-regular-gc.js new file mode 100644 index 00000000000000..7a4a4797eadcf0 --- /dev/null +++ b/test/parallel/test-finalization-group-regular-gc.js @@ -0,0 +1,25 @@ +// Flags: --harmony-weak-refs +'use strict'; +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(); +});