async_hooks: C++ Embedder API overhaul #14040

AndreasMadsen · 2017-07-02T12:48:41Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

I'm not sure how to split up the AsyncHooksGetTriggerAsyncId and the async_context commit. If someone has a specific suggestion I would be happy to do it.

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.

/cc @nodejs/async_hooks

AndreasMadsen · 2017-07-02T12:52:13Z

I think this is a semver-minor since it only touches async_hooks experimental APIs.

refack · 2017-07-02T13:03:37Z

Does this solve nodejs/help#644?

addaleax · 2017-07-02T13:06:06Z

@refack It’s unlikely, and it’s not really clear what the problem in that issue is in the first place.

refack · 2017-07-02T13:08:27Z

@refack It’s unlikely, and it’s not really clear what the problem in that issue is in the first place.

AFAICT the issue there is Environment::AsyncHooks::ExecScope being internal, but necessary in order to start the loop.

addaleax · 2017-07-02T13:10:08Z

@refack Can we keep the discussion for that issue in that issue? What you’re saying sounds reasonable, could maybe post a minimal example?

refack · 2017-07-02T13:11:26Z

@refack Can we keep the discussion for that issue in that issue? What you’re saying sounds reasonable, could maybe post a minimal example?

Sure.

addaleax

Mostly looks good from a quick glance!

addaleax · 2017-07-02T13:02:38Z

src/node.h

@@ -517,7 +517,11 @@ typedef void (*promise_hook_func) (v8::PromiseHookType type,
                                   v8::Local<v8::Value> parent,
                                   void* arg);

-typedef double async_uid;
+typedef double async_id;
+typedef struct async_context {


Fwiw you don’t need a typedef, just struct async_context { ... }; will work fine.

addaleax · 2017-07-02T13:22:50Z

src/node.h

+NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate,
+                                        v8::Local<v8::Object> resource,
+                                        const char* name,
+                                        async_id trigger_async_id = -1);


It seems to me like this breakage would be shim-able… If that’s right, I’d say we label this dont-land and I can do a non-breaking backport to v8.x, if you like

That would be awesome. In particular, I couldn't see how to make EmitAsyncInit backward compatible.

The old API required trigger_async_id instead of leaving it optional, so we could make it return a pair based on that parameter, right?

What should EmitAsyncInit then return when trigger_async_id is specified?

I don’t really understand the question, sorry. Basically, what we need to shim is the old definition

async_uid EmitAsyncInit(v8::Isolate* isolate, v8::Local<v8::Object> resource, const char* name, async_uid trigger_async_id);

in terms of the new one in this PR, right?

So what’s speaking against OldStyleEmitAsyncInit(resource, name, trigger_async_id) := (NewStyleAsyncInit(resource, name, trigger_async_id)).async_id_?

Or are you thinking about the return type? The hacky solution would be to enable casting from async_context to async_id by returning async_id_ (and immediately deprecating that), it’s not pretty but as long as it doesn’t end up in master I’m okay with that.

(One more thing: I’m noticing, maybe we should not underscore-suffix the fields of async_context given that they are public, not internal…)

Or are you thinking about the return type? The hacky solution would be to enable casting from async_context to async_id by returning async_id_ (and immediately deprecating that), it’s not pretty but as long as it doesn’t end up in master I’m okay with that.

Yes, it is the return type. The casting makes sense.

(One more thing: I’m noticing, maybe we should not underscore-suffix the fields of async_context given that they are public, not internal…)

Right. Please check ffdd431d7d42efe939f3cc378814844b5796e4b5.

AndreasMadsen · 2017-07-03T09:26:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8949/

addaleax

Once again thank you for all your hard work. LGTM!

AndreasMadsen · 2017-07-03T20:43:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8968/

trevnorris · 2017-07-03T22:56:08Z

src/node.h

    }

    v8::Local<v8::Object> get_resource() {
      return resource_.Get(isolate_);
    }

-    async_uid get_uid() const {
-      return uid_;
+    async_id get_uid() const {


For the sake of keeping things the same, want to rename this to get_async_id()?

AndreasMadsen · 2017-07-05T14:12:38Z

@trevnorris I renamed the method and added a deprecated alias.

CI: https://ci.nodejs.org/job/node-test-pull-request/8989/

* 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 async_hooks: fix AsyncHooksGetTriggerAsyncId

AndreasMadsen · 2017-07-05T19:53:50Z

Rebased and added get_trigger_async_id to match the JavaScript version of AsyncResource.

CI: https://ci.nodejs.org/job/node-test-pull-request/8993/

addaleax

Just making it it explicit that this still LGTM

AndreasMadsen · 2017-07-06T06:24:24Z

landed in c6ce500

refack

An After the fact 👍

* 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]>

* 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]>

nodejs-github-bot added async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Jul 2, 2017

addaleax reviewed Jul 2, 2017

View reviewed changes

AndreasMadsen added the dont-land-on-v8.x label Jul 2, 2017

addaleax approved these changes Jul 2, 2017

View reviewed changes

AndreasMadsen added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 3, 2017

addaleax approved these changes Jul 3, 2017

View reviewed changes

trevnorris reviewed Jul 3, 2017

View reviewed changes

AndreasMadsen added 9 commits July 5, 2017 21:40

[squash] use context struct and fix AsyncResources

df876b8

[squash] rename async_uid to async_id

1a6e24d

[squash] minor cleanup

c7c441e

[squash] cleanup struct def

89e7ccd

[squash] fix lint issue

e78d988

[squash] make async_context properties public

9aa3ba2

[squash] rename get_uid to get_async_id

98b5e9b

[squash] add AsyncResource::get_trigger_async_id

48ef6a2

addaleax approved these changes Jul 5, 2017

View reviewed changes

trevnorris approved these changes Jul 5, 2017

View reviewed changes

AndreasMadsen closed this Jul 6, 2017

refack reviewed Jul 6, 2017

View reviewed changes

addaleax mentioned this pull request Jul 6, 2017

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

Closed

addaleax added backported-to-v8.x and removed dont-land-on-v8.x labels Jul 6, 2017

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

Flarna mentioned this pull request Feb 26, 2018

async_hooks in node 8 nodejs/nan#739

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: C++ Embedder API overhaul #14040

async_hooks: C++ Embedder API overhaul #14040

AndreasMadsen commented Jul 2, 2017 •

edited

Loading

AndreasMadsen commented Jul 2, 2017 •

edited

Loading

refack commented Jul 2, 2017

addaleax commented Jul 2, 2017

refack commented Jul 2, 2017

addaleax commented Jul 2, 2017

refack commented Jul 2, 2017

addaleax left a comment

addaleax Jul 2, 2017

addaleax Jul 2, 2017

AndreasMadsen Jul 2, 2017

addaleax Jul 2, 2017

AndreasMadsen Jul 3, 2017

addaleax Jul 3, 2017

AndreasMadsen Jul 3, 2017

AndreasMadsen commented Jul 3, 2017 •

edited

Loading

addaleax left a comment

AndreasMadsen commented Jul 3, 2017

trevnorris Jul 3, 2017

AndreasMadsen commented Jul 5, 2017 •

edited

Loading

AndreasMadsen commented Jul 5, 2017

addaleax left a comment

AndreasMadsen commented Jul 6, 2017

refack left a comment

async_hooks: C++ Embedder API overhaul #14040

async_hooks: C++ Embedder API overhaul #14040

Conversation

AndreasMadsen commented Jul 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

AndreasMadsen commented Jul 2, 2017 • edited Loading

refack commented Jul 2, 2017

addaleax commented Jul 2, 2017

refack commented Jul 2, 2017

addaleax commented Jul 2, 2017

refack commented Jul 2, 2017

addaleax left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 3, 2017 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 3, 2017

Choose a reason for hiding this comment

AndreasMadsen commented Jul 5, 2017 • edited Loading

AndreasMadsen commented Jul 5, 2017

addaleax left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 6, 2017

refack left a comment

Choose a reason for hiding this comment

AndreasMadsen commented Jul 2, 2017 •

edited

Loading

AndreasMadsen commented Jul 2, 2017 •

edited

Loading

AndreasMadsen commented Jul 3, 2017 •

edited

Loading

AndreasMadsen commented Jul 5, 2017 •

edited

Loading