Repository navigation
Late enabling of async_hooks after promise creation causes process abort #13237
Description
- 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.
Activity
Fishrock123 commentedon May 26, 2017
Could we create a new
PromiseWrapif there wasn't one already?addaleax commentedon May 26, 2017
Ack, I think this is a better solution, at least as long as we don’t have metrics on how expensive enabling a
PromiseHookactually is; I can’t really imagine that it’s close to having no overhead. I’ll open a PR shortly.AndreasMadsen commentedon May 26, 2017
Sounds good. But we will have to make sure the
inithooks isn't fired upon creation only inPromiseHookType::kInit.addaleax commentedon May 26, 2017
Done in #13242 :)
async_hooks: fix Promises with later enabled hooks
async_wrap: fix Promises with later enabled hooks
async_wrap: fix Promises with later enabled hooks