src: make CleanupHandles() tear down handles/reqs

Previously, handles would not be closed when the current `Environment`
stopped, which is acceptable in a single-`Environment`-per-process
situation, but would otherwise create memory and file descriptor
leaks.

Also, introduce a generic way to close handles via the
`Environment::CloseHandle()` function, which automatically keeps
track of whether a close callback has been called yet or not.

Many thanks for Stephen Belanger for reviewing the original version of
this commit in the Ayo.js project.

Refs: https://github.com/ayojs/ayo/pull/85
PR-URL: https://github.com/nodejs/node/pull/19377
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

src/cares_wrap.cc

@@ -267,9 +267,8 @@ void ares_poll_cb(uv_poll_t* watcher, int status, int events) {
 }
 
 
-void ares_poll_close_cb(uv_handle_t* watcher) {
+void ares_poll_close_cb(uv_poll_t* watcher) {
-  node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher,
+  node_ares_task* task = ContainerOf(&node_ares_task::poll_watcher, watcher);
-                                  reinterpret_cast<uv_poll_t*>(watcher));
   free(task);
 }
 
@@ -347,8 +346,7 @@ void ares_sockstate_cb(void* data,
           "When an ares socket is closed we should have a handle for it");
 
     channel->task_list()->erase(it);
-    uv_close(reinterpret_cast<uv_handle_t*>(&task->poll_watcher),
+    channel->env()->CloseHandle(&task->poll_watcher, ares_poll_close_cb);
-             ares_poll_close_cb);
 
     if (channel->task_list()->empty()) {
       uv_timer_stop(channel->timer_handle());
@@ -517,10 +515,7 @@ ChannelWrap::~ChannelWrap() {
 void ChannelWrap::CleanupTimer() {
   if (timer_handle_ == nullptr) return;
 
-  uv_close(reinterpret_cast<uv_handle_t*>(timer_handle_),
+  env()->CloseHandle(timer_handle_, [](uv_timer_t* handle){ delete handle; });
-           [](uv_handle_t* handle) {
-    delete reinterpret_cast<uv_timer_t*>(handle);
-  });
   timer_handle_ = nullptr;
 }
 
@@ -610,8 +605,7 @@ class QueryWrap : public AsyncWrap {
                static_cast<void*>(this));
   }
 
-  static void CaresAsyncClose(uv_handle_t* handle) {
+  static void CaresAsyncClose(uv_async_t* async) {
-    uv_async_t* async = reinterpret_cast<uv_async_t*>(handle);
     auto data = static_cast<struct CaresAsyncData*>(async->data);
     delete data->wrap;
     delete data;
@@ -636,7 +630,7 @@ class QueryWrap : public AsyncWrap {
       free(host);
     }
 
-    uv_close(reinterpret_cast<uv_handle_t*>(handle), CaresAsyncClose);
+    wrap->env()->CloseHandle(handle, CaresAsyncClose);
   }
 
   static void Callback(void *arg, int status, int timeouts,

src/connection_wrap.h

@@ -23,8 +23,6 @@ class ConnectionWrap : public LibuvStreamWrap {
   ConnectionWrap(Environment* env,
                  v8::Local<v8::Object> object,
                  ProviderType provider);
-  ~ConnectionWrap() {
-  }
 
   UVType handle_;
 };

src/env-inl.h

@@ -349,8 +349,26 @@ inline void Environment::RegisterHandleCleanup(uv_handle_t* handle,
   handle_cleanup_queue_.push_back(HandleCleanup{handle, cb, arg});
 }
 
-inline void Environment::FinishHandleCleanup(uv_handle_t* handle) {
+template <typename T, typename OnCloseCallback>
-  handle_cleanup_waiting_--;
+inline void Environment::CloseHandle(T* handle, OnCloseCallback callback) {
+  handle_cleanup_waiting_++;
+  static_assert(sizeof(T) >= sizeof(uv_handle_t), "T is a libuv handle");
+  static_assert(offsetof(T, data) == offsetof(uv_handle_t, data),
+                "T is a libuv handle");
+  static_assert(offsetof(T, close_cb) == offsetof(uv_handle_t, close_cb),
+                "T is a libuv handle");
+  struct CloseData {
+    Environment* env;
+    OnCloseCallback callback;
+    void* original_data;
+  };
+  handle->data = new CloseData { this, callback, handle->data };
+  uv_close(reinterpret_cast<uv_handle_t*>(handle), [](uv_handle_t* handle) {
+    std::unique_ptr<CloseData> data { static_cast<CloseData*>(handle->data) };
+    data->env->handle_cleanup_waiting_--;
+    handle->data = data->original_data;
+    data->callback(reinterpret_cast<T*>(handle));
+  });
 }
 
 inline uv_loop_t* Environment::event_loop() const {

src/env.cc

@@ -209,9 +209,7 @@ void Environment::RegisterHandleCleanups() {
                                         void* arg) {
     handle->data = env;
 
-    uv_close(handle, [](uv_handle_t* handle) {
+    env->CloseHandle(handle, [](uv_handle_t* handle) {});
-      static_cast<Environment*>(handle->data)->FinishHandleCleanup(handle);
-    });
   };
 
   RegisterHandleCleanup(
@@ -233,13 +231,17 @@ void Environment::RegisterHandleCleanups() {
 }
 
 void Environment::CleanupHandles() {
-  for (HandleCleanup& hc : handle_cleanup_queue_) {
+  for (ReqWrap<uv_req_t>* request : req_wrap_queue_)
-    handle_cleanup_waiting_++;
+    request->Cancel();
+
+  for (HandleWrap* handle : handle_wrap_queue_)
+    handle->Close();
+
+  for (HandleCleanup& hc : handle_cleanup_queue_)
     hc.cb_(this, hc.handle_, hc.arg_);
-  }
   handle_cleanup_queue_.clear();
 
-  while (handle_cleanup_waiting_ != 0)
+  while (handle_cleanup_waiting_ != 0 || !handle_wrap_queue_.IsEmpty())
     uv_run(event_loop(), UV_RUN_ONCE);
 }
 
@@ -306,6 +308,8 @@ void Environment::PrintSyncTrace() const {
 }
 
 void Environment::RunCleanup() {
+  CleanupHandles();
+
   while (!cleanup_hooks_.empty()) {
     // Copy into a vector, since we can't sort an unordered_set in-place.
     std::vector<CleanupHookCallback> callbacks(
@@ -329,8 +333,8 @@ void Environment::RunCleanup() {
 
       cb.fn_(cb.arg_);
       cleanup_hooks_.erase(cb);
-      CleanupHandles();
     }
+    CleanupHandles();
   }
 }
 

src/env.h

@@ -577,10 +577,14 @@ class Environment {
 
   void RegisterHandleCleanups();
   void CleanupHandles();
+
+  // Register clean-up cb to be called on environment destruction.
   inline void RegisterHandleCleanup(uv_handle_t* handle,
                                     HandleCleanupCb cb,
                                     void *arg);
-  inline void FinishHandleCleanup(uv_handle_t* handle);
+
+  template <typename T, typename OnCloseCallback>
+  inline void CloseHandle(T* handle, OnCloseCallback callback);
 
   inline void AssignToContext(v8::Local<v8::Context> context,
                               const ContextInfo& info);

src/fs_event_wrap.cc

@@ -78,11 +78,12 @@ FSEventWrap::FSEventWrap(Environment* env, Local<Object> object)
     : HandleWrap(env,
                  object,
                  reinterpret_cast<uv_handle_t*>(&handle_),
-                 AsyncWrap::PROVIDER_FSEVENTWRAP) {}
+                 AsyncWrap::PROVIDER_FSEVENTWRAP) {
+  MarkAsUninitialized();
+}
 
 
 FSEventWrap::~FSEventWrap() {
-  CHECK_EQ(initialized_, false);
 }
 
 void FSEventWrap::GetInitialized(const FunctionCallbackInfo<Value>& args) {
@@ -153,6 +154,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {
   }
 
   err = uv_fs_event_start(&wrap->handle_, OnEvent, *path, flags);
+  wrap->MarkAsInitialized();
   wrap->initialized_ = true;
 
   if (err != 0) {

src/handle_wrap.cc

@@ -61,29 +61,40 @@ void HandleWrap::HasRef(const FunctionCallbackInfo<Value>& args) {
 
 
 void HandleWrap::Close(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-
   HandleWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
 
-  // Guard against uninitialized handle or double close.
+  wrap->Close(args[0]);
-  if (!IsAlive(wrap))
+}
+
+void HandleWrap::Close(v8::Local<v8::Value> close_callback) {
+  if (state_ != kInitialized)
     return;
 
-  if (wrap->state_ != kInitialized)
+  CHECK_EQ(false, persistent().IsEmpty());
-    return;
+  uv_close(handle_, OnClose);
+  state_ = kClosing;
 
-  CHECK_EQ(false, wrap->persistent().IsEmpty());
+  if (!close_callback.IsEmpty() && close_callback->IsFunction()) {
-  uv_close(wrap->handle_, OnClose);
+    object()->Set(env()->context(), env()->onclose_string(), close_callback)
-  wrap->state_ = kClosing;
+        .FromJust();
-
+    state_ = kClosingWithCallback;
-  if (args[0]->IsFunction()) {
-    wrap->object()->Set(env->onclose_string(), args[0]);
-    wrap->state_ = kClosingWithCallback;
   }
 }
 
 
+void HandleWrap::MarkAsInitialized() {
+  env()->handle_wrap_queue()->PushBack(this);
+  state_ = kInitialized;
+}
+
+
+void HandleWrap::MarkAsUninitialized() {
+  handle_wrap_queue_.Remove();
+  state_ = kClosed;
+}
+
+
 HandleWrap::HandleWrap(Environment* env,
                        Local<Object> object,
                        uv_handle_t* handle,
@@ -110,6 +121,8 @@ void HandleWrap::OnClose(uv_handle_t* handle) {
   const bool have_close_callback = (wrap->state_ == kClosingWithCallback);
   wrap->state_ = kClosed;
 
+  wrap->OnClose();
+
   if (have_close_callback)
     wrap->MakeCallback(env->onclose_string(), 0, nullptr);
 

src/handle_wrap.h

@@ -70,11 +70,17 @@ class HandleWrap : public AsyncWrap {
 
   inline uv_handle_t* GetHandle() const { return handle_; }
 
+  void Close(v8::Local<v8::Value> close_callback = v8::Local<v8::Value>());
+
  protected:
   HandleWrap(Environment* env,
              v8::Local<v8::Object> object,
              uv_handle_t* handle,
              AsyncWrap::ProviderType provider);
+  virtual void OnClose() {}
+
+  void MarkAsInitialized();
+  void MarkAsUninitialized();
 
  private:
   friend class Environment;

src/node_stat_watcher.cc

@@ -75,11 +75,6 @@ void StatWatcher::Initialize(Environment* env, Local<Object> target) {
 }
 
 
-static void Delete(uv_handle_t* handle) {
-  delete reinterpret_cast<uv_fs_poll_t*>(handle);
-}
-
-
 StatWatcher::StatWatcher(Environment* env, Local<Object> wrap)
     : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_STATWATCHER),
       watcher_(new uv_fs_poll_t) {
@@ -93,7 +88,7 @@ StatWatcher::~StatWatcher() {
   if (IsActive()) {
     Stop();
   }
-  uv_close(reinterpret_cast<uv_handle_t*>(watcher_), Delete);
+  env()->CloseHandle(watcher_, [](uv_fs_poll_t* handle) { delete handle; });
 }
 
 

src/process_wrap.cc

@@ -88,6 +88,7 @@ class ProcessWrap : public HandleWrap {
                    object,
                    reinterpret_cast<uv_handle_t*>(&process_),
                    AsyncWrap::PROVIDER_PROCESSWRAP) {
+    MarkAsUninitialized();
   }
 
   static void ParseStdioOptions(Environment* env,
@@ -256,6 +257,7 @@ class ProcessWrap : public HandleWrap {
     }
 
     int err = uv_spawn(env->event_loop(), &wrap->process_, &options);
+    wrap->MarkAsInitialized();
 
     if (err == 0) {
       CHECK_EQ(wrap->process_.data, wrap);

src/req_wrap-inl.h

@@ -7,6 +7,7 @@
 #include "async_wrap-inl.h"
 #include "env-inl.h"
 #include "util-inl.h"
+#include "uv.h"
 
 namespace node {
 
@@ -37,6 +38,11 @@ ReqWrap<T>* ReqWrap<T>::from_req(T* req) {
   return ContainerOf(&ReqWrap<T>::req_, req);
 }
 
+template <typename T>
+void ReqWrap<T>::Cancel() {
+  uv_cancel(reinterpret_cast<uv_req_t*>(&req_));
+}
+
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/req_wrap.h

@@ -19,6 +19,7 @@ class ReqWrap : public AsyncWrap {
   inline ~ReqWrap() override;
   inline void Dispatched();  // Call this after the req has been dispatched.
   T* req() { return &req_; }
+  inline void Cancel();
 
   static ReqWrap* from_req(T* req);
 

src/tty_wrap.cc

@@ -172,6 +172,8 @@ TTYWrap::TTYWrap(Environment* env,
                       reinterpret_cast<uv_stream_t*>(&handle_),
                       AsyncWrap::PROVIDER_TTYWRAP) {
   *init_err = uv_tty_init(env->event_loop(), &handle_, fd, readable);
+  if (*init_err != 0)
+    MarkAsUninitialized();
 }
 
 }  // namespace node