[v8.x] async_hooks: C++ Embedder API overhaul#14109
Uh oh!
There was an error while loading. Please reload this page.
Conversation
addaleax added addons 
nodejs-github-bot added c++ addaleax added the semver-minor 
refack commented Jul 6, 2017
I would love to see just the squashed tests be run on |
addaleax commented Jul 6, 2017
I mean, it’s not like you can’t do that already. I’ll try if I find the time, but you know, you have the same tools in front of me that I have ;) |
refack commented Jul 6, 2017
Yes, also I assume they pass, I'm more thinking out loud about adding a regression test ad-hoc... |
AndreasMadsen commented Jul 7, 2017
Besides containing some API changes, this also contains an important fix to |
src/async-wrap.cc Outdated
There was a problem hiding this comment.
No particular reason, except maybe that it kind of belongs to the rest of the legacy code here.
There was a problem hiding this comment.
All MakeCallback functions are in node.cc including the legacy ones. But since this won't land in master I don't care that much.
src/async-wrap.cc Outdated
There was a problem hiding this comment.
Should this not be async_id? Not that it really matters.
There was a problem hiding this comment.
Maybe? I can change it if you like. 😄
There was a problem hiding this comment.
The EmitAsyncInit that returns async_id requires 4 parameters, correct? Or is this a test for the async_context casting?
Also, is node::async_uid by purpose?
There was a problem hiding this comment.
The
EmitAsyncInitthat returnsasync_idrequires 4 parameters, correct? Or is this a test for theasync_contextcasting?
Yes, the latter. The old one is not really exposed through our API anymore, it’s just the symbol that has to keep being exported so already-compiled libraries can reference it.
Also, is
node::async_uidby purpose?
Yes, everywhere else in the tests we use async_id now, so there should be one place where we don’t.
AndreasMadsen left a comment
AndreasMadsen left a comment There was a problem hiding this comment.
LGTM. I have a few questions but I think it is correct.
I don't have skills to review the ABI stuff, someone else should do that. The logic itself looks fine.
src/node.h Outdated
There was a problem hiding this comment.
Should we not deprecate these too?
There was a problem hiding this comment.
Yes, thanks for pointing that out.
* Fix AsyncHooksGetTriggerAsyncId such it corresponds to async_hooks.triggerAsyncId and not async_hooks.initTriggerId. * Use an async_context struct instead of two async_uid values. This change was necessary since the fixing AsyncHooksGetTriggerAsyncId otherwise makes it impossible to get the correct default trigger id. It also prevents an invalid triggerAsyncId in MakeCallback. * Rename async_uid to async_id for consistency * Rename get_uid to get_async_id * Add get_trigger_async_id to AsyncResource class PR-URL: nodejs#14040 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
addaleaxforce-pushed the v8.x-async-wrap-cpp-overhaul branch from 3409631 to a2f61c4Compareaddaleax commented Jul 11, 2017
* Fix AsyncHooksGetTriggerAsyncId such it corresponds to async_hooks.triggerAsyncId and not async_hooks.initTriggerId. * Use an async_context struct instead of two async_uid values. This change was necessary since the fixing AsyncHooksGetTriggerAsyncId otherwise makes it impossible to get the correct default trigger id. It also prevents an invalid triggerAsyncId in MakeCallback. * Rename async_uid to async_id for consistency * Rename get_uid to get_async_id * Add get_trigger_async_id to AsyncResource class PR-URL: #14040 Backport-PR-URL: #14109 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax commented Jul 12, 2017
Landed in c7fb1ff |
* Fix AsyncHooksGetTriggerAsyncId such it corresponds to async_hooks.triggerAsyncId and not async_hooks.initTriggerId. * Use an async_context struct instead of two async_uid values. This change was necessary since the fixing AsyncHooksGetTriggerAsyncId otherwise makes it impossible to get the correct default trigger id. It also prevents an invalid triggerAsyncId in MakeCallback. * Rename async_uid to async_id for consistency * Rename get_uid to get_async_id * Add get_trigger_async_id to AsyncResource class PR-URL: #14040 Backport-PR-URL: #14109 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
* Fix AsyncHooksGetTriggerAsyncId such it corresponds to async_hooks.triggerAsyncId and not async_hooks.initTriggerId. * Use an async_context struct instead of two async_uid values. This change was necessary since the fixing AsyncHooksGetTriggerAsyncId otherwise makes it impossible to get the correct default trigger id. It also prevents an invalid triggerAsyncId in MakeCallback. * Rename async_uid to async_id for consistency * Rename get_uid to get_async_id * Add get_trigger_async_id to AsyncResource class PR-URL: #14040 Backport-PR-URL: #14109 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
5 participants
Backporting #14040, the first commit is a clean cherry-pick (but should be squashed together with its friend).
/cc @nodejs/async_hooks and in particular @AndreasMadsen