async_hooks: implement C++ embedder API#13142
async_hooks: implement C++ embedder API#13142addaleax wants to merge 8 commits intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
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.
src/node.cc
Outdated
There was a problem hiding this comment.
There should be no async hooks firing for the call that runs setImmediate() timers, right?
src/node.cc
Outdated
There was a problem hiding this comment.
I’m really not sure whether 0 is correct here.
There was a problem hiding this comment.
Good question. In theory, we should only have 0 as the id when the resource is unknown, which should only happen because of legacy code.
I'm not really sure what the appropriate resource is, I guess we could create an event-loop resource on startup.
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
If SecureContext inherits from AsyncWrap as @trevnorris suggests this problem should be solved.
There was a problem hiding this comment.
See #13176, I don’t think we need to actually care about this
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
Hm. I missed this. This should inherit from AsyncWrap.
src/node.h
Outdated
There was a problem hiding this comment.
Suggestion: Helper class users can optionally inherit from.
There was a problem hiding this comment.
Thanks for taking the time to implement this, here is my initial review:
-
Can we not use
DomainEnterandDomainExitinMakeCallback? -
How is the trigger set in the
Initemit?set_init_trigger_idis not a public API, I think we need to maketrigger_idan option inEmitAsyncInitandAsyncResource::AsyncResource. That should also allow us to removetrigger_idfromAsyncResource::MakeCallback.
src/node.h
Outdated
There was a problem hiding this comment.
In the JS Embedder API this is called AsyncEvent, I actually like the AsyncResource name better, but at the very least it is inconsistent.
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:
|
e6c71b6 to
de2fba0
Compare
done
done! |
src/node.h
Outdated
There was a problem hiding this comment.
The trigger_id passed to AsyncResource::MakeCallback should be the same as the one passed to AsyncResource::AsyncResource, so I think we should just store trigger_id in a private field in AsyncResource::AsyncResource and use that field in AsyncResource::MakeCallback.
That way we don't need the trigger_id argument in AsyncResource::MakeCallback.
There was a problem hiding this comment.
Done, although I have to admit I’m not a hundred percent sure what the semantics of trigger_id in before/after hooks would be if the value is always the same that was passed for the init hook … maybe I share @joshgav’s confusion about it? ;)
48a3c4f to
616a20f
Compare
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), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks