async_wrap: call destroy() callback in uv_idle_t

Calling JS during GC is a no-no. So intead create a queue of all ids that need to have their destroy() callback called and call them later. Removed checking destroy() in test-async-wrap-uid because destroy() can be called after the 'exit' callback. Missing a reliable test to reproduce the issue that caused the FATAL_ERROR. PR-URL: nodejs#9753 Fixes: nodejs#8216 Fixes: nodejs#9465 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
jmdarling · Dec 8, 2016 · d479826 · d479826
1 parent 03c1127
commit d479826
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 19 deletions.
diff --git a/src/async-wrap.cc b/src/async-wrap.cc
@@ -5,6 +5,7 @@
 #include "util.h"
 #include "util-inl.h"
 
+#include "uv.h"
 #include "v8.h"
 #include "v8-profiler.h"
 
@@ -182,6 +183,38 @@ void AsyncWrap::Initialize(Local<Object> target,
 }
 
 
+void AsyncWrap::DestroyIdsCb(uv_idle_t* handle) {
+  uv_idle_stop(handle);
+
+  Environment* env = Environment::from_destroy_ids_idle_handle(handle);
+  // None of the V8 calls done outside the HandleScope leak a handle. If this
+  // changes in the future then the SealHandleScope wrapping the uv_run()
+  // will catch this can cause the process to abort.
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
+  Local<Function> fn = env->async_hooks_destroy_function();
+
+  if (fn.IsEmpty())
+    return env->destroy_ids_list()->clear();
+
+  TryCatch try_catch(env->isolate());
+
+  for (auto current_id : *env->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);
+    }
+  }
+}
+
+
 void LoadAsyncWrapperInfo(Environment* env) {
   HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler();
 #define V(PROVIDER)                                                           \
@@ -248,18 +281,10 @@ AsyncWrap::~AsyncWrap() {
   if (!ran_init_callback())
     return;
 
-  Local<Function> fn = env()->async_hooks_destroy_function();
-  if (!fn.IsEmpty()) {
-    HandleScope scope(env()->isolate());
-    Local<Value> uid = Number::New(env()->isolate(), get_uid());
-    TryCatch try_catch(env()->isolate());
-    MaybeLocal<Value> ret =
-        fn->Call(env()->context(), Null(env()->isolate()), 1, &uid);
-    if (ret.IsEmpty()) {
-      ClearFatalExceptionHandlers(env());
-      FatalException(env()->isolate(), try_catch);
-    }
-  }
+  if (env()->destroy_ids_list()->empty())
+    uv_idle_start(env()->destroy_ids_idle_handle(), DestroyIdsCb);
+
+  env()->destroy_ids_list()->push_back(get_uid());
 }
 
 

diff --git a/src/async-wrap.h b/src/async-wrap.h
@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "base-object.h"
+#include "uv.h"
 #include "v8.h"
 
 #include <stdint.h>
@@ -60,6 +61,8 @@ class AsyncWrap : public BaseObject {
                          v8::Local<v8::Value> unused,
                          v8::Local<v8::Context> context);
 
+  static void DestroyIdsCb(uv_idle_t* handle);
+
   inline ProviderType provider_type() const;
 
   inline int64_t get_uid() const;

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -194,6 +194,8 @@ inline Environment::Environment(IsolateData* isolate_data,
 
   RB_INIT(&cares_task_list_);
   AssignToContext(context);
+
+  destroy_ids_list_.reserve(512);
 }
 
 inline Environment::~Environment() {
@@ -247,6 +249,15 @@ 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 uv_idle_t* Environment::destroy_ids_idle_handle() {
+  return &destroy_ids_idle_handle_;
+}
+
 inline void Environment::RegisterHandleCleanup(uv_handle_t* handle,
                                                HandleCleanupCb cb,
                                                void *arg) {
@@ -301,6 +312,10 @@ inline int64_t Environment::get_async_wrap_uid() {
   return ++async_wrap_uid_;
 }
 
+inline std::vector<int64_t>* Environment::destroy_ids_list() {
+  return &destroy_ids_list_;
+}
+
 inline uint32_t* Environment::heap_statistics_buffer() const {
   CHECK_NE(heap_statistics_buffer_, nullptr);
   return heap_statistics_buffer_;

diff --git a/src/env.cc b/src/env.cc
@@ -49,6 +49,9 @@ 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()));
+
   auto close_and_finish = [](Environment* env, uv_handle_t* handle, void* arg) {
     handle->data = env;
 

diff --git a/src/env.h b/src/env.h
@@ -16,6 +16,7 @@
 #include "v8.h"
 
 #include <stdint.h>
+#include <vector>
 
 // Caveat emptor: we're going slightly crazy with macros here but the end
 // hopefully justifies the means. We have a lot of per-context properties
@@ -431,8 +432,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);
   inline uv_check_t* immediate_check_handle();
   inline uv_idle_t* immediate_idle_handle();
+  inline uv_idle_t* destroy_ids_idle_handle();
 
   // Register clean-up cb to be called on environment destruction.
   inline void RegisterHandleCleanup(uv_handle_t* handle,
@@ -463,6 +466,9 @@ class Environment {
 
   inline int64_t get_async_wrap_uid();
 
+  // List of id's that have been destroyed and need the destroy() cb called.
+  inline std::vector<int64_t>* destroy_ids_list();
+
   inline uint32_t* heap_statistics_buffer() const;
   inline void set_heap_statistics_buffer(uint32_t* pointer);
 
@@ -548,6 +554,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_prepare_t idle_prepare_handle_;
   uv_check_t idle_check_handle_;
   AsyncHooks async_hooks_;
@@ -562,6 +569,7 @@ class Environment {
   bool trace_sync_io_;
   size_t makecallback_cntr_;
   int64_t async_wrap_uid_;
+  std::vector<int64_t> destroy_ids_list_;
   debugger::Agent debugger_agent_;
 #if HAVE_INSPECTOR
   inspector::Agent inspector_agent_;

diff --git a/test/parallel/test-async-wrap-uid.js b/test/parallel/test-async-wrap-uid.js
@@ -6,15 +6,14 @@ const assert = require('assert');
 const async_wrap = process.binding('async_wrap');
 
 const storage = new Map();
-async_wrap.setupHooks({ init, pre, post, destroy });
+async_wrap.setupHooks({ init, pre, post });
 async_wrap.enable();
 
 function init(uid) {
   storage.set(uid, {
     init: true,
     pre: false,
     post: false,
-    destroy: false
   });
 }
 
@@ -26,10 +25,6 @@ function post(uid) {
   storage.get(uid).post = true;
 }
 
-function destroy(uid) {
-  storage.get(uid).destroy = true;
-}
-
 fs.access(__filename, function(err) {
   assert.ifError(err);
 });
@@ -51,7 +46,6 @@ process.once('exit', function() {
       init: true,
       pre: true,
       post: true,
-      destroy: true
     });
   }
 });