async_wrap: use kTotals to enable PromiseHook

Keep a total of enabled hook callbacks in kTotals. This value is used to track whether node::PromiseHook (src/async-wrap.cc) should be enabled or disabled. Don't enable node::PromiseHook, using enablePromiseHook(), until a hook has been added. Then, using disablePromiseHook(), disable node::PromiseHook when all hooks have been disabled. Need to use a native test in order to check the internal field of the Promise and check for a PromiseWrap. PR-URL: #13509 Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Author: trevnorris

lib/async_hooks.js

@@ -38,8 +38,8 @@ var tmp_async_hook_fields = null;
 // Each constant tracks how many callbacks there are for any given step of
 // async execution. These are tracked so if the user didn't include callbacks
 // for a given step, that step can bail out early.
-const{ kInit, kBefore, kAfter, kDestroy,kCurrentAsyncId, kCurrentTriggerId,
- kAsyncUidCntr, kInitTriggerId }=async_wrap.constants;
+const{ kInit, kBefore, kAfter, kDestroy,kTotals, kCurrentAsyncId,
+kCurrentTriggerId, kAsyncUidCntr, kInitTriggerId }=async_wrap.constants;
 
 const{ async_id_symbol, trigger_id_symbol }=async_wrap;
 
@@ -50,7 +50,9 @@ const after_symbol = Symbol('after');
 constdestroy_symbol=Symbol('destroy');
 
 // Setup the callbacks that node::AsyncWrap will call when there are hooks to
-// process. They use the same functions as the JS embedder API.
+// process. They use the same functions as the JS embedder API. These callbacks
+// are setup immediately to prevent async_wrap.setupHooks() from being hijacked
+// and the cost of doing so is negligible.
 async_wrap.setupHooks({ init,
 before: emitBeforeN,
 after: emitAfterN,
@@ -103,14 +105,21 @@ class AsyncHook{
 if(hooks_array.includes(this))
 returnthis;
 
+constprev_kTotals=hook_fields[kTotals];
+hook_fields[kTotals]=0;
+
 // createHook() has already enforced that the callbacks are all functions,
 // so here simply increment the count of whether each callbacks exists or
 // not.
-hook_fields[kInit]+=+!!this[init_symbol];
-hook_fields[kBefore]+=+!!this[before_symbol];
-hook_fields[kAfter]+=+!!this[after_symbol];
-hook_fields[kDestroy]+=+!!this[destroy_symbol];
+hook_fields[kTotals]+=hook_fields[kInit]+=+!!this[init_symbol];
+hook_fields[kTotals]+=hook_fields[kBefore]+=+!!this[before_symbol];
+hook_fields[kTotals]+=hook_fields[kAfter]+=+!!this[after_symbol];
+hook_fields[kTotals]+=hook_fields[kDestroy]+=+!!this[destroy_symbol];
 hooks_array.push(this);
+
+if(prev_kTotals===0&&hook_fields[kTotals]>0)
+async_wrap.enablePromiseHook();
+
 returnthis;
 }
 
@@ -121,11 +130,18 @@ class AsyncHook{
 if(index===-1)
 returnthis;
 
-hook_fields[kInit]-=+!!this[init_symbol];
-hook_fields[kBefore]-=+!!this[before_symbol];
-hook_fields[kAfter]-=+!!this[after_symbol];
-hook_fields[kDestroy]-=+!!this[destroy_symbol];
+constprev_kTotals=hook_fields[kTotals];
+hook_fields[kTotals]=0;
+
+hook_fields[kTotals]+=hook_fields[kInit]-=+!!this[init_symbol];
+hook_fields[kTotals]+=hook_fields[kBefore]-=+!!this[before_symbol];
+hook_fields[kTotals]+=hook_fields[kAfter]-=+!!this[after_symbol];
+hook_fields[kTotals]+=hook_fields[kDestroy]-=+!!this[destroy_symbol];
 hooks_array.splice(index,1);
+
+if(prev_kTotals>0&&hook_fields[kTotals]===0)
+async_wrap.disablePromiseHook();
+
 returnthis;
 }
 }

src/async-wrap.cc

@@ -335,12 +335,17 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
  Local<Value> parent, void* arg){
  Local<Context> context = promise->CreationContext();
  Environment* env = Environment::GetCurrent(context);
+
+// PromiseHook() should never be called if no hooks have been enabled.
+CHECK_GT(env->async_hooks()->fields()[AsyncHooks::kTotals], 0);
+
  Local<Value> resource_object_value = promise->GetInternalField(0);
  PromiseWrap* wrap = nullptr;
 if (resource_object_value->IsObject()){
  Local<Object> resource_object = resource_object_value.As<Object>();
  wrap = Unwrap<PromiseWrap>(resource_object);
  }
+
 if (type == PromiseHookType::kInit || wrap == nullptr){
 bool silent = type != PromiseHookType::kInit;
  PromiseWrap* parent_wrap = nullptr;
@@ -368,6 +373,7 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
  } elseif (type == PromiseHookType::kResolve){
 // TODO(matthewloring): need to expose this through the async hooks api.
  }
+
 CHECK_NE(wrap, nullptr);
 if (type == PromiseHookType::kBefore){
 PreCallbackExecution(wrap, false);
@@ -401,7 +407,6 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args){
 SET_HOOK_FN(before);
 SET_HOOK_FN(after);
 SET_HOOK_FN(destroy);
- env->AddPromiseHook(PromiseHook, nullptr);
 #undef SET_HOOK_FN
 
 {
@@ -542,6 +547,7 @@ void AsyncWrap::Initialize(Local<Object> target,
 SET_HOOKS_CONSTANT(kBefore);
 SET_HOOKS_CONSTANT(kAfter);
 SET_HOOKS_CONSTANT(kDestroy);
+SET_HOOKS_CONSTANT(kTotals);
 SET_HOOKS_CONSTANT(kCurrentAsyncId);
 SET_HOOKS_CONSTANT(kCurrentTriggerId);
 SET_HOOKS_CONSTANT(kAsyncUidCntr);

src/env.h

@@ -344,6 +344,7 @@ class Environment{
 kBefore,
 kAfter,
 kDestroy,
+kTotals,
 kFieldsCount,
  };
 

test/addons/async-hooks-promise/binding.cc

@@ -0,0 +1,43 @@
+#include<node.h>
+#include<v8.h>
+
+namespace{
+
+using v8::FunctionCallbackInfo;
+using v8::Isolate;
+using v8::Local;
+using v8::NewStringType;
+using v8::Object;
+using v8::Promise;
+using v8::String;
+using v8::Value;
+
+staticvoidThrowError(Isolate* isolate, constchar* err_msg){
+ Local<String> str = String::NewFromOneByte(
+ isolate,
+reinterpret_cast<constuint8_t*>(err_msg),
+ NewStringType::kNormal).ToLocalChecked();
+ isolate->ThrowException(str);
+}
+
+staticvoidGetPromiseField(const FunctionCallbackInfo<Value>& args){
+auto isolate = args.GetIsolate();
+
+if (!args[0]->IsPromise())
+returnThrowError(isolate, "arg is not an Promise");
+
+auto p = args[0].As<Promise>();
+if (p->InternalFieldCount() < 1)
+returnThrowError(isolate, "Promise has no internal field");
+
+auto l = p->GetInternalField(0);
+ args.GetReturnValue().Set(l);
+}
+
+inlinevoidInitialize(v8::Local<v8::Object> binding){
+NODE_SET_METHOD(binding, "getPromiseField", GetPromiseField);
+}
+
+NODE_MODULE(binding, Initialize)
+
+} // anonymous namespace

test/addons/async-hooks-promise/binding.gyp

@@ -0,0 +1,9 @@
+{
+'targets': [
+{
+'target_name': 'binding',
+'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
+'sources': [ 'binding.cc' ]
+ }
+ ]
+}

test/addons/async-hooks-promise/test.js

@@ -0,0 +1,43 @@
+'use strict';
+
+constcommon=require('../../common');
+constassert=require('assert');
+constasync_hooks=require('async_hooks');
+constbinding=require(`./build/${common.buildType}/binding`);
+
+// Baseline to make sure the internal field isn't being set.
+assert.strictEqual(
+binding.getPromiseField(Promise.resolve(1)),
+0,
+'Promise internal field used despite missing enabled AsyncHook');
+
+consthook0=async_hooks.createHook({}).enable();
+
+// Check that no PromiseWrap is created when there are no hook callbacks.
+assert.strictEqual(
+binding.getPromiseField(Promise.resolve(1)),
+0,
+'Promise internal field used despite missing enabled AsyncHook');
+
+hook0.disable();
+
+letpwrap=null;
+consthook1=async_hooks.createHook({
+init(id,type,tid,resource){
+pwrap=resource;
+}
+}).enable();
+
+// Check that the internal field returns the same PromiseWrap passed to init().
+assert.strictEqual(
+binding.getPromiseField(Promise.resolve(1)),
+pwrap,
+'Unexpected PromiseWrap');
+
+hook1.disable();
+
+// Check that internal fields are no longer being set.
+assert.strictEqual(
+binding.getPromiseField(Promise.resolve(1)),
+0,
+'Promise internal field used despite missing enabled AsyncHook');

test/parallel/test-async-hooks-promise-enable-disable.js

@@ -0,0 +1,47 @@
+'use strict';
+
+constcommon=require('../common');
+constassert=require('assert');
+constasync_hooks=require('async_hooks');
+constEXPECTED_INITS=2;
+letp_resource=null;
+letp_er=null;
+letp_inits=0;
+
+// Not useful to place common.mustCall() around 'exit' event b/c it won't be
+// able to check it anway.
+process.on('exit',(code)=>{
+if(code!==0)
+return;
+if(p_er!==null)
+throwp_er;
+// Expecint exactly 2 PROMISE types to reach init.
+assert.strictEqual(p_inits,EXPECTED_INITS);
+});
+
+constmustCallInit=common.mustCall(functioninit(id,type,tid,resource){
+if(type!=='PROMISE')
+return;
+p_inits++;
+p_resource=resource.promise;
+},EXPECTED_INITS);
+
+consthook=async_hooks.createHook({
+init: mustCallInit
+// Enable then disable to test whether disable() actually works.
+}).enable().disable().disable();
+
+newPromise(common.mustCall((res)=>{
+res(42);
+})).then(common.mustCall((val)=>{
+hook.enable().enable();
+constp=newPromise((res)=>res(val));
+assert.strictEqual(p,p_resource);
+hook.disable();
+returnp;
+})).then(common.mustCall((val2)=>{
+hook.enable();
+constp=newPromise((res)=>res(val2));
+hook.disable();
+returnp;
+})).catch((er)=>p_er=er);