async_hooks: clean up usage in internal code #18720

apapirovski · 2018-02-11T22:44:51Z

Instead of exposing internals of async_hooks & async_wrap throughout
the code base, create necessary helper functions within the internal
async_hooks that allow easy usage by Node.js internals. This stops
every single internal user of async_hooks from importing a ton of
functions, constants and internal Aliased Buffers from C++ async_wrap.

Adds functions initHooksExist, afterHooksExist, and destroyHooksExist
to determine whether the related emit methods need to be triggered.

Adds clearDefaultTriggerAsyncId and clearAsyncIdStack on the JS side
as an alternative to always calling C++.

Moves async_id_symbol and trigger_async_id_symbol to internal
async_hooks as they are never used in C++.

Adjusts usage throughout the codebase, as well as in a couple of tests.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

async_hooks

apapirovski · 2018-02-11T22:47:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13111/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1278/
Benchmark CI:
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/123/ (timers)
https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/124/ (process)

apapirovski · 2018-02-14T00:36:58Z

/cc @addaleax, @ofrobots, @AndreasMadsen — I would appreciate a review from someone involved with async hooks. Want to make sure I'm not missing anything, in particular with moving the symbols to internal/async_hooks (if they're meant to be quasi-public — as they were via process.binding — then they should probably be exported from async_hooks to be honest).

ofrobots · 2018-02-14T01:07:09Z

Apologies for the delay. I have been busy with the diagnostic summit. I will take a look tomorrow.

…

On Tue, Feb 13, 2018, 7:37 PM Anatoli Papirovski ***@***.***> wrote: /cc @addaleax <https://github.com/addaleax>, @ofrobots <https://github.com/ofrobots>, @AndreasMadsen <https://github.com/andreasmadsen> — I would appreciate a review from someone involved with async hooks. Want to make sure I'm not missing anything, in particular with moving the symbols to internal/async_hooks (if they're meant to be quasi-public — as they were via process.binding — then they should probably be exported from async_hooks to be honest). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18720 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAE0qXKnJcc94uCMWI91_eeC9ZuV2zuTks5tUirfgaJpZM4SBi6R> .

apapirovski · 2018-02-14T01:07:59Z

@ofrobots No worries :) Hope the summit was productive, disappointed I had to miss it.

ofrobots

LGTM w/ nits. Thanks for doing all this cleanup!

ofrobots · 2018-02-15T21:09:09Z

lib/async_hooks.js

-  return async_id_fields[kExecutionAsyncId];
-}
-
-


nit: executionAsyncId and triggerAsyncId should probably stay together. Either both move or neither.

Works for me. I'll move both. (I wanted to avoid requiring both files during bootstrap.)

ofrobots · 2018-02-15T21:25:38Z

lib/internal/async_hooks.js

@@ -345,6 +365,18 @@ function emitDestroyScript(asyncId) {
 }


+function clearAsyncIdStack() {


nit: needs a comment that needs to stay in sync with the C++ copy in env-inl.h (and vice versa).

Good point. Will update!

ofrobots · 2018-02-15T21:37:25Z

lib/internal/async_hooks.js

@@ -270,6 +272,11 @@ function getDefaultTriggerAsyncId() {
 }


+function clearDefaultTriggerAsyncId() {


nit: It might be worth moving the copy of this operation in env-inl.h into a standalone create_default_trigger_async_id function so that we can attach a comment in both places to keep the copies in sync.

Aside: I've wanted to cleanup the magical -1 and 0 hard-coded constants for async id, but that should probably happen in a separate PR.

I don't know if it makes sense to move it off into a function in C++ since it seems to only be done from the constructor. Perhaps a constant makes the most sense.

Could you add a comment that says that this should never be used except for exception handling?

apapirovski · 2018-02-15T22:21:46Z

@ofrobots Updated now. Thanks for the feedback!

apapirovski · 2018-02-16T13:50:16Z

@addaleax Would you mind reviewing this? Looking to land today but there's one commit that hasn't been reviewed. Thanks!

apapirovski · 2018-02-16T13:50:19Z

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

addaleax

Looks fine to me :)

AndreasMadsen · 2018-02-16T15:28:45Z

lib/internal/timers.js

  emitInit
 } = require('internal/async_hooks');
-// Grab the constants necessary for working with internal arrays.
-const { kInit, kAsyncIdCounter } = async_wrap.constants;
 // Symbols for storing async id state.
 const async_id_symbol = Symbol('asyncId');


Why do we have separate symbols for this? 😂 I think they should just be async_id_symbol and trigger_async_id_symbol from async_hooks.

Funny enough, it's actually necessary because of enroll and unenroll. Since they could use an object that already has an asyncId associated, it would overwrite it or misrepresent it in emit.

AndreasMadsen · 2018-02-16T15:30:22Z

lib/internal/async_hooks.js

    init_symbol, before_symbol, after_symbol, destroy_symbol,
    promise_resolve_symbol
  },
  enableHooks,
  disableHooks,
+  clearDefaultTriggerAsyncId,
+  clearAsyncIdStack,
+  hasAsyncIdStack,
  // Internal Embedder API
  newUid,


nit: You would be truly loved if you renamed this to newAsyncId :)

Instead of exposing internals of async_hooks & async_wrap throughout the code base, create necessary helper methods within the internal async_hooks that allows easy usage by Node.js internals. This stops every single internal user of async_hooks from importing a ton of functions, constants and internal Aliased Buffers from C++ async_wrap. Adds functions initHooksExist, afterHooksExist, and destroyHooksExist to determine whether the related emit methods need to be triggered. Adds clearDefaultTriggerAsyncId and clearAsyncIdStack on the JS side as an alternative to always calling C++. Moves async_id_symbol and trigger_async_id_symbol to internal async_hooks as they are never used in C++. Adjusts usage throughout the codebase, as well as in a couple of tests.

apapirovski · 2018-02-16T19:03:09Z

Lite CI before landing: https://ci.nodejs.org/job/node-test-pull-request-lite/175/

Instead of exposing internals of async_hooks & async_wrap throughout the code base, create necessary helper methods within the internal async_hooks that allows easy usage by Node.js internals. This stops every single internal user of async_hooks from importing a ton of functions, constants and internal Aliased Buffers from C++ async_wrap. Adds functions initHooksExist, afterHooksExist, and destroyHooksExist to determine whether the related emit methods need to be triggered. Adds clearDefaultTriggerAsyncId and clearAsyncIdStack on the JS side as an alternative to always calling C++. Moves async_id_symbol and trigger_async_id_symbol to internal async_hooks as they are never used in C++. Renames newUid to newAsyncId for added clarity of its purpose. Adjusts usage throughout the codebase, as well as in a couple of tests. PR-URL: #18720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

apapirovski · 2018-02-16T20:03:51Z

Landed in e9ac80b

MylesBorins · 2018-02-21T15:07:38Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

needs to come with #18823

Instead of exposing internals of async_hooks & async_wrap throughout the code base, create necessary helper methods within the internal async_hooks that allows easy usage by Node.js internals. This stops every single internal user of async_hooks from importing a ton of functions, constants and internal Aliased Buffers from C++ async_wrap. Adds functions initHooksExist, afterHooksExist, and destroyHooksExist to determine whether the related emit methods need to be triggered. Adds clearDefaultTriggerAsyncId and clearAsyncIdStack on the JS side as an alternative to always calling C++. Moves async_id_symbol and trigger_async_id_symbol to internal async_hooks as they are never used in C++. Renames newUid to newAsyncId for added clarity of its purpose. Adjusts usage throughout the codebase, as well as in a couple of tests. PR-URL: nodejs#18720 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins · 2018-08-07T15:14:12Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 11, 2018

apapirovski added async_wrap async_hooks Issues and PRs related to the async hooks subsystem. labels Feb 11, 2018

BridgeAR requested review from ofrobots, AndreasMadsen and addaleax February 12, 2018 10:01

jasnell approved these changes Feb 13, 2018

View reviewed changes

ofrobots approved these changes Feb 15, 2018

View reviewed changes

ofrobots reviewed Feb 15, 2018

View reviewed changes

addaleax approved these changes Feb 16, 2018

View reviewed changes

AndreasMadsen reviewed Feb 16, 2018

View reviewed changes

apapirovski added 2 commits February 16, 2018 13:09

fixup: ofrobots feedback

029cd57

apapirovski force-pushed the patch-async-should-fns branch from 13f1c56 to b400c80 Compare February 16, 2018 19:03

fixup: rename newUid to newAsyncId

2abd4a7

apapirovski force-pushed the patch-async-should-fns branch from b400c80 to 2abd4a7 Compare February 16, 2018 19:12

apapirovski closed this Feb 16, 2018

apapirovski deleted the patch-async-should-fns branch February 16, 2018 20:03

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

MylesBorins mentioned this pull request Feb 21, 2018

test: fix unrelated variable name changes #18823

Closed

3 tasks

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async_hooks: clean up usage in internal code #18720

async_hooks: clean up usage in internal code #18720

apapirovski commented Feb 11, 2018 •

edited

Loading

apapirovski commented Feb 11, 2018 •

edited

Loading

apapirovski commented Feb 14, 2018

ofrobots commented Feb 14, 2018 via email

apapirovski commented Feb 14, 2018

ofrobots left a comment

ofrobots Feb 15, 2018

apapirovski Feb 15, 2018

ofrobots Feb 15, 2018

apapirovski Feb 15, 2018

ofrobots Feb 15, 2018

apapirovski Feb 15, 2018

AndreasMadsen Feb 16, 2018

apapirovski commented Feb 15, 2018

apapirovski commented Feb 16, 2018

apapirovski commented Feb 16, 2018

addaleax left a comment

AndreasMadsen Feb 16, 2018 •

edited

Loading

apapirovski Feb 16, 2018 •

edited

Loading

AndreasMadsen Feb 16, 2018

apapirovski commented Feb 16, 2018 •

edited

Loading

apapirovski commented Feb 16, 2018

MylesBorins commented Feb 21, 2018 •

edited

Loading

MylesBorins commented Aug 7, 2018

		@@ -345,6 +365,18 @@ function emitDestroyScript(asyncId) {
		}


		function clearAsyncIdStack() {

		@@ -270,6 +272,11 @@ function getDefaultTriggerAsyncId() {
		}


		function clearDefaultTriggerAsyncId() {

async_hooks: clean up usage in internal code #18720

async_hooks: clean up usage in internal code #18720

Conversation

apapirovski commented Feb 11, 2018 • edited Loading

Checklist

Affected core subsystem(s)

apapirovski commented Feb 11, 2018 • edited Loading

apapirovski commented Feb 14, 2018

ofrobots commented Feb 14, 2018 via email

apapirovski commented Feb 14, 2018

ofrobots 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

apapirovski commented Feb 15, 2018

apapirovski commented Feb 16, 2018

apapirovski commented Feb 16, 2018

addaleax left a comment

Choose a reason for hiding this comment

AndreasMadsen Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

apapirovski Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Feb 16, 2018 • edited Loading

apapirovski commented Feb 16, 2018

MylesBorins commented Feb 21, 2018 • edited Loading

MylesBorins commented Aug 7, 2018

apapirovski commented Feb 11, 2018 •

edited

Loading

apapirovski commented Feb 11, 2018 •

edited

Loading

AndreasMadsen Feb 16, 2018 •

edited

Loading

apapirovski Feb 16, 2018 •

edited

Loading

apapirovski commented Feb 16, 2018 •

edited

Loading

MylesBorins commented Feb 21, 2018 •

edited

Loading