src: use cleanup hooks to tear down BaseObjects

addaleax · addaleax · commit 9e2554ce9879 · 2018-05-14T19:19:32.000+02:00
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>
diff --git a/src/base_object-inl.h b/src/base_object-inl.h
@@ -37,10 +37,13 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
   CHECK_EQ(false, object.IsEmpty());
   CHECK_GT(object->InternalFieldCount(), 0);
   object->SetAlignedPointerInInternalField(0, static_cast<void*>(this));
+  env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
 }
 
 
 BaseObject::~BaseObject() {
+  env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
+
   if (persistent_handle_.IsEmpty()) {
     // This most likely happened because the weak callback below cleared it.
     return;
@@ -80,6 +83,12 @@ T* BaseObject::FromJSObject(v8::Local<v8::Object> object) {
 }
 
 
+void BaseObject::DeleteMe(void* data) {
+  BaseObject* self = static_cast<BaseObject*>(data);
+  delete self;
+}
+
+
 void BaseObject::MakeWeak() {
   persistent_handle_.SetWeak(
       this,
diff --git a/src/base_object.h b/src/base_object.h
@@ -71,6 +71,8 @@ class BaseObject {
  private:
   BaseObject();
 
+  static inline void DeleteMe(void* data);
+
   // persistent_handle_ needs to be at a fixed offset from the start of the
   // class because it is used by src/node_postmortem_metadata.cc to calculate
   // offsets and generate debug symbols for BaseObject, which assumes that the
diff --git a/src/env.cc b/src/env.cc
@@ -133,6 +133,10 @@ Environment::Environment(IsolateData* isolate_data,
 }
 
 Environment::~Environment() {
+  // Make sure there are no re-used libuv wrapper objects.
+  // CleanupHandles() should have removed all of them.
+  CHECK(file_handle_read_wrap_freelist_.empty());
+
   v8::HandleScope handle_scope(isolate());
 
 #if HAVE_INSPECTOR
@@ -245,6 +249,8 @@ void Environment::CleanupHandles() {
          !handle_wrap_queue_.IsEmpty()) {
     uv_run(event_loop(), UV_RUN_ONCE);
   }
+
+  file_handle_read_wrap_freelist_.clear();
 }
 
 void Environment::StartProfilerIdleNotifier() {
diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -576,6 +576,8 @@ std::unique_ptr<InspectorSession> Agent::Connect(
 
 void Agent::WaitForDisconnect() {
   CHECK_NE(client_, nullptr);
+  // TODO(addaleax): Maybe this should use an at-exit hook for the Environment
+  // or something similar?
   client_->contextDestroyed(parent_env_->context());
   if (io_ != nullptr) {
     io_->WaitForDisconnect();
diff --git a/src/node.cc b/src/node.cc
@@ -4578,12 +4578,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
 
   const int exit_code = EmitExit(&env);
 
+  WaitForInspectorDisconnect(&env);
+
   env.RunCleanup();
   RunAtExit(&env);
 
   v8_platform.DrainVMTasks(isolate);
   v8_platform.CancelVMTasks(isolate);
-  WaitForInspectorDisconnect(&env);
 #if defined(LEAK_SANITIZER)
   __lsan_do_leak_check();
 #endif
diff --git a/src/req_wrap-inl.h b/src/req_wrap-inl.h
@@ -26,7 +26,6 @@ ReqWrap<T>::ReqWrap(Environment* env,
 
 template <typename T>
 ReqWrap<T>::~ReqWrap() {
-  CHECK_EQ(req_.data, this);  // Assert that someone has called Dispatched().
   CHECK_EQ(false, persistent().IsEmpty());
 }
 

Original file line number	Diff line number	Diff line change
`@@ -133,6 +133,10 @@ Environment::Environment(IsolateData* isolate_data,`
`133`	`133`	`}`
`134`	`134`
`135`	`135`	`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`	`+`
`136`	`140`	`v8::HandleScope handle_scope(isolate());`
`137`	`141`
`138`	`142`	`#if HAVE_INSPECTOR`
`@@ -245,6 +249,8 @@ void Environment::CleanupHandles() {`
`245`	`249`	`!handle_wrap_queue_.IsEmpty()) {`
`246`	`250`	`uv_run(event_loop(), UV_RUN_ONCE);`
`247`	`251`	`}`
	`252`	`+`
	`253`	`+ file_handle_read_wrap_freelist_.clear();`
`248`	`254`	`}`
`249`	`255`
`250`	`256`	`void Environment::StartProfilerIdleNotifier() {`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,6 @@ ReqWrap<T>::ReqWrap(Environment* env,`
`26`	`26`
`27`	`27`	`template <typename T>`
`28`	`28`	`ReqWrap<T>::~ReqWrap() {`
`29`		`- CHECK_EQ(req_.data, this); // Assert that someone has called Dispatched().`
`30`	`29`	`CHECK_EQ(false, persistent().IsEmpty());`
`31`	`30`	`}`
`32`	`31`