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: https://github.com/ayojs/ayo/pull/88
PR-URL: https://github.com/nodejs/node/pull/19377
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

6 files changed, 21 insertions, 2 deletions

diff --git a/src/base_object-inl.h b/src/base_object-inl.h
index 786e1f26b4..3bd854639b 100644
--- 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
index 2a4967c1aa..e0b6084340 100644
--- 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
index e5b9c0fd6a..ab5de3e2ee 100644
--- 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
index 4e0c04a7b9..391d3bc037 100644
--- 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
index 06acd1c74b..70787f2f2f 100644
--- 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
index e3b26c1f5c..7e9e2d9fbb 100644
--- 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());
 }