Late enabling of async_hooks after promise creation causes process abort

[AndreasMadsen]

• Version: master
• Platform: all
• Subsystem: async_hooks

Having thought some more about #13177, I don't think there is much difference between that approach and checking if async_hooks is enabled by:

uint32_t* fields_ptr = async_hooks->fields();
if (fields_ptr[AsyncHooks::kInit] +
    fields_ptr[AsyncHooks::kBefore] +
    fields_ptr[AsyncHooks::kAfter] +
    fields_ptr[AsyncHooks::kDestroy] > 0) {

}

But the above solution is more consistent when .disable() is called after .enable(). Perhaps, more importantly, it highlights an issue where the promise is created just before the hooks are setup. In which case the node process will currently abort because of CHECK_NE(wrap, nullptr);.

const async_hooks = require('async_hooks');

const p = new Promise((resolve) => resolve(1));
p.then();

const hooks = async_hooks.createHook({
  before(id) {
    process._rawDebug(id);
  }
}).enable();

causes the following error:

./out/Release/node[21762]: ../src/async-wrap.cc:310:void node::PromiseHook(v8::PromiseHookType, Local<v8::Promise>, Local<v8::Value>, void *): Assertion `(wrap) != (nullptr)' failed.
 1: node::Abort() [./node]
 2: node::FatalError(char const*, char const*) [./node]
 3: node::PromiseHook(v8::PromiseHookType, v8::Local<v8::Promise>, v8::Local<v8::Value>, void*) [./node]
 4: node::Environment::EnvPromiseHook(v8::PromiseHookType, v8::Local<v8::Promise>, v8::Local<v8::Value>) [./node]
 5: v8::internal::Runtime_PromiseHookBefore(int, v8::internal::Object**, v8::internal::Isolate*) [./node]
 6: 0x16a920f0437d
[1]    21762 abort      ./node test.js

I don't think there is a good solution to this problem. But I would like to another opinion on that.

Otherwise, we will just have to check for the env->promise_wrap() property and not emit before, after and destroy in this case. This is a difference behavior than that for all other *Wrap types, which is why it is not a good solution.

/cc @addaleax @matthewloring

[Fishrock123]

Could we create a new PromiseWrap if there wasn't one already?

[addaleax]

Could we create a new PromiseWrap if there wasn't one already?

Ack, I think this is a better solution, at least as long as we don’t have metrics on how expensive enabling a PromiseHook actually is; I can’t really imagine that it’s close to having no overhead. I’ll open a PR shortly.

[AndreasMadsen]

Could we create a new PromiseWrap if there wasn't one already?

Sounds good. But we will have to make sure the init hooks isn't fired upon creation only in PromiseHookType::kInit.

[addaleax]

Sounds good. But we will have to make sure the init hooks isn't fired upon creation only in PromiseHookType::kInit.

Done in #13242 :)