Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Late enabling of async_hooks after promise creation causes process abort #13237

Closed
AndreasMadsen opened this issue May 26, 2017 · 4 comments
Closed
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@AndreasMadsen
Copy link
Member

AndreasMadsen commented May 26, 2017

  • 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

@AndreasMadsen AndreasMadsen added async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs. async_wrap labels May 26, 2017
@Fishrock123
Copy link
Contributor

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

@addaleax
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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

Done in #13242 :)

addaleax added a commit to addaleax/node that referenced this issue May 27, 2017
Assign a `PromiseWrap` instance to Promises that do not have one
yet when the PromiseHook is being called.

Fixes: nodejs#13237
jasnell pushed a commit that referenced this issue May 28, 2017
Assign a `PromiseWrap` instance to Promises that do not have one
yet when the PromiseHook is being called.

Fixes: #13237
PR-URL: #13242
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Matthew Loring <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

3 participants