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. Fixes: #8216Fixes: #9465 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Author: trevnorris

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"
 
@@ -183,6 +184,38 @@ void AsyncWrap::Initialize(Local<Object> target,
 }
 
 
+voidAsyncWrap::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);
+ }
+ }
+}
+
+
 voidLoadAsyncWrapperInfo(Environment* env){
  HeapProfiler* heap_profiler = env->isolate()->GetHeapProfiler();
 #defineV(PROVIDER) \
@@ -249,18 +282,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());
 }
 
 

src/async-wrap.h

@@ -2,6 +2,7 @@
 #defineSRC_ASYNC_WRAP_H_
 
 #include"base-object.h"
+#include"uv.h"
 #include"v8.h"
 
 #include<stdint.h>
@@ -58,6 +59,8 @@ class AsyncWrap : public BaseObject{
  v8::Local<v8::Value> unused,
  v8::Local<v8::Context> context);
 
+staticvoidDestroyIdsCb(uv_idle_t* handle);
+
 inline ProviderType provider_type() const;
 
 inlineint64_tget_uid() const;

src/env-inl.h

@@ -227,6 +227,8 @@ inline Environment::Environment(v8::Local<v8::Context> context,
 
 RB_INIT(&cares_task_list_);
  handle_cleanup_waiting_ = 0;
+
+ destroy_ids_list_.reserve(512);
 }
 
 inlineEnvironment::~Environment(){
@@ -286,6 +288,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){
+returnContainerOf(&Environment::destroy_ids_idle_handle_, handle);
+}
+
+inlineuv_idle_t* Environment::destroy_ids_idle_handle(){
+return &destroy_ids_idle_handle_;
+}
+
 inline Environment* Environment::from_idle_prepare_handle(
 uv_prepare_t* handle){
 returnContainerOf(&Environment::idle_prepare_handle_, handle);
@@ -362,6 +373,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_;
+}
+
 inlineuint32_t* Environment::heap_statistics_buffer() const{
 CHECK_NE(heap_statistics_buffer_, nullptr);
 return heap_statistics_buffer_;

src/env.h

@@ -11,6 +11,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
@@ -413,8 +414,10 @@ class Environment{
 inlineuint32_twatched_providers() const;
 
 staticinline Environment* from_immediate_check_handle(uv_check_t* handle);
+staticinline Environment* from_destroy_ids_idle_handle(uv_idle_t* handle);
 inlineuv_check_t* immediate_check_handle();
 inlineuv_idle_t* immediate_idle_handle();
+inlineuv_idle_t* destroy_ids_idle_handle();
 
 staticinline Environment* from_idle_prepare_handle(uv_prepare_t* handle);
 inlineuv_prepare_t* idle_prepare_handle();
@@ -451,6 +454,9 @@ class Environment{
 
 inlineint64_tget_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();
+
 inlineuint32_t* heap_statistics_buffer() const;
 inlinevoidset_heap_statistics_buffer(uint32_t* pointer);
 
@@ -531,6 +537,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_;
@@ -546,6 +553,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_;
 
  HandleWrapQueue handle_wrap_queue_;

src/node.cc

@@ -4202,6 +4202,9 @@ Environment* CreateEnvironment(Isolate* isolate,
 uv_unref(reinterpret_cast<uv_handle_t*>(env->idle_prepare_handle()));
 uv_unref(reinterpret_cast<uv_handle_t*>(env->idle_check_handle()));
 
+uv_idle_init(env->event_loop(), env->destroy_ids_idle_handle());
+uv_unref(reinterpret_cast<uv_handle_t*>(env->destroy_ids_idle_handle()));
+
 // Register handle cleanups
  env->RegisterHandleCleanup(
 reinterpret_cast<uv_handle_t*>(env->immediate_check_handle()),

test/parallel/test-async-wrap-throw-from-callback.js

@@ -6,7 +6,7 @@ const assert = require('assert');
 constcrypto=require('crypto');
 constdomain=require('domain');
 constspawn=require('child_process').spawn;
-constcallbacks=['init','pre','post','destroy'];
+constcallbacks=['init','pre','post'];
 consttoCall=process.argv[2];
 varmsgCalled=0;
 varmsgReceived=0;
@@ -23,13 +23,9 @@ function post(){
 if(toCall==='post')
 thrownewError('post');
 }
-functiondestroy(){
-if(toCall==='destroy')
-thrownewError('destroy');
-}
 
 if(typeofprocess.argv[2]==='string'){
-async_wrap.setupHooks({ init, pre, post, destroy});
+async_wrap.setupHooks({ init, pre, post });
 async_wrap.enable();
 
 process.on('uncaughtException',()=>assert.ok(0,'UNREACHABLE'));

test/parallel/test-async-wrap-uid.js

@@ -6,15 +6,14 @@ const assert = require('assert');
 constasync_wrap=process.binding('async_wrap');
 
 conststorage=newMap();
-async_wrap.setupHooks({ init, pre, post, destroy});
+async_wrap.setupHooks({ init, pre, post });
 async_wrap.enable();
 
 functioninit(uid){
 storage.set(uid,{
 init: true,
 pre: false,
 post: false,
-destroy: false
 });
 }
 
@@ -26,10 +25,6 @@ function post(uid){
 storage.get(uid).post=true;
 }
 
-functiondestroy(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
 });
 }
 });