From 5df0c78ad77f495ece29fbf0f9ca512a3e9ef0ed Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Tue, 10 Sep 2024 08:18:52 -0700 Subject: [PATCH] src: add Cleanable class to Environment We store a linked list of `Cleanable` objects on the `node::Environment` and invoke their `Clean()` method during env teardown. `Cleanable`s can be added/removed using `Environment::AddCleanable()` resp. `Environment::RemoveCleanable()`. This eliminates the need for adding many cleanup hooks. --- src/env-inl.h | 25 +++++++++++++++++++++++++ src/env.cc | 7 +++++++ src/env.h | 13 +++++++++++++ src/node_buffer.cc | 20 +++++++++----------- 4 files changed, 54 insertions(+), 11 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 08fe98e10b7716..dbb31b02d81915 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -148,6 +148,31 @@ inline void Environment::PopAsyncCallbackScope() { async_callback_scope_depth_--; } +inline void Environment::AddCleanable(Cleanable* new_cleanable) { + CHECK_NOT_NULL(new_cleanable); + new_cleanable->next_ = cleanables_; + new_cleanable->prev_ = nullptr; + if (cleanables_ != nullptr) { + cleanables_->prev_ = new_cleanable; + } + cleanables_ = new_cleanable; +} + +inline void Environment::RemoveCleanable(Cleanable* old_cleanable) { + CHECK_NOT_NULL(old_cleanable); + if (old_cleanable->prev_ != nullptr) { + old_cleanable->prev_->next_ = old_cleanable->next_; + } + if (old_cleanable->next_ != nullptr) { + old_cleanable->next_->prev_ = old_cleanable->prev_; + } + if (cleanables_ == old_cleanable) { + cleanables_ = old_cleanable->next_; + } + old_cleanable->next_ = nullptr; + old_cleanable->prev_ = nullptr; +} + inline AliasedUint32Array& ImmediateInfo::fields() { return fields_; } diff --git a/src/env.cc b/src/env.cc index 4a67c80f1a99bd..cf7461ad9f7500 100644 --- a/src/env.cc +++ b/src/env.cc @@ -804,6 +804,7 @@ Environment::Environment(IsolateData* isolate_data, timeout_info_(isolate_, 1, MAYBE_FIELD_PTR(env_info, timeout_info)), tick_info_(isolate, MAYBE_FIELD_PTR(env_info, tick_info)), timer_base_(uv_now(isolate_data->event_loop())), + cleanables_(nullptr), exec_argv_(exec_args), argv_(args), exec_path_(Environment::GetExecPath(args)), @@ -1279,6 +1280,12 @@ void Environment::RunCleanup() { // Defer the BaseObject cleanup after handles are cleaned up. CleanupHandles(); + while (cleanables_ != nullptr) { + Cleanable* cleanable = cleanables_; + RemoveCleanable(cleanable); + cleanable->Clean(); + } + while (!cleanup_queue_.empty() || principal_realm_->HasCleanupHooks() || native_immediates_.size() > 0 || native_immediates_threadsafe_.size() > 0 || diff --git a/src/env.h b/src/env.h index 772cbb26108665..87d8da7693f6fb 100644 --- a/src/env.h +++ b/src/env.h @@ -594,6 +594,15 @@ void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code); v8::Maybe SpinEventLoopInternal(Environment* env); v8::Maybe EmitProcessExitInternal(Environment* env); +class Cleanable { + friend class Environment; + public: + virtual void Clean() = 0; + private: + Cleanable* next_ = nullptr; + Cleanable* prev_ = nullptr; +}; + /** * Environment is a per-isolate data structure that represents an execution * environment. Each environment has a principal realm. An environment can @@ -611,6 +620,9 @@ class Environment final : public MemoryRetainer { static std::string GetExecPath(const std::vector& argv); static std::string GetCwd(const std::string& exec_path); + void AddCleanable(Cleanable* new_cleanable); + void RemoveCleanable(Cleanable* old_cleanable); + inline size_t SelfSize() const override; bool IsRootNode() const override { return true; } void MemoryInfo(MemoryTracker* tracker) const override; @@ -1080,6 +1092,7 @@ class Environment final : public MemoryRetainer { void TrackContext(v8::Local context); void UntrackContext(v8::Local context); + Cleanable* cleanables_; std::list loaded_addons_; v8::Isolate* const isolate_; IsolateData* const isolate_data_; diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 42bd7b42d398ec..ab4f5db68072af 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -84,7 +84,7 @@ using v8::Value; namespace { -class CallbackInfo { +class CallbackInfo: public Cleanable { public: static inline Local CreateTrackedArrayBuffer( Environment* env, @@ -95,9 +95,9 @@ class CallbackInfo { CallbackInfo(const CallbackInfo&) = delete; CallbackInfo& operator=(const CallbackInfo&) = delete; + void Clean(); private: - static void CleanupHook(void* data); inline void OnBackingStoreFree(); inline void CallAndResetCallback(); inline CallbackInfo(Environment* env, @@ -152,25 +152,23 @@ CallbackInfo::CallbackInfo(Environment* env, data_(data), hint_(hint), env_(env) { - env->AddCleanupHook(CleanupHook, this); + env->AddCleanable(this); env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this)); } -void CallbackInfo::CleanupHook(void* data) { - CallbackInfo* self = static_cast(data); - +void CallbackInfo::Clean() { { - HandleScope handle_scope(self->env_->isolate()); - Local ab = self->persistent_.Get(self->env_->isolate()); + HandleScope handle_scope(env_->isolate()); + Local ab = persistent_.Get(env_->isolate()); if (!ab.IsEmpty() && ab->IsDetachable()) { ab->Detach(Local()).Check(); - self->persistent_.Reset(); + persistent_.Reset(); } } // Call the callback in this case, but don't delete `this` yet because the // BackingStore deleter callback will do so later. - self->CallAndResetCallback(); + CallAndResetCallback(); } void CallbackInfo::CallAndResetCallback() { @@ -182,7 +180,7 @@ void CallbackInfo::CallAndResetCallback() { } if (callback != nullptr) { // Clean up all Environment-related state and run the callback. - env_->RemoveCleanupHook(CleanupHook, this); + env_->RemoveCleanable(this); int64_t change_in_bytes = -static_cast(sizeof(*this)); env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);