AsyncWrap external API improvements #3461

trevnorris · 2015-10-20T23:05:18Z

Few improvements to make the AsyncWrap external API more usable. These include:

Hooks are optional (except the init hook).
All AsyncWrap instances receive a unique id.
Call callback from the AsyncWrap destructor.

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

trevnorris · 2015-10-20T23:06:04Z

src/async-wrap.cc

-  env->set_async_hooks_pre_function(args[1].As<Function>());
-  env->set_async_hooks_post_function(args[2].As<Function>());
+  if (args[0]->IsFunction())
+    env->set_async_hooks_init_function(args[0].As<Function>());


Since the init callback is always necessary, will change this to fail if init callback is not passed.

jasnell · 2015-10-21T00:33:54Z

semver-minor or major?

thefourtheye · 2015-10-21T03:38:23Z

src/async-wrap.cc

-  env->set_async_hooks_init_function(args[0].As<Function>());
-  env->set_async_hooks_pre_function(args[1].As<Function>());
-  env->set_async_hooks_post_function(args[2].As<Function>());
+  if (args[0]->IsFunction())


Earlier if the arguments are not functions, it will throw. This change will ignore the values if they are not functions. Is that okay?

Edit: Ah, I see it now. This is the first point in the PR description.

thefourtheye · 2015-10-21T03:41:32Z

Why do we need the unique ids?

rvagg · 2015-10-21T03:46:09Z

re semver—so far we've been treating AsyncWrap changes as neither major nor minor but getting all changes in to patch releases because it's sort of in "unofficial" land, undocumented anyway

at some point we have to grow out of that though! @trevnorris do you have a feel for maturity of AsyncWrap? We can't leave it in limbo, it needs to be an official API at some point.

trevnorris · 2015-10-21T07:14:59Z

@thefourtheye For the new destructed callback. We can't return the instance like we do in the others, so instead we give the id of the instance being destroyed.

@rvagg My recent flurry of PRs regarding async wrap is to address this. I feel like it's almost ready to be made publicly experimental API, but there is still some cleanup that needs to be done. e.g. it can't simply abort if something goes wrong. like right now if any of the callbacks throw the program will simply crash. I did that b/c I honestly have no idea what will happen to the application if execution proceeds after an exception.

There's also the case of timers. Currently none of these are being tracked. While this could be delayed until after making the API public, I have a couple implementation ideas to try out first.

trevnorris · 2015-10-21T21:43:18Z

Added test and rebased.

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

rvagg · 2015-10-25T18:09:35Z

needs someone more qualified than me to review, @nodejs/tracing, @bnoordhuis or @thlorenz perhaps?

AndreasMadsen · 2015-10-26T10:12:14Z

src/async-wrap.cc

@@ -103,6 +103,9 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) {

 static void EnableHooksJS(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);
+  Local<Function> init_fn = env->async_hooks_init_function();
+  if (init_fn.IsEmpty() || !init_fn->IsFunction())


why is the function check required? In the reset of the code only init_fn.IsEmpty() is used to check of async-wrap is enabled. The SetupHooks function also checks that init_fn is a function.

Currently in AsyncWrap::AsyncWrap we check if the init hook is set. If not then it returns early. If this is the case then bits_ will never be set and no additional callback will ever be called for that object as a result. I can probably rethink the logic, but this seemed the most straight forward for this PR.

AndreasMadsen · 2015-10-26T10:19:12Z

Very awesome with the destructor hook. But how does this fit into the tick/turn logic, does it follow immediately after the post hook in the oncomplete cases?

trevnorris · 2015-10-26T16:48:39Z

how does this fit into the tick/turn logic, does it follow immediately after the post hook in the oncomplete cases?

Doesn't have a specific timing. Sometimes the handle is cleaned up on a close() call, and other times it's cleaned up whenever GC gets around to it. It fires whenever the class instance is being destructed, and that can be done manually or be handled automatically.

indutny · 2015-11-06T22:59:59Z

LGTM, if CI is green

trevnorris · 2015-11-06T23:05:12Z

One final CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/681/

Only enforce that the init callback is passed to setupHooks(). The remaining hooks can be optionally passed. Throw if async_wrap.enable() runs before setting the init callback or if setupHooks() is called while async wrap is enabled. Add test to verify calls throw appropriately. PR-URL: nodejs#3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

New instances of AsyncWrap are automatically assigned a unique id. The value will be used in future commits to communicate additional information via the async hooks. While the largest value we can reliably communicate to JS is 2^53, even if a new AsyncWrap is created every 100ns the uid won't reach its end for 28.5 years. PR-URL: nodejs#3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Call a user's callback to notify that the handle has been destroyed. Only pass the id of the AsyncWrap instance since the object no longer exists. The object that's being destructed should never be inspected within the callback or any time afterward. This commit make a breaking change. The init callback will now be passed arguments in the order of provider, id, parent. PR-URL: nodejs#3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

trevnorris · 2015-11-06T23:40:59Z

All CI failures were Jenkins related.

Landed in a4e9487, 80a66ba and bb1bd76.

trevnorris · 2015-11-06T23:42:29Z

@jasnell Thoughts on LTS worthy? Since this is technically internal API still, no API breakage.

Only enforce that the init callback is passed to setupHooks(). The remaining hooks can be optionally passed. Throw if async_wrap.enable() runs before setting the init callback or if setupHooks() is called while async wrap is enabled. Add test to verify calls throw appropriately. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

New instances of AsyncWrap are automatically assigned a unique id. The value will be used in future commits to communicate additional information via the async hooks. While the largest value we can reliably communicate to JS is 2^53, even if a new AsyncWrap is created every 100ns the uid won't reach its end for 28.5 years. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Call a user's callback to notify that the handle has been destroyed. Only pass the id of the AsyncWrap instance since the object no longer exists. The object that's being destructed should never be inspected within the callback or any time afterward. This commit make a breaking change. The init callback will now be passed arguments in the order of provider, id, parent. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins · 2015-11-30T22:13:40Z

@jasnell @rvagg thoughts on lts? Don't feel qualified to make this decision

fwiw the commits all land cleanly and test / test-addons pass

rvagg · 2015-11-30T22:18:21Z

I'm a tentative +1 on getting the asyncwrap improvements into v4.x but I think this might be worth discussing in either a CTC or LTS WG meeting because the API is getting close to being finalised and documented, how far do we go in keeping it updated in LTS? It's useful but mostly a background feature for everyone but those in the know.

MylesBorins · 2015-12-15T00:10:31Z

@rvagg @trevnorris was a decision reached regarding landing this on 4.x?

rvagg · 2015-12-15T05:58:34Z

I can't recall exactly but I'm pretty sure we agreed to land AsyncWrap functionality on to v4 but to stop short of announcing it as a major addition—we really just want to get parity where it gets to before it's announced as public in v5/v6 so we don't risk shipping a 1/2 baked version that's not compatible with anything else and is simply technical debt in v4.

Only enforce that the init callback is passed to setupHooks(). The remaining hooks can be optionally passed. Throw if async_wrap.enable() runs before setting the init callback or if setupHooks() is called while async wrap is enabled. Add test to verify calls throw appropriately. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

New instances of AsyncWrap are automatically assigned a unique id. The value will be used in future commits to communicate additional information via the async hooks. While the largest value we can reliably communicate to JS is 2^53, even if a new AsyncWrap is created every 100ns the uid won't reach its end for 28.5 years. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Call a user's callback to notify that the handle has been destroyed. Only pass the id of the AsyncWrap instance since the object no longer exists. The object that's being destructed should never be inspected within the callback or any time afterward. This commit make a breaking change. The init callback will now be passed arguments in the order of provider, id, parent. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

MylesBorins · 2015-12-29T21:37:33Z

landed in v4.x-staging as ae46a23...dab862f

If there are any concerns about this being in LTS please let me know and I'll back the commits out.

AndreasMadsen · 2015-12-29T22:06:43Z

@thealphanerd This was discussed in nodejs/Release#59 (comment)

CTC discussion suggested that AsyncWrap should be brought up to parity with the final version, just not announced as a public and supported API in v4.

Only enforce that the init callback is passed to setupHooks(). The remaining hooks can be optionally passed. Throw if async_wrap.enable() runs before setting the init callback or if setupHooks() is called while async wrap is enabled. Add test to verify calls throw appropriately. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

New instances of AsyncWrap are automatically assigned a unique id. The value will be used in future commits to communicate additional information via the async hooks. While the largest value we can reliably communicate to JS is 2^53, even if a new AsyncWrap is created every 100ns the uid won't reach its end for 28.5 years. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

Call a user's callback to notify that the handle has been destroyed. Only pass the id of the AsyncWrap instance since the object no longer exists. The object that's being destructed should never be inspected within the callback or any time afterward. This commit make a breaking change. The init callback will now be passed arguments in the order of provider, id, parent. PR-URL: #3461 Reviewed-By: Fedor Indutny <fedor@indutny.com>

trevnorris added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 20, 2015

trevnorris reviewed Oct 20, 2015
View reviewed changes

thefourtheye reviewed Oct 21, 2015
View reviewed changes

trevnorris force-pushed the async-id-destruct branch from 20c95fd to c11d56f Compare October 21, 2015 21:42

trevnorris force-pushed the async-id-destruct branch from c11d56f to f8a3b16 Compare October 22, 2015 17:45

AndreasMadsen reviewed Oct 26, 2015
View reviewed changes

AndreasMadsen mentioned this pull request Oct 26, 2015

[async_hooks] tracking issue nodejs/diagnostics#29

Closed

27 tasks

trevnorris force-pushed the async-id-destruct branch from f8a3b16 to 1144ed2 Compare October 26, 2015 19:32

trevnorris force-pushed the async-id-destruct branch from 1144ed2 to 325cb95 Compare November 6, 2015 23:01

trevnorris force-pushed the async-id-destruct branch from 325cb95 to 8983ab4 Compare November 6, 2015 23:13

trevnorris added 3 commits November 6, 2015 16:36

trevnorris force-pushed the async-id-destruct branch from 8983ab4 to bb1bd76 Compare November 6, 2015 23:38

trevnorris merged commit bb1bd76 into nodejs:master Nov 6, 2015

trevnorris deleted the async-id-destruct branch November 6, 2015 23:42

This was referenced Nov 10, 2015

Release proposal: v5.1.0 #3700

Closed

Propose v5.1.0 #3736

Merged

jasnell added the lts-watch-v4.x label Nov 13, 2015

MylesBorins mentioned this pull request Nov 17, 2015

v4.2.2+1 commit candidates nodejs/Release#57

Closed

rvagg mentioned this pull request Nov 30, 2015

AsyncWrap for LTS Argon nodejs/Release#59

Closed

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

AndreasMadsen mentioned this pull request Dec 28, 2015

Async wrap update - destroy hook and uid nodejs/diagnostics#37

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

AndreasMadsen mentioned this pull request Jan 4, 2016

AsyncWrap init hook threw AndreasMadsen/trace#19

Closed

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AsyncWrap external API improvements #3461

AsyncWrap external API improvements #3461

trevnorris commented Oct 20, 2015

trevnorris Oct 20, 2015

jasnell commented Oct 21, 2015

thefourtheye Oct 21, 2015

thefourtheye commented Oct 21, 2015

rvagg commented Oct 21, 2015

trevnorris commented Oct 21, 2015

trevnorris commented Oct 21, 2015

rvagg commented Oct 25, 2015

AndreasMadsen Oct 26, 2015

trevnorris Oct 26, 2015

AndreasMadsen commented Oct 26, 2015

trevnorris commented Oct 26, 2015

indutny commented Nov 6, 2015

trevnorris commented Nov 6, 2015

trevnorris commented Nov 6, 2015

trevnorris commented Nov 6, 2015

MylesBorins commented Nov 30, 2015

rvagg commented Nov 30, 2015

MylesBorins commented Dec 15, 2015

rvagg commented Dec 15, 2015

MylesBorins commented Dec 29, 2015

AndreasMadsen commented Dec 29, 2015

AsyncWrap external API improvements #3461

AsyncWrap external API improvements #3461

Conversation

trevnorris commented Oct 20, 2015

trevnorris Oct 20, 2015

Choose a reason for hiding this comment

jasnell commented Oct 21, 2015

thefourtheye Oct 21, 2015

Choose a reason for hiding this comment

thefourtheye commented Oct 21, 2015

rvagg commented Oct 21, 2015

trevnorris commented Oct 21, 2015

trevnorris commented Oct 21, 2015

rvagg commented Oct 25, 2015

AndreasMadsen Oct 26, 2015

Choose a reason for hiding this comment

trevnorris Oct 26, 2015

Choose a reason for hiding this comment

AndreasMadsen commented Oct 26, 2015

trevnorris commented Oct 26, 2015

indutny commented Nov 6, 2015

trevnorris commented Nov 6, 2015

trevnorris commented Nov 6, 2015

trevnorris commented Nov 6, 2015

MylesBorins commented Nov 30, 2015

rvagg commented Nov 30, 2015

MylesBorins commented Dec 15, 2015

rvagg commented Dec 15, 2015

MylesBorins commented Dec 29, 2015

AndreasMadsen commented Dec 29, 2015