async_hooks: expose list of types #13610

refack · 2017-06-11T12:31:15Z

Ref: #13561 (comment)

Also refactor JS type names allocation, so it provided by asunc_hooks

/cc @nodejs/async_hooks

Checklist

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

Affected core subsystem(s)

async_hooks,timers,process

refack · 2017-06-11T12:32:31Z

WIP (needs docs, and tests)
I'm looking for early feedback.

addaleax · 2017-06-11T12:33:33Z

lib/async_hooks.js

+function _getTypeName(module, type) {
+  return Object.keys(JSSideProviders)
+    .find((k) => JSSideProviders[k] === `${module} ${type}`);
+}


This seems unnecessarily complicated.

Yeah definatly a kludge.
This is what I came up with in my current (sleep deprived) state 😧

I'm looking for a structure that:

Can get the keys out easily

can be queried for a specific name by modules and type.

I’d suggest really just listing the types for now

I have a better idea: lookup by function:
_getTypeName(setTimeout), _getTypeName(setImmediate)
that way it's also useful to end users.
The question is will it be robust (no name conflicts)

The question is will it be robust (no name conflicts)

That depends on what exactly your idea is – just have this for timers and nextTick? That doesn’t seem much more useful than listing these as values. If you’re trying to do this for generic async providers, you’ll run into the problem that many public APIs (e.g. net.Socket) can init different async types.

that way it's also useful to end users.

I wouldn’t underscore-prefix it in that case

@refack

I have a better idea: lookup by function:

As @addaleax said, if you want this to work generically only a path of pain and suffering awaits you. I spent a couple weeks trying to get this and a similar feature to work. Will never work reliably.

TBH I'm not loving adding the module name to each type. Feels like serious overkill to me.

@refack

I have a better idea: lookup by function:
As @addaleax said, if you want this to work generically only a path of pain and suffering awaits you. I spent a couple weeks trying to get this and a similar feature to work. Will never work reliably.

TBH I'm not loving adding the module name to each type. Feels like serious overkill to me.

I didn't have any bright ideas. I think that I'd invert the dependency:

function registerTypeName(type) { }

This way implementers (or node modules) could inform us of the names they choose. We could warn about possible coalitions, but that is obviously timing dependant.

jasnell · 2017-06-12T14:49:56Z

lib/async_hooks.js

@@ -480,4 +496,6 @@ module.exports = {
  emitBefore: emitBeforeS,
  emitAfter: emitAfterS,
  emitDestroy: emitDestroyS,
+  // internal
+  _getTypeName,


If this is meant for internal only, I would prefer that it be named with a Symbol rather than introducing a new _-prefixed property. Or, better yet, move it to an appropriate lib/internal location.

made it public

trevnorris · 2017-06-12T22:42:37Z

This feels too complicated. Types were always meant to be a single string. Let's not mess with that. If we absolutely want to include the module name then just add it in the string. e.g. timers/Immediate (still not a fan of this, but maybe could be convinced otherwise)

Though this PR gives me another idea. I like getTypes(), and possibly we include registerType() for users of AsyncResource(). Could use it to make sure two module authors don't have a collision of names or such. But that's outside the scope of this PR.

trevnorris · 2017-06-12T22:44:12Z

@refack

This way implementers (or node modules) could inform us of the names they choose. We could warn about possible coalitions, but that is obviously timing dependant.

Holy hell, did you read my mind as I was typing out my last comment?

addaleax · 2017-06-12T22:46:37Z

Tbh, just export Object.keys(binding.Providers).concat(['TickObject', 'Timeout', 'Immediate']) on the module, document it, test it, done; it doesn’t always need to be made more complicated than it is.

refack · 2017-06-12T22:55:05Z

IMHO current imp is nice and compact.

Tbh, just export Object.keys(binding.Providers).concat(['TickObject', 'Timeout', 'Immediate']) on the module, document it, test it, done; it doesn’t always need to be made more complicated than it is.

Could go K.I.S.S. if current doesn't make sense...

refack · 2017-06-12T22:57:12Z

Could use it to make sure two module authors don't have a collision of names or such. But that's outside the scope of this PR.

The copy and paste of 'TickObject', 'Timeout', 'Immediate' gave me a creepy crawly feeling, so I did the extra 6 lines.

addaleax

Yea, this is much better.

addaleax · 2017-06-12T22:58:25Z

lib/timers.js

@@ -190,7 +192,8 @@ function insert(item, unrefed) {
    item[async_id_symbol] = ++async_uid_fields[kAsyncUidCntr];
    item[trigger_id_symbol] = initTriggerId();
    if (async_hook_fields[kInit] > 0)
-      emitInit(item[async_id_symbol], 'Timeout', item[trigger_id_symbol], item);
+      emitInit(item[async_id_symbol], asyncTypeTO, item[trigger_id_symbol],


These changes are still confusing, imho. In any case, if you want to keep them, name them kTimeoutType or similar since they are constants. :)

* refactor JS type names allocation

trevnorris · 2017-06-12T23:07:59Z

lib/async_hooks.js

+
+function registerTypeName(type) {
+  if (JSProviders.has(type))
+    throw new errors.TypeError('ERR_ASYNC_PROVIDER_NAME', type);


here's some insanity, but could be useful for debugging:

const JSProviders = new Map(); function registerTypeName(type) { if (JSProviders.has(type)) throw new errors.TypeError('ERR_ASYNC_PROVIDER_NAME', type); // include the old stack somehow JSProviders.set(type, new Error().stack); }

Not suggesting we implement this, but thought something like it could be helpful for identifying the bugger who is using my type name.

could do:

const JSProviders = new Map(); function registerTypeName(type) { if (JSProviders.has(type)) throw JSProviders.get(type); JSProviders.set(type, new errors.TypeError('ERR_ASYNC_PROVIDER_NAME', type)); }

Will throw weird errors, good for the AsyncResource implementer, bad for a 6-degrees-of-separation user

trevnorris

I worry about the overhead of getTypes() (I've used it before in init()), but will worry about improving that later. Just don't forget to name the constants for timers and nexttick. Thanks!

refack · 2017-06-12T23:30:31Z

Quick check: https://ci.nodejs.org/job/node-test-commit-linuxone/6574/

refack · 2017-06-13T00:57:07Z

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

TimothyGu · 2017-06-13T01:26:14Z

doc/api/async_hooks.md

+added: REPLACEME
+-->
+
+* Returns: `{Array}` list of async resource type names


The "list of async resource type names" is redundant considering it is documented two lines below.

TimothyGu · 2017-06-13T01:26:16Z

doc/api/async_hooks.md

+added: REPLACEME
+-->
+
+* Returns: `{Array}` list of async resource type names


The "list of async resource type names" is redundant considering it is documented two lines below.

Some note about why these are useful would be helpful here.

TimothyGu · 2017-06-13T01:26:35Z

doc/api/async_hooks.md

@@ -428,6 +438,13 @@ const server = net.createServer(function onConnection(conn) {
 });
 ```

+#### `async_hooks.registerTypeName(type)`


added YAML annotation needed.

TimothyGu · 2017-06-13T01:29:15Z

doc/api/async_hooks.md

+#### `async_hooks.registerTypeName(type)`
+
+* `type` {string} a new type of async resource
+* Returns {string} the type name to use


Why not just return undefined? The way it is documented can be interpreted to imply some processing has been done on type, while it is actually returned verbatim.

I find it convenient to do the same as is also already in this PR:

const kTickObjectType = async_hooks.registerTypeName('TickObject');

TimothyGu · 2017-06-13T01:29:34Z

lib/async_hooks.js

@@ -461,6 +462,18 @@ function init(asyncId, type, triggerId, resource) {
  processing_hook = false;
 }

+const JSProviders = new Set();
+function registerTypeName(type) {
+  if (JSProviders.has(type))


Maybe we should make sure type isn't already contained in async_wrap.Providers either?

TimothyGu · 2017-06-13T01:30:18Z

lib/async_hooks.js

+
+function getTypes() {
+  return Object.keys(async_wrap.Providers)
+    .concat(...JSProviders.keys());


No need for .keys(). Set's iteration protocol returns its contents by default.

This is certainly not the fastest method, but given that this shouldn't be a hot code path that should be ok

TimothyGu · 2017-06-13T01:31:04Z

doc/api/async_hooks.md

@@ -131,6 +131,16 @@ provided by AsyncHooks itself. The logging should then be skipped when
 it was the logging itself that caused AsyncHooks callback to call. By
 doing this the otherwise infinite recursion is broken.

+#### `async_hooks.getTypes()`


Alphabetical order.

TimothyGu · 2017-06-13T01:34:56Z

lib/async_hooks.js

@@ -461,6 +462,18 @@ function init(asyncId, type, triggerId, resource) {
  processing_hook = false;
 }

+const JSProviders = new Set();
+function registerTypeName(type) {


I'd like to see a type check here, ensuring type is a string.

jasnell · 2017-06-13T15:52:12Z

doc/api/async_hooks.md

@@ -131,6 +131,16 @@ provided by AsyncHooks itself. The logging should then be skipped when
 it was the logging itself that caused AsyncHooks callback to call. By
 doing this the otherwise infinite recursion is broken.

+#### `async_hooks.getTypes()`
+


Unnecessary whitespace before the yaml comment

AndreasMadsen · 2017-07-05T20:11:47Z

If this is still something we like to do, I think we should validate the type name in the AsyncResource constructor. Otherwise, there is no way to enforce this. Depending on where we are on breaking changes in experimental API that might have to be semver-major change. But we can then backport this to v8.x, just without the assertion.
I don't see any reason to make a specific JSProvider set, just initialize the set with async_wrap.Providers. That should make getTypes and the suggested assertion easier.
What is the use case for getTypes, is this something we really need? If not I would like to keep the API exposure minimal, it is so much easier to add this stuff later.

trevnorris · 2017-07-06T10:25:21Z

@AndreasMadsen I like the idea of being able to register a type name and having it error if the name has already been registered. Though I also think the file name where the name was registered should be in the error message to help developers identify where it's happening (can see myself freaking out not being able to identify which dependency is using a specific name).

I'm impartial to how the type names are accessed, but it does seem that adding values to .Providers should be enough IMO.

AndreasMadsen · 2017-07-06T10:49:34Z

I like the idea of being able to register a type name and having it error if the name has already been registered. Though I also think the file name where the name was registered should be in the error message to help developers identify where it's happening (can see myself freaking out not being able to identify which dependency is using a specific name).

Completely agree. I'm just saying that it has little value if the user can just pass whatever value they want in AsyncResource. Thus we also need to validate that the type input to AsyncResource is registered.

BridgeAR · 2017-09-19T22:45:59Z

@refack this needs a rebase, the comments should be addressed and I am not sure if this is in a state that would land as is. Would you mind following up on this?

BridgeAR · 2017-09-27T22:29:45Z

Ping @refack

BridgeAR · 2017-11-22T11:42:43Z

Closing due to long inactivity. @refack if you want to pursue this further, please feel free to reopen!

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Jun 11, 2017

refack added the wip Issues and PRs that are still a work in progress. label Jun 11, 2017

refack mentioned this pull request Jun 11, 2017

doc: update async_hooks providers list #13561

Closed

2 tasks

tniessen added the process Issues and PRs related to the process subsystem. label Jun 11, 2017

addaleax reviewed Jun 11, 2017

View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 12, 2017

jasnell reviewed Jun 12, 2017

View reviewed changes

addaleax approved these changes Jun 12, 2017

View reviewed changes

addaleax reviewed Jun 12, 2017

View reviewed changes

refack added 2 commits June 12, 2017 19:04

async_hooks: expose list of types

c81e975

* refactor JS type names allocation

invert dependency from _getTypeName to registerTypeName

2813840

trevnorris reviewed Jun 12, 2017

View reviewed changes

trevnorris approved these changes Jun 12, 2017

View reviewed changes

refack added 2 commits June 12, 2017 19:29

a spot of cleanup

df0a172

test & doc

65b17b9

refack force-pushed the async-types-list branch from a8a9744 to 65b17b9 Compare June 12, 2017 23:29

refack added 3 commits June 12, 2017 20:12

senior moment with test code

8f5d972

changes error

b77ba7a

that's it I'm moving to ubuntu

3808174

refack removed the wip Issues and PRs that are still a work in progress. label Jun 13, 2017

TimothyGu suggested changes Jun 13, 2017

View reviewed changes

jasnell reviewed Jun 13, 2017

View reviewed changes

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

AndreasMadsen mentioned this pull request Aug 31, 2017

async_hooks: docs and api ergonomics tweaks #15103

Closed

4 tasks

refack self-assigned this Sep 23, 2017

BridgeAR closed this Nov 22, 2017

refack removed their assignment Oct 12, 2018

refack added the stalled Issues and PRs that are stalled. label Oct 29, 2018

async_hooks: expose list of types #13610

async_hooks: expose list of types #13610

Conversation

refack commented Jun 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Jun 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jun 12, 2017 • edited Loading

trevnorris commented Jun 12, 2017

addaleax commented Jun 12, 2017

refack commented Jun 12, 2017

refack commented Jun 12, 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

refack Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

trevnorris left a comment • edited Loading

Choose a reason for hiding this comment

refack commented Jun 12, 2017

refack commented Jun 13, 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

TimothyGu Jun 13, 2017 • edited Loading

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 5, 2017

trevnorris commented Jul 6, 2017

AndreasMadsen commented Jul 6, 2017

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 27, 2017

BridgeAR commented Nov 22, 2017

refack commented Jun 11, 2017 •

edited

Loading

trevnorris commented Jun 12, 2017 •

edited

Loading

refack Jun 12, 2017 •

edited

Loading

trevnorris left a comment •

edited

Loading

TimothyGu Jun 13, 2017 •

edited

Loading