async_wrap: run destroy in uv_timer_t

Calling the destroy callbacks in a uv_idle_t causes a timing issue where
if a handle or request is closed then the class isn't deleted until
uv_close() callbacks are called (which happens after the poll phase).
This results in some destroy callbacks not being called just before the
application exits. So instead switch the destroy callbacks to be called
in a uv_timer_t with the timeout set to zero.

When uv_run() is called with UV_RUN_ONCE the final operation of the
event loop is to process all remaining timers. By setting the timeout to
zero it results in the destroy callbacks being processed after
uv_close() but before uv_run() returned. Processing the destroyed ids
that were previously missed.

Also, process the destroy_ids_list() in a do {} while() loop that makes
sure the vector is empty before returning. Which also makes running
clear() unnecessary.

Fixes: https://github.com/nodejs/node/issues/13262
PR-URL: https://github.com/nodejs/node/pull/13369
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

src/async-wrap.cc

@@ -138,10 +138,8 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) {
 // end RetainedAsyncInfo
 
 
-static void DestroyIdsCb(uv_idle_t* handle) {
-  uv_idle_stop(handle);
-
-  Environment* env = Environment::from_destroy_ids_idle_handle(handle);
+static void DestroyIdsCb(uv_timer_t* handle) {
+  Environment* env = Environment::from_destroy_ids_timer_handle(handle);
 
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
@@ -149,23 +147,23 @@ static void DestroyIdsCb(uv_idle_t* handle) {
 
   TryCatch try_catch(env->isolate());
 
-  std::vector<double> destroy_ids_list;
-  destroy_ids_list.swap(*env->destroy_ids_list());
-  for (auto current_id : destroy_ids_list) {
-    // Want each callback to be cleaned up after itself, instead of cleaning
-    // them all up after the while() loop completes.
-    HandleScope scope(env->isolate());
-    Local<Value> argv = Number::New(env->isolate(), current_id);
-    MaybeLocal<Value> ret = fn->Call(
-        env->context(), Undefined(env->isolate()), 1, &argv);
-
-    if (ret.IsEmpty()) {
-      ClearFatalExceptionHandlers(env);
-      FatalException(env->isolate(), try_catch);
+  do {
+    std::vector<double> destroy_ids_list;
+    destroy_ids_list.swap(*env->destroy_ids_list());
+    for (auto current_id : destroy_ids_list) {
+      // Want each callback to be cleaned up after itself, instead of cleaning
+      // them all up after the while() loop completes.
+      HandleScope scope(env->isolate());
+      Local<Value> argv = Number::New(env->isolate(), current_id);
+      MaybeLocal<Value> ret = fn->Call(
+          env->context(), Undefined(env->isolate()), 1, &argv);
+
+      if (ret.IsEmpty()) {
+        ClearFatalExceptionHandlers(env);
+        FatalException(env->isolate(), try_catch);
+      }
     }
-  }
-
-  env->destroy_ids_list()->clear();
+  } while (!env->destroy_ids_list()->empty());
 }
 
 
@@ -174,7 +172,7 @@ static void PushBackDestroyId(Environment* env, double id) {
     return;
 
   if (env->destroy_ids_list()->empty())
-    uv_idle_start(env->destroy_ids_idle_handle(), DestroyIdsCb);
+    uv_timer_start(env->destroy_ids_timer_handle(), DestroyIdsCb, 0, 0);
 
   env->destroy_ids_list()->push_back(id);
 }

src/env-inl.h

@@ -356,13 +356,13 @@ inline uv_idle_t* Environment::immediate_idle_handle() {
   return &immediate_idle_handle_;
 }
 
-inline Environment* Environment::from_destroy_ids_idle_handle(
-    uv_idle_t* handle) {
-  return ContainerOf(&Environment::destroy_ids_idle_handle_, handle);
+inline Environment* Environment::from_destroy_ids_timer_handle(
+    uv_timer_t* handle) {
+  return ContainerOf(&Environment::destroy_ids_timer_handle_, handle);
 }
 
-inline uv_idle_t* Environment::destroy_ids_idle_handle() {
-  return &destroy_ids_idle_handle_;
+inline uv_timer_t* Environment::destroy_ids_timer_handle() {
+  return &destroy_ids_timer_handle_;
 }
 
 inline void Environment::RegisterHandleCleanup(uv_handle_t* handle,

src/env.cc

@@ -49,8 +49,7 @@ void Environment::Start(int argc,
   uv_unref(reinterpret_cast<uv_handle_t*>(&idle_prepare_handle_));
   uv_unref(reinterpret_cast<uv_handle_t*>(&idle_check_handle_));
 
-  uv_idle_init(event_loop(), destroy_ids_idle_handle());
-  uv_unref(reinterpret_cast<uv_handle_t*>(destroy_ids_idle_handle()));
+  uv_timer_init(event_loop(), destroy_ids_timer_handle());
 
   auto close_and_finish = [](Environment* env, uv_handle_t* handle, void* arg) {
     handle->data = env;
@@ -102,16 +101,6 @@ void Environment::CleanupHandles() {
   while (handle_cleanup_waiting_ != 0)
     uv_run(event_loop(), UV_RUN_ONCE);
 
-  // Closing the destroy_ids_idle_handle_ within the handle cleanup queue
-  // prevents the async wrap destroy hook from being called.
-  uv_handle_t* handle =
-    reinterpret_cast<uv_handle_t*>(&destroy_ids_idle_handle_);
-  handle->data = this;
-  handle_cleanup_waiting_ = 1;
-  uv_close(handle, [](uv_handle_t* handle) {
-    static_cast<Environment*>(handle->data)->FinishHandleCleanup(handle);
-  });
-
   while (handle_cleanup_waiting_ != 0)
     uv_run(event_loop(), UV_RUN_ONCE);
 }

src/env.h

@@ -526,10 +526,10 @@ class Environment {
   inline uint32_t watched_providers() const;
 
   static inline Environment* from_immediate_check_handle(uv_check_t* handle);
-  static inline Environment* from_destroy_ids_idle_handle(uv_idle_t* handle);
+  static inline Environment* from_destroy_ids_timer_handle(uv_timer_t* handle);
   inline uv_check_t* immediate_check_handle();
   inline uv_idle_t* immediate_idle_handle();
-  inline uv_idle_t* destroy_ids_idle_handle();
+  inline uv_timer_t* destroy_ids_timer_handle();
 
   // Register clean-up cb to be called on environment destruction.
   inline void RegisterHandleCleanup(uv_handle_t* handle,
@@ -662,7 +662,7 @@ class Environment {
   IsolateData* const isolate_data_;
   uv_check_t immediate_check_handle_;
   uv_idle_t immediate_idle_handle_;
-  uv_idle_t destroy_ids_idle_handle_;
+  uv_timer_t destroy_ids_timer_handle_;
   uv_prepare_t idle_prepare_handle_;
   uv_check_t idle_check_handle_;
   AsyncHooks async_hooks_;

test/async-hooks/test-writewrap.js

@@ -53,7 +53,7 @@ function checkDestroyedWriteWraps(n, stage) {
     assert.strictEqual(typeof w.uid, 'number', 'uid is a number');
     assert.strictEqual(typeof w.triggerId, 'number', 'triggerId is a number');
 
-    checkInvocations(w, { init: 1, destroy: 1 }, 'when ' + stage);
+    checkInvocations(w, { init: 1 }, 'when ' + stage);
   }
   as.forEach(checkValidWriteWrap);
 }