From 545f7282d126ee43cf9cfeb66c83d0cbd2c60614 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 11 Oct 2019 15:53:41 -0700 Subject: [PATCH] src: implement v8 host weakref hooks PR-URL: https://github.com/nodejs/node/pull/29874 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis --- .eslintrc.js | 1 + src/api/callback.cc | 3 +++ src/api/environment.cc | 12 ++++++++++ src/env-inl.h | 5 ++++ src/env.cc | 16 +++++++++++++ src/env.h | 5 ++++ src/node_task_queue.cc | 2 ++ test/.eslintrc.yaml | 1 + .../parallel/test-finalization-group-error.js | 23 +++++++++++++++++++ test/parallel/test-finalization-group.js | 13 +++++++++++ test/parallel/test-weakref.js | 13 +++++++++++ 11 files changed, 94 insertions(+) create mode 100644 test/parallel/test-finalization-group-error.js create mode 100644 test/parallel/test-finalization-group.js create mode 100644 test/parallel/test-weakref.js diff --git a/.eslintrc.js b/.eslintrc.js index cd59fd83394424..8bdf2ad09ae88c 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -317,5 +317,6 @@ module.exports = { TextEncoder: 'readable', TextDecoder: 'readable', queueMicrotask: 'readable', + globalThis: 'readable', }, }; diff --git a/src/api/callback.cc b/src/api/callback.cc index e6098d5921a038..6d4e28e1d9070f 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -100,6 +100,9 @@ void InternalCallbackScope::Close() { TickInfo* tick_info = env_->tick_info(); if (!env_->can_call_into_js()) return; + + OnScopeLeave weakref_cleanup([&]() { env_->RunWeakRefCleanup(); }); + if (!tick_info->has_tick_scheduled()) { MicrotasksScope::PerformCheckpoint(env_->isolate()); } diff --git a/src/api/environment.cc b/src/api/environment.cc index 2c0fe1306319b2..3c5665a2d22623 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -12,6 +12,7 @@ using errors::TryCatchScope; using v8::Array; using v8::Context; using v8::EscapableHandleScope; +using v8::FinalizationGroup; using v8::Function; using v8::HandleScope; using v8::Isolate; @@ -76,6 +77,15 @@ static MaybeLocal PrepareStackTraceCallback(Local context, return result; } +static void HostCleanupFinalizationGroupCallback( + Local context, Local group) { + Environment* env = Environment::GetCurrent(context); + if (env == nullptr) { + return; + } + env->RegisterFinalizationGroupForCleanup(group); +} + void* NodeArrayBufferAllocator::Allocate(size_t size) { if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers) return UncheckedCalloc(size); @@ -203,6 +213,8 @@ void SetIsolateUpForNode(v8::Isolate* isolate, IsolateSettingCategories cat) { isolate->SetAllowWasmCodeGenerationCallback( AllowWasmCodeGenerationCallback); isolate->SetPromiseRejectCallback(task_queue::PromiseRejectCallback); + isolate->SetHostCleanupFinalizationGroupCallback( + HostCleanupFinalizationGroupCallback); v8::CpuProfiler::UseDetailedSourcePositionsForProfiling(isolate); break; default: diff --git a/src/env-inl.h b/src/env-inl.h index a2ab98d66db10d..d61ae8f8df1d17 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -1115,6 +1115,11 @@ void Environment::RemoveCleanupHook(void (*fn)(void*), void* arg) { cleanup_hooks_.erase(search); } +inline void Environment::RegisterFinalizationGroupForCleanup( + v8::Local group) { + cleanup_finalization_groups_.emplace_back(isolate(), group); +} + size_t CleanupHookCallback::Hash::operator()( const CleanupHookCallback& cb) const { return std::hash()(cb.arg_); diff --git a/src/env.cc b/src/env.cc index 2400785ea82fe4..ff1868e75ecd5a 100644 --- a/src/env.cc +++ b/src/env.cc @@ -29,6 +29,7 @@ using v8::ArrayBuffer; using v8::Boolean; using v8::Context; using v8::EmbedderGraph; +using v8::FinalizationGroup; using v8::Function; using v8::FunctionTemplate; using v8::HandleScope; @@ -1051,6 +1052,21 @@ void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose( keep_alive_allocators_->insert(allocator); } +bool Environment::RunWeakRefCleanup() { + isolate()->ClearKeptObjects(); + + while (!cleanup_finalization_groups_.empty()) { + Local 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; diff --git a/src/env.h b/src/env.h index df0df0b99ca87a..46aecfd2521260 100644 --- a/src/env.h +++ b/src/env.h @@ -1129,6 +1129,9 @@ class Environment : public MemoryRetainer { void AtExit(void (*cb)(void* arg), void* arg); void RunAtExitCallbacks(); + void RegisterFinalizationGroupForCleanup(v8::Local 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) @@ -1335,6 +1338,8 @@ class Environment : public MemoryRetainer { uint64_t thread_id_; std::unordered_set sub_worker_contexts_; + std::deque> cleanup_finalization_groups_; + static void* const kNodeContextTagPtr; static int const kNodeContextTag; diff --git a/src/node_task_queue.cc b/src/node_task_queue.cc index 3b00cbc600fcbd..ef1aff6cd4138d 100644 --- a/src/node_task_queue.cc +++ b/src/node_task_queue.cc @@ -43,6 +43,8 @@ static void EnqueueMicrotask(const FunctionCallbackInfo& args) { // Should be in sync with runNextTicks in internal/process/task_queues.js bool RunNextTicksNative(Environment* env) { + OnScopeLeave weakref_cleanup([&]() { env->RunWeakRefCleanup(); }); + TickInfo* tick_info = env->tick_info(); if (!tick_info->has_tick_scheduled() && !tick_info->has_rejection_to_warn()) MicrotasksScope::PerformCheckpoint(env->isolate()); diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index 8a36ac522acda4..1dd006566a8c8d 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -34,3 +34,4 @@ globals: BigInt64Array: false BigUint64Array: false SharedArrayBuffer: false + globalThis: false diff --git a/test/parallel/test-finalization-group-error.js b/test/parallel/test-finalization-group-error.js new file mode 100644 index 00000000000000..0754370f2d4bfa --- /dev/null +++ b/test/parallel/test-finalization-group-error.js @@ -0,0 +1,23 @@ +'use strict'; + +// Flags: --expose-gc --harmony-weak-refs + +const common = require('../common'); +const assert = require('assert'); + +const g = new globalThis.FinalizationGroup(common.mustCallAtLeast(() => { + throw new Error('test'); +}, 1)); +g.register({}, 42); + +setTimeout(() => { + globalThis.gc(); + assert.throws(() => { + g.cleanupSome(); + }, { + name: 'Error', + message: 'test', + }); +}, 200); + +process.on('uncaughtException', common.mustCall()); diff --git a/test/parallel/test-finalization-group.js b/test/parallel/test-finalization-group.js new file mode 100644 index 00000000000000..3e6b9d72e35648 --- /dev/null +++ b/test/parallel/test-finalization-group.js @@ -0,0 +1,13 @@ +'use strict'; + +// Flags: --expose-gc --harmony-weak-refs + +const common = require('../common'); + +const g = new globalThis.FinalizationGroup(common.mustCallAtLeast(1)); +g.register({}, 42); + +setTimeout(() => { + globalThis.gc(); + g.cleanupSome(); +}, 200); diff --git a/test/parallel/test-weakref.js b/test/parallel/test-weakref.js new file mode 100644 index 00000000000000..9dac8463760dac --- /dev/null +++ b/test/parallel/test-weakref.js @@ -0,0 +1,13 @@ +'use strict'; + +// Flags: --expose-gc --harmony-weak-refs + +require('../common'); +const assert = require('assert'); + +const w = new globalThis.WeakRef({}); + +setTimeout(() => { + globalThis.gc(); + assert.strictEqual(w.deref(), undefined); +}, 200);