[v8.x] async_hooks: C++ Embedder API overhaul #14109

addaleax · 2017-07-06T17:08:45Z

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

refack · 2017-07-06T18:08:49Z

I would love to see just the squashed tests be run on v8.1.3...
So I'm thinking of a way to "retroactively" (metaphorically) add the tests to before the change. Maybe by splitting the two commits to code & tests, then squash the tests and rebase so they are the first commit (or even a separate PR).
But I'm not sure it worth messing up the commits.

addaleax · 2017-07-06T18:27:09Z

I would love to see just the squashed tests be run on v8.1.3...

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 · 2017-07-06T18:31:52Z

I would love to see just the squashed tests be run on v8.1.3...

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 ;)

Yes, also I assume they pass, I'm more thinking out loud about adding a regression test ad-hoc...
I'd be happy to do it if we agree it's worth messing with the commit order (since b14a1af424b0d4cc4ff3c9077f3eb70f73ee9d03 lands smoothly) ⚖️

AndreasMadsen · 2017-07-07T07:46:38Z

I would love to see just the squashed tests be run on v8.1.3...

Besides containing some API changes, this also contains an important fix to AsyncHooksGetTriggerAsyncId so the tests won't pass.

AndreasMadsen · 2017-07-07T07:48:26Z

src/async-wrap.cc

+
+namespace node {
+
+MaybeLocal<Value> MakeCallback(Isolate* isolate,


Why are these not in node.cc?

No particular reason, except maybe that it kind of belongs to the rest of the legacy code here.

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.

AndreasMadsen · 2017-07-07T07:49:43Z

src/async-wrap.cc

+// Undo the Node-8.x-only alias from node.h
+#undef EmitAsyncInit
+
+async_uid EmitAsyncInit(Isolate* isolate,


Should this not be async_id? Not that it really matters.

Maybe? I can change it if you like. 😄

AndreasMadsen · 2017-07-07T07:53:22Z

test/addons/async-hooks-id/binding.cc

+void EmitAsyncInit(const FunctionCallbackInfo<Value>& args) {
+  assert(args[0]->IsObject());
+  node::async_uid uid =
+      node::EmitAsyncInit(args.GetIsolate(), args[0].As<Object>(), "foobar");


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?

The EmitAsyncInit that returns async_id requires 4 parameters, correct? Or is this a test for the async_context casting?

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_uid by purpose?

Yes, everywhere else in the tests we use async_id now, so there should be one place where we don’t.

AndreasMadsen

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.

AndreasMadsen · 2017-07-07T07:57:12Z

src/node.h

+// Legacy, Node 8.x only.
+
+NODE_EXTERN
+v8::MaybeLocal<v8::Value> MakeCallback(v8::Isolate* isolate,


Should we not deprecate these too?

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 <trev.norris@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax · 2017-07-11T18:06:32Z

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

* 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 <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-07-12T13:40:46Z

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 <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added addons Issues and PRs related to native addons. async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. v8.x labels Jul 6, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Jul 6, 2017

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 6, 2017

refack approved these changes Jul 6, 2017

View reviewed changes

AndreasMadsen reviewed Jul 7, 2017

View reviewed changes

AndreasMadsen approved these changes Jul 7, 2017

View reviewed changes

AndreasMadsen reviewed Jul 7, 2017

View reviewed changes

jasnell approved these changes Jul 10, 2017

View reviewed changes

addaleax force-pushed the v8.x-staging branch from c2c7eb1 to f457b05 Compare July 11, 2017 17:57

AndreasMadsen and others added 3 commits July 11, 2017 20:05

[squash] ABI/API compat

85597ca

[squash] add deprecation warnings for deprecated MakeCallbacks

a2f61c4

addaleax force-pushed the v8.x-async-wrap-cpp-overhaul branch from 3409631 to a2f61c4 Compare July 11, 2017 18:05

addaleax closed this Jul 12, 2017

addaleax deleted the v8.x-async-wrap-cpp-overhaul branch July 12, 2017 13:41

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

addaleax mentioned this pull request Jul 21, 2017

async_hooks: remove deprecated APIs #14414

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v8.x] async_hooks: C++ Embedder API overhaul #14109

[v8.x] async_hooks: C++ Embedder API overhaul #14109

addaleax commented Jul 6, 2017

refack commented Jul 6, 2017

addaleax commented Jul 6, 2017

refack commented Jul 6, 2017

AndreasMadsen commented Jul 7, 2017

AndreasMadsen Jul 7, 2017

addaleax Jul 7, 2017

AndreasMadsen Jul 8, 2017

AndreasMadsen Jul 7, 2017

addaleax Jul 7, 2017

AndreasMadsen Jul 7, 2017

addaleax Jul 7, 2017

AndreasMadsen left a comment

AndreasMadsen Jul 7, 2017

addaleax Jul 7, 2017

addaleax commented Jul 11, 2017

addaleax commented Jul 12, 2017


		namespace node {

		MaybeLocal<Value> MakeCallback(Isolate* isolate,

[v8.x] async_hooks: C++ Embedder API overhaul #14109

[v8.x] async_hooks: C++ Embedder API overhaul #14109

Conversation

addaleax commented Jul 6, 2017

refack commented Jul 6, 2017

addaleax commented Jul 6, 2017

refack commented Jul 6, 2017

AndreasMadsen commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jul 11, 2017

addaleax commented Jul 12, 2017