Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 9e2554c

Browse filesBrowse files
addaleaxaddaleax
committed
src: use cleanup hooks to tear down BaseObjects
Clean up after `BaseObject` instances when the `Environment` is being shut down. This takes care of closing non-libuv resources like `zlib` instances, which do not require asynchronous shutdown. Many thanks for Stephen Belanger, Timothy Gu and Alexey Orlenko for reviewing the original version of this commit in the Ayo.js project. Refs: ayojs/ayo#88 PR-URL: #19377 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 8995408 commit 9e2554c
Copy full SHA for 9e2554c

File tree

Expand file treeCollapse file tree

6 files changed

+21
-2
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

6 files changed

+21
-2
lines changed
Open diff view settings
Collapse file

‎src/base_object-inl.h‎

Copy file name to clipboardExpand all lines: src/base_object-inl.h
+9Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,13 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
3737
CHECK_EQ(false, object.IsEmpty());
3838
CHECK_GT(object->InternalFieldCount(), 0);
3939
object->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
40+
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
4041
}
4142

4243

4344
BaseObject::~BaseObject() {
45+
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
46+
4447
if (persistent_handle_.IsEmpty()) {
4548
// This most likely happened because the weak callback below cleared it.
4649
return;
@@ -80,6 +83,12 @@ T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
8083
}
8184

8285

86+
void BaseObject::DeleteMe(void* data) {
87+
BaseObject* self = static_cast<BaseObject*>(data);
88+
delete self;
89+
}
90+
91+
8392
void BaseObject::MakeWeak() {
8493
persistent_handle_.SetWeak(
8594
this,
Collapse file

‎src/base_object.h‎

Copy file name to clipboardExpand all lines: src/base_object.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,8 @@ class BaseObject {
7171
private:
7272
BaseObject();
7373

74+
static inline void DeleteMe(void* data);
75+
7476
// persistent_handle_ needs to be at a fixed offset from the start of the
7577
// class because it is used by src/node_postmortem_metadata.cc to calculate
7678
// offsets and generate debug symbols for BaseObject, which assumes that the
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,10 @@ Environment::Environment(IsolateData* isolate_data,
133133
}
134134

135135
Environment::~Environment() {
136+
// Make sure there are no re-used libuv wrapper objects.
137+
// CleanupHandles() should have removed all of them.
138+
CHECK(file_handle_read_wrap_freelist_.empty());
139+
136140
v8::HandleScope handle_scope(isolate());
137141

138142
#if HAVE_INSPECTOR
@@ -245,6 +249,8 @@ void Environment::CleanupHandles() {
245249
!handle_wrap_queue_.IsEmpty()) {
246250
uv_run(event_loop(), UV_RUN_ONCE);
247251
}
252+
253+
file_handle_read_wrap_freelist_.clear();
248254
}
249255

250256
void Environment::StartProfilerIdleNotifier() {
Collapse file

‎src/inspector_agent.cc‎

Copy file name to clipboardExpand all lines: src/inspector_agent.cc
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,8 @@ std::unique_ptr<InspectorSession> Agent::Connect(
576576

577577
void Agent::WaitForDisconnect() {
578578
CHECK_NE(client_, nullptr);
579+
// TODO(addaleax): Maybe this should use an at-exit hook for the Environment
580+
// or something similar?
579581
client_->contextDestroyed(parent_env_->context());
580582
if (io_ != nullptr) {
581583
io_->WaitForDisconnect();
Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4578,12 +4578,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
45784578

45794579
const int exit_code = EmitExit(&env);
45804580

4581+
WaitForInspectorDisconnect(&env);
4582+
45814583
env.RunCleanup();
45824584
RunAtExit(&env);
45834585

45844586
v8_platform.DrainVMTasks(isolate);
45854587
v8_platform.CancelVMTasks(isolate);
4586-
WaitForInspectorDisconnect(&env);
45874588
#if defined(LEAK_SANITIZER)
45884589
__lsan_do_leak_check();
45894590
#endif
Collapse file

‎src/req_wrap-inl.h‎

Copy file name to clipboardExpand all lines: src/req_wrap-inl.h
-1Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ ReqWrap<T>::ReqWrap(Environment* env,
2626

2727
template <typename T>
2828
ReqWrap<T>::~ReqWrap() {
29-
CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched().
3029
CHECK_EQ(false, persistent().IsEmpty());
3130
}
3231

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.