Skip to content

Commit

Permalink
src: add Cleanable class to Environment
Browse files Browse the repository at this point in the history
We store a linked list of `Cleanable` objects on the
`node::Environment` and invoke their `Clean()` method during env
teardown.

This eliminates the need for adding many cleanup hooks.

PR-URL: #54880
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
  • Loading branch information
gabrielschulhof authored and targos committed Oct 2, 2024
1 parent 124f715 commit 85542b0
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 12 deletions.
5 changes: 5 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,11 @@ void Environment::RunCleanup() {
// Defer the BaseObject cleanup after handles are cleaned up.
CleanupHandles();

while (!cleanable_queue_.IsEmpty()) {
Cleanable* cleanable = cleanable_queue_.PopFront();
cleanable->Clean();
}

while (!cleanup_queue_.empty() || principal_realm_->HasCleanupHooks() ||
native_immediates_.size() > 0 ||
native_immediates_threadsafe_.size() > 0 ||
Expand Down
17 changes: 17 additions & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,18 @@ void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code);
v8::Maybe<ExitCode> SpinEventLoopInternal(Environment* env);
v8::Maybe<ExitCode> EmitProcessExitInternal(Environment* env);

class Cleanable {
public:
virtual ~Cleanable() = default;

protected:
ListNode<Cleanable> cleanable_queue_;

private:
virtual void Clean() = 0;
friend class Environment;
};

/**
* Environment is a per-isolate data structure that represents an execution
* environment. Each environment has a principal realm. An environment can
Expand Down Expand Up @@ -905,8 +917,12 @@ class Environment : public MemoryRetainer {

typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;
typedef ListHead<Cleanable, &Cleanable::cleanable_queue_> CleanableQueue;

inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
inline CleanableQueue* cleanable_queue() {
return &cleanable_queue_;
}
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }

// https://w3c.github.io/hr-time/#dfn-time-origin
Expand Down Expand Up @@ -1182,6 +1198,7 @@ class Environment : public MemoryRetainer {
// memory are predictable. For more information please refer to
// `doc/contributing/node-postmortem-support.md`
friend int GenDebugSymbols();
CleanableQueue cleanable_queue_;
HandleWrapQueue handle_wrap_queue_;
ReqWrapQueue req_wrap_queue_;
std::list<HandleCleanup> handle_cleanup_queue_;
Expand Down
21 changes: 9 additions & 12 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ using v8::Value;

namespace {

class CallbackInfo {
class CallbackInfo : public Cleanable {
public:
static inline Local<ArrayBuffer> CreateTrackedArrayBuffer(
Environment* env,
Expand All @@ -94,7 +94,7 @@ class CallbackInfo {
CallbackInfo& operator=(const CallbackInfo&) = delete;

private:
static void CleanupHook(void* data);
void Clean();
inline void OnBackingStoreFree();
inline void CallAndResetCallback();
inline CallbackInfo(Environment* env,
Expand All @@ -109,7 +109,6 @@ class CallbackInfo {
Environment* const env_;
};


Local<ArrayBuffer> CallbackInfo::CreateTrackedArrayBuffer(
Environment* env,
char* data,
Expand Down Expand Up @@ -149,25 +148,23 @@ CallbackInfo::CallbackInfo(Environment* env,
data_(data),
hint_(hint),
env_(env) {
env->AddCleanupHook(CleanupHook, this);
env->cleanable_queue()->PushFront(this);
env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
}

void CallbackInfo::CleanupHook(void* data) {
CallbackInfo* self = static_cast<CallbackInfo*>(data);

void CallbackInfo::Clean() {
{
HandleScope handle_scope(self->env_->isolate());
Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
HandleScope handle_scope(env_->isolate());
Local<ArrayBuffer> ab = persistent_.Get(env_->isolate());
if (!ab.IsEmpty() && ab->IsDetachable()) {
ab->Detach(Local<Value>()).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() {
Expand All @@ -179,7 +176,7 @@ void CallbackInfo::CallAndResetCallback() {
}
if (callback != nullptr) {
// Clean up all Environment-related state and run the callback.
env_->RemoveCleanupHook(CleanupHook, this);
cleanable_queue_.Remove();
int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);

Expand Down

0 comments on commit 85542b0

Please sign in to comment.