Skip to content

Commit 3c55b46

Browse files
gabrielschulhofRafaelGSS
authored andcommitted
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. This eliminates the need for adding many cleanup hooks. PR-URL: #54880 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
1 parent 5fdab31 commit 3c55b46

File tree

3 files changed

+31
-12
lines changed

3 files changed

+31
-12
lines changed

src/env.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,6 +1290,11 @@ void Environment::RunCleanup() {
12901290
// Defer the BaseObject cleanup after handles are cleaned up.
12911291
CleanupHandles();
12921292

1293+
while (!cleanable_queue_.IsEmpty()) {
1294+
Cleanable* cleanable = cleanable_queue_.PopFront();
1295+
cleanable->Clean();
1296+
}
1297+
12931298
while (!cleanup_queue_.empty() || principal_realm_->HasCleanupHooks() ||
12941299
native_immediates_.size() > 0 ||
12951300
native_immediates_threadsafe_.size() > 0 ||

src/env.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,18 @@ void DefaultProcessExitHandlerInternal(Environment* env, ExitCode exit_code);
598598
v8::Maybe<ExitCode> SpinEventLoopInternal(Environment* env);
599599
v8::Maybe<ExitCode> EmitProcessExitInternal(Environment* env);
600600

601+
class Cleanable {
602+
public:
603+
virtual ~Cleanable() = default;
604+
605+
protected:
606+
ListNode<Cleanable> cleanable_queue_;
607+
608+
private:
609+
virtual void Clean() = 0;
610+
friend class Environment;
611+
};
612+
601613
/**
602614
* Environment is a per-isolate data structure that represents an execution
603615
* environment. Each environment has a principal realm. An environment can
@@ -906,8 +918,12 @@ class Environment final : public MemoryRetainer {
906918

907919
typedef ListHead<HandleWrap, &HandleWrap::handle_wrap_queue_> HandleWrapQueue;
908920
typedef ListHead<ReqWrapBase, &ReqWrapBase::req_wrap_queue_> ReqWrapQueue;
921+
typedef ListHead<Cleanable, &Cleanable::cleanable_queue_> CleanableQueue;
909922

910923
inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; }
924+
inline CleanableQueue* cleanable_queue() {
925+
return &cleanable_queue_;
926+
}
911927
inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; }
912928

913929
// https://w3c.github.io/hr-time/#dfn-time-origin
@@ -1196,6 +1212,7 @@ class Environment final : public MemoryRetainer {
11961212
// memory are predictable. For more information please refer to
11971213
// `doc/contributing/node-postmortem-support.md`
11981214
friend int GenDebugSymbols();
1215+
CleanableQueue cleanable_queue_;
11991216
HandleWrapQueue handle_wrap_queue_;
12001217
ReqWrapQueue req_wrap_queue_;
12011218
std::list<HandleCleanup> handle_cleanup_queue_;

src/node_buffer.cc

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ using v8::Value;
8484

8585
namespace {
8686

87-
class CallbackInfo {
87+
class CallbackInfo : public Cleanable {
8888
public:
8989
static inline Local<ArrayBuffer> CreateTrackedArrayBuffer(
9090
Environment* env,
@@ -97,7 +97,7 @@ class CallbackInfo {
9797
CallbackInfo& operator=(const CallbackInfo&) = delete;
9898

9999
private:
100-
static void CleanupHook(void* data);
100+
void Clean();
101101
inline void OnBackingStoreFree();
102102
inline void CallAndResetCallback();
103103
inline CallbackInfo(Environment* env,
@@ -112,7 +112,6 @@ class CallbackInfo {
112112
Environment* const env_;
113113
};
114114

115-
116115
Local<ArrayBuffer> CallbackInfo::CreateTrackedArrayBuffer(
117116
Environment* env,
118117
char* data,
@@ -152,25 +151,23 @@ CallbackInfo::CallbackInfo(Environment* env,
152151
data_(data),
153152
hint_(hint),
154153
env_(env) {
155-
env->AddCleanupHook(CleanupHook, this);
154+
env->cleanable_queue()->PushFront(this);
156155
env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
157156
}
158157

159-
void CallbackInfo::CleanupHook(void* data) {
160-
CallbackInfo* self = static_cast<CallbackInfo*>(data);
161-
158+
void CallbackInfo::Clean() {
162159
{
163-
HandleScope handle_scope(self->env_->isolate());
164-
Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
160+
HandleScope handle_scope(env_->isolate());
161+
Local<ArrayBuffer> ab = persistent_.Get(env_->isolate());
165162
if (!ab.IsEmpty() && ab->IsDetachable()) {
166163
ab->Detach(Local<Value>()).Check();
167-
self->persistent_.Reset();
164+
persistent_.Reset();
168165
}
169166
}
170167

171168
// Call the callback in this case, but don't delete `this` yet because the
172169
// BackingStore deleter callback will do so later.
173-
self->CallAndResetCallback();
170+
CallAndResetCallback();
174171
}
175172

176173
void CallbackInfo::CallAndResetCallback() {
@@ -182,7 +179,7 @@ void CallbackInfo::CallAndResetCallback() {
182179
}
183180
if (callback != nullptr) {
184181
// Clean up all Environment-related state and run the callback.
185-
env_->RemoveCleanupHook(CleanupHook, this);
182+
cleanable_queue_.Remove();
186183
int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));
187184
env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);
188185

0 commit comments

Comments
 (0)