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

async_hooks: implement C++ embedder API #13142

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@addaleax
Member

addaleax commented May 21, 2017

This is based on top of the current version of #13000, and currently still missing tests.

/cc @nodejs/diagnostics and in particular @AndreasMadsen @trevnorris @matthewloring @danbev

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

async_hooks

@addaleax

I’ve got a few questions about edge cases where it would be cool if @AndreasMadsen or @trevnorris could chime in.

Also, I can open a separate PR for cfe5d401921e72af3f6dc03a5a373401ba4e32e8 because it’s not really related to the rest here, and could be backported to all release lines if you think that makes sense.

Show outdated Hide outdated src/node.cc
Show outdated Hide outdated src/node.cc
Show outdated Hide outdated src/node_crypto.cc
Show outdated Hide outdated src/node_crypto.cc
@danbev

danbev approved these changes May 23, 2017

Show outdated Hide outdated src/node.h
@AndreasMadsen

Thanks for taking the time to implement this, here is my initial review:

  • Can we not use DomainEnter and DomainExit in MakeCallback?

  • How is the trigger set in the Init emit? set_init_trigger_id is not a public API, I think we need to make trigger_id an option in EmitAsyncInit and AsyncResource::AsyncResource. That should also allow us to remove trigger_id from AsyncResource::MakeCallback.

/* Helper class users can optionally inherit from. If
* `AsyncResource::MakeCallback()` is used, then all four callbacks will be
* called automatically. */
class AsyncResource {

This comment has been minimized.

@AndreasMadsen

AndreasMadsen May 24, 2017

Member

In the JS Embedder API this is called AsyncEvent, I actually like the AsyncResource name better, but at the very least it is inconsistent.

@AndreasMadsen

AndreasMadsen May 24, 2017

Member

In the JS Embedder API this is called AsyncEvent, I actually like the AsyncResource name better, but at the very least it is inconsistent.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 24, 2017

Member

Can we not use DomainEnter and DomainExit in MakeCallback?

Can you be a bit more specific why we wouldn’t want to use these?

In the JS Embedder API this is called AsyncEvent, I actually like the AsyncResource name better, but at the very least it is inconsistent.

#13192

Member

addaleax commented May 24, 2017

Can we not use DomainEnter and DomainExit in MakeCallback?

Can you be a bit more specific why we wouldn’t want to use these?

In the JS Embedder API this is called AsyncEvent, I actually like the AsyncResource name better, but at the very least it is inconsistent.

#13192

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 24, 2017

Member

Can you be a bit more specific why we wouldn’t want to use these?

Sorry, I think I messed up in language negatives. I meant:

Can we not use DomainEnter and DomainExit in MakeCallback?

Member

AndreasMadsen commented May 24, 2017

Can you be a bit more specific why we wouldn’t want to use these?

Sorry, I think I messed up in language negatives. I meant:

Can we not use DomainEnter and DomainExit in MakeCallback?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 24, 2017

Member

think we need to make trigger_id an option in EmitAsyncInit and AsyncResource::AsyncResource

done

Can we not use DomainEnter and DomainExit in MakeCallback?

done!

Member

addaleax commented May 24, 2017

think we need to make trigger_id an option in EmitAsyncInit and AsyncResource::AsyncResource

done

Can we not use DomainEnter and DomainExit in MakeCallback?

done!

Show outdated Hide outdated src/node.h

@AndreasMadsen AndreasMadsen referenced this pull request May 24, 2017

Closed

[async_hooks] tracking issue #29

23 of 27 tasks complete
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

Added a test. @danbev If you sign off on the test code (edit: and the switch to utf8), I’ll squash the two commits together; otherwise, could somebody else review the test?

CI: https://ci.nodejs.org/job/node-test-commit/10155/

Member

addaleax commented May 25, 2017

Added a test. @danbev If you sign off on the test code (edit: and the switch to utf8), I’ll squash the two commits together; otherwise, could somebody else review the test?

CI: https://ci.nodejs.org/job/node-test-commit/10155/

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 25, 2017

Member

Looks good. I hate to be the person who would like more test but doesn't have the time to implement them, but I think we need:

AsyncResource

  • test for trigger_id in AsyncResource
  • test for AsyncResource::get_uid
  • test for AsyncResource::get_resource

createHooks

  • test for id in async_hooks.createHooks
  • test for resource in async_hooks.createHooks
  • test for type in async_hooks.createHooks
  • test for triggerId in async_hooks.createHooks

AsyncHook state

  • test for node::AsyncHooksGetCurrentId
  • test for node::AsyncHooksGetTriggerId

Standalone

  • test for node::EmitAsyncInit
  • test for node::EmitAsyncDestroy
  • test for node::MakeCallback
Member

AndreasMadsen commented May 25, 2017

Looks good. I hate to be the person who would like more test but doesn't have the time to implement them, but I think we need:

AsyncResource

  • test for trigger_id in AsyncResource
  • test for AsyncResource::get_uid
  • test for AsyncResource::get_resource

createHooks

  • test for id in async_hooks.createHooks
  • test for resource in async_hooks.createHooks
  • test for type in async_hooks.createHooks
  • test for triggerId in async_hooks.createHooks

AsyncHook state

  • test for node::AsyncHooksGetCurrentId
  • test for node::AsyncHooksGetTriggerId

Standalone

  • test for node::EmitAsyncInit
  • test for node::EmitAsyncDestroy
  • test for node::MakeCallback
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

AsyncResource

Mhm, I can do those.

createHooks

I would like if you could be more specific here. What you list are the arguments to init, but I’d say at least id and type are already reasonably checked, so I’m not sure I get what you’re referring to?

AsyncHook state & Standalone

Yeah, if you insist I can do that, but since AsyncResource is just a really thin wrapper around these I’m not sure that’s really worth at this point?

Member

addaleax commented May 25, 2017

AsyncResource

Mhm, I can do those.

createHooks

I would like if you could be more specific here. What you list are the arguments to init, but I’d say at least id and type are already reasonably checked, so I’m not sure I get what you’re referring to?

AsyncHook state & Standalone

Yeah, if you insist I can do that, but since AsyncResource is just a really thin wrapper around these I’m not sure that’s really worth at this point?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

@AndreasMadsen I think I addressed your first point here; are you okay with this, i.e. does the test LGTY?

Member

addaleax commented May 25, 2017

@AndreasMadsen I think I addressed your first point here; are you okay with this, i.e. does the test LGTY?

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 25, 2017

Member

Sorry, for not being more specific

  • test for id in async_hooks.createHooks, check that the id is a new number, in particular that it is not `undefined^ or something silly.
  • test for resource in async_hooks.createHooks, check that the resource equals the test object.
  • test for type in async_hooks.createHooks, this is fine.
  • test for triggerId in async_hooks.createHooks, check that the triggerId is correctly propergated from the AsuncResource constructor argument.

Yeah, if you insist I can do that, but since AsyncResource is just a really thin wrapper around these I’m not sure that’s really worth at this point?

At the very least test node::AsyncHooksGetCurrentId since it is not wrapped by AsyncResource.


I think I addressed your first point here; are you okay with this, i.e. does the test LGTY?

Thanks, I will take a closer look tomorrow.

Member

AndreasMadsen commented May 25, 2017

Sorry, for not being more specific

  • test for id in async_hooks.createHooks, check that the id is a new number, in particular that it is not `undefined^ or something silly.
  • test for resource in async_hooks.createHooks, check that the resource equals the test object.
  • test for type in async_hooks.createHooks, this is fine.
  • test for triggerId in async_hooks.createHooks, check that the triggerId is correctly propergated from the AsuncResource constructor argument.

Yeah, if you insist I can do that, but since AsyncResource is just a really thin wrapper around these I’m not sure that’s really worth at this point?

At the very least test node::AsyncHooksGetCurrentId since it is not wrapped by AsyncResource.


I think I addressed your first point here; are you okay with this, i.e. does the test LGTY?

Thanks, I will take a closer look tomorrow.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 25, 2017

Member

@AndreasMadsen Thanks for explaining, those points should be addressed now. :)

EDIT: new CI: https://ci.nodejs.org/job/node-test-commit/10160/

Member

addaleax commented May 25, 2017

@AndreasMadsen Thanks for explaining, those points should be addressed now. :)

EDIT: new CI: https://ci.nodejs.org/job/node-test-commit/10160/

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 26, 2017

Member

Windows CI failure was real, I forgot to add NODE_EXTERNs…

CI: https://ci.nodejs.org/job/node-test-commit/10171/

Member

addaleax commented May 26, 2017

Windows CI failure was real, I forgot to add NODE_EXTERNs…

CI: https://ci.nodejs.org/job/node-test-commit/10171/

Show outdated Hide outdated src/node.cc
@AndreasMadsen

LGTM, but I would like another review on this.

I'm okay with not addressing the beforeExit and exit issues here. The issue already existed, it is just more obvious now.

@addaleax addaleax referenced this pull request May 26, 2017

Closed

async_hooks: fix Promises with later enabled hooks #13242

3 of 3 tasks complete
@matthewloring

Looks good overall! Put down a few questions

Show outdated Hide outdated src/node.cc
Show outdated Hide outdated src/node.cc

addaleax added some commits May 21, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
Member

addaleax commented May 27, 2017

@AndreasMadsen

This comment has been minimized.

Show comment
Hide comment
@AndreasMadsen

AndreasMadsen May 27, 2017

Member

Are you okay with running CI + landing this if it comes back green?

yes! LGTM, you can merge if CI is green.

Member

AndreasMadsen commented May 27, 2017

Are you okay with running CI + landing this if it comes back green?

yes! LGTM, you can merge if CI is green.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 27, 2017

Member

Landed in a86323d, 1a2cd91

Member

addaleax commented May 27, 2017

Landed in a86323d, 1a2cd91

@addaleax addaleax closed this May 27, 2017

@addaleax addaleax deleted the addaleax:async-hooks-native-api branch May 27, 2017

addaleax added a commit that referenced this pull request May 27, 2017

async_hooks: implement C++ embedder API
PR-URL: #13142
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

addaleax added a commit that referenced this pull request May 27, 2017

test: add AsyncResource addon test
PR-URL: #13142
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

jasnell added a commit that referenced this pull request May 28, 2017

async_hooks: implement C++ embedder API
PR-URL: #13142
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

jasnell added a commit that referenced this pull request May 28, 2017

test: add AsyncResource addon test
PR-URL: #13142
Reviewed-By: Matthew Loring <mattloring@google.com>
Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

@jasnell jasnell referenced this pull request May 28, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment