[async_wrap] call destroy hooks on uv_timer_t

src/async-wrap.cc

@@ -138,34 +138,32 @@ 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());
   Local<Function> fn = env->async_hooks_destroy_function();
 
   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);
 }

test/parallel/test-async-hooks-close-during-destroy.js

@@ -0,0 +1,37 @@
+'use strict';
+// Test that async ids that are added to the destroy queue while running a
+// `destroy` callback are handled correctly.
+
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+
+const initCalls = new Set();
+let destroyResCallCount = 0;
+let res2;
+
+async_hooks.createHook({
+  init: common.mustCallAtLeast((id, provider, triggerId) => {
+    if (provider === 'foobar')
+      initCalls.add(id);
+  }, 2),
+  destroy: common.mustCallAtLeast((id) => {
+    if (!initCalls.has(id)) return;
+
+    switch (destroyResCallCount++) {
+      case 0:
+        // Trigger the second `destroy` call.
+        res2.emitDestroy();
+        break;
+      case 2:
+        assert.fail('More than 2 destroy() invocations');
+        break;
+    }
+  }, 2)
+}).enable();
+
+const res1 = new async_hooks.AsyncResource('foobar');
+res2 = new async_hooks.AsyncResource('foobar');
+res1.emitDestroy();
+
+process.on('exit', () => assert.strictEqual(destroyResCallCount, 2));

test/parallel/test-async-hooks-top-level-clearimmediate.js

@@ -0,0 +1,27 @@
+'use strict';
+
+// Regression test for https://github.com/nodejs/node/issues/13262
+
+const common = require('../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+
+let seenId, seenResource;
+
+async_hooks.createHook({
+  init: common.mustCall((id, provider, triggerId, resource) => {
+    seenId = id;
+    seenResource = resource;
+    assert.strictEqual(provider, 'Immediate');
+    assert.strictEqual(triggerId, 1);
+  }),
+  before: common.mustNotCall(),
+  after: common.mustNotCall(),
+  destroy: common.mustCall((id) => {
+    assert.strictEqual(seenId, id);
+  })
+}).enable();
+
+const immediate = setImmediate(common.mustNotCall());
+assert.strictEqual(immediate, seenResource);
+clearImmediate(immediate);