async_hooks: reduce code duplication by using a factory by BridgeAR · Pull Request #13755 · nodejs/node

BridgeAR · 2017-06-18T01:33:15Z

The difference between the hooks is only a single variable and it can be initialized in the beginning. So I think it's nicer to stick to DRY.
The init function is also very similar but that would require an additional check, so I kept it as it is.

I also added a TODO entry as the comment somewhat confused me (ping @trevnorris ).

Checklist

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

Affected core subsystem(s)

async_hooks

mscdex · 2017-06-18T03:02:48Z

lib/async_hooks.js

Why is this still called emitAfterN?

cjihrig · 2017-06-18T21:35:06Z

lib/async_hooks.js

This line is over 80 characters.

trevnorris

Minor change request, but other than that LGTM

trevnorris · 2017-06-21T08:27:03Z

lib/async_hooks.js

 const before_symbol = Symbol('before');
 const after_symbol = Symbol('after');
 const destroy_symbol = Symbol('destroy');
+const emitBeforeN = hookFactory(before_symbol);


nit: change this to hookFactoryEmit or emitHookFactory.

@trevnorris PTAL...

trevnorris · 2017-06-21T08:27:46Z

lib/async_hooks.js


-  // I'd prefer allowing these checks to not exist, or only throw in a debug
-  // build, in order to improve performance.
+  // TODO(trevnorris): I'd prefer allowing these checks to not exist, or only


good call changing this to a TODO.

trevnorris · 2017-06-21T08:29:05Z

lib/async_hooks.js

+function hookFactory(symbol) {
+  // Called from native. The asyncId stack handling is taken care of there
+  // before this is called.
+  return function(asyncId) {


Have we verified how this'll change the stack trace from any of these? If V8 can properly infer the name based on the variable name then great.

It does that on the newer v8 versions:

> const a = { foo: function () {}, bar() {}, baz: () => {} } undefined > a.foo.name 'foo' > a.bar.name 'bar' > a.baz.name 'baz'

But I am not certain if there might be a context in which it is not inferred. I am also not sure where to trigger a stack trace from one of these functions as I didn't look deep into the async_hooks code so far. Can you point me out to something or give me a small example?

I could enforce setting the name though.

function hookFactory(symbol, name) { const fn = function () {} Object.defineProperty(fn, 'name', { value: name }) return fn } const hook = hookFactory(Symbol(), 'myName')

The stack trace was changed so I now explicitly set the name and it's now looking good again.

Error at AsyncHook.before (repl:2:26) at emitBeforeN (async_hooks.js:358:40) at emitBeforeS (async_hooks.js:393:3) at nextTickEmitBefore (internal/process/next_tick.js:158:7) at process._tickDomainCallback (internal/process/next_tick.js:230:9)

BridgeAR · 2017-06-23T21:55:07Z

PTAL

refack

LGTM
1 question

refack · 2017-06-23T22:06:25Z

lib/async_hooks.js

 const before_symbol = Symbol('before');
 const after_symbol = Symbol('after');
 const destroy_symbol = Symbol('destroy');
+const emitBeforeN = emitHookFactory(before_symbol);


Question: is there a need to declare these? Stack beauty?
Otherwise could just be used in L#61

Only the emitDestroyN hook is used only once, the other two are used in more than one spot. I made them all const for consistency.

trevnorris · 2017-06-29T23:40:08Z

lib/async_hooks.js

+        }
      }
+    } catch (e) {
+      fatalError(e);


You asked how to reach this fatalError call. The following will do it:

require('async_hooks').createHook({ before() { throw new Error('ah crud') }, }).enable(); setTimeout(() => {}, 10);

BridgeAR · 2017-06-30T12:40:45Z

Comment addressed

AndreasMadsen · 2017-07-03T08:33:21Z

lib/async_hooks.js

 const before_symbol = Symbol('before');
 const after_symbol = Symbol('after');
 const destroy_symbol = Symbol('destroy');
+const emitBeforeN = emitHookFactory(before_symbol, 'emitBeforeN');


Could you change the N to Native while you are at it? That short naming convention really annoys me.

I guess in that case the the S should also be renamed but I can't think of what it stands for right now. Does it stand for Script?

AndreasMadsen · 2017-07-03T08:35:39Z

lib/async_hooks.js

+      restoreTmpHooks();
+    }
+  };
+  Object.defineProperty(fn, 'name', {


Mind leaving a comment that explains this sets the anonymous function name such it looks good in stack traces.

AndreasMadsen · 2017-07-03T10:58:56Z

@trevnorris why is restoreTmpHooks(); not called in init?

restoreTmpHooks(); is called in emitInitS, so I think it can/should be moved to init. This would allow us to factorize init too.

@BridgeAR If the above is true, please also rename init to emitInitNative.

BridgeAR · 2017-07-03T12:14:44Z

@AndreasMadsen I'm not sure if we can factorize init as it calls the hooks with asyncId, type, triggerAsyncId and resource and not only the id. And if this is a hot code path adding a if in the loop might slow things down.
Only moving the restoreTmpHooks(); might work though, at least the tests pass after doing so.

AndreasMadsen · 2017-07-03T12:24:33Z

I'm not sure if we can factorize init as it calls the hooks with asyncId, type, triggerAsyncId and resource and not only the id. And if this is a hot code path adding a if in the loop might slow things down.

Haha, completly forgot about that, never mind :)

AndreasMadsen · 2017-07-03T12:25:18Z

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

AndreasMadsen

LGTM

AndreasMadsen · 2017-07-05T13:53:21Z

landed in 5c6c029

PR-URL: #13755 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

This fixes an error that could occure by nesting async_hooks calls PR-URL: #14054 Ref: #13755 (comment) Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

PR-URL: #13755 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

This fixes an error that could occure by nesting async_hooks calls PR-URL: #14054 Ref: #13755 (comment) Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jun 18, 2017

mscdex reviewed Jun 18, 2017

View reviewed changes

lib/async_hooks.js Outdated

Copy link

Contributor

mscdex Jun 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this still called emitAfterN?

addaleax reacted with thumbs up emoji

cjihrig reviewed Jun 18, 2017

View reviewed changes

lib/async_hooks.js Outdated

Copy link

Contributor

cjihrig Jun 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is over 80 characters.

BridgeAR added 2 commits June 19, 2017 16:47

async_hooks: reduce duplication with factory

58f011f

async_hooks: add TODO entry

4c00eee

BridgeAR force-pushed the dry-code branch from 5d174d2 to 4c00eee Compare June 19, 2017 14:47

trevnorris suggested changes Jun 21, 2017

View reviewed changes

rename factory

8a8117e

refack approved these changes Jun 23, 2017

View reviewed changes

refack self-assigned this Jun 23, 2017

trevnorris reviewed Jun 29, 2017

View reviewed changes

address comment: use named functions

8918fc8

AndreasMadsen reviewed Jul 3, 2017

View reviewed changes

BridgeAR added 3 commits July 3, 2017 14:19

fixup func-style

e4cd110

fixup rename function

795358a

fixup add comment about naming fn

fc397f8

BridgeAR force-pushed the dry-code branch from 0cf8591 to fc397f8 Compare July 3, 2017 12:22

BridgeAR mentioned this pull request Jul 3, 2017

async_hooks: move statement #14054

Closed

4 tasks

AndreasMadsen approved these changes Jul 3, 2017

View reviewed changes

AndreasMadsen closed this Jul 5, 2017

addaleax pushed a commit that referenced this pull request Jul 11, 2017

async_hooks: reduce duplication with factory

3f3749b

PR-URL: #13755 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

addaleax mentioned this pull request Jul 11, 2017

v8.2.0 proposal #13744

Merged

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

addaleax pushed a commit that referenced this pull request Jul 18, 2017

async_hooks: reduce duplication with factory

2eabd92

PR-URL: #13755 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017

async_hooks: reduce duplication with factory

70dad23

PR-URL: #13755 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com>

BridgeAR added the backported-to-v8.x label Sep 14, 2017

refack removed their assignment Oct 20, 2018

BridgeAR deleted the dry-code branch April 1, 2019 23:36

Uh oh!

Conversation

BridgeAR commented Jun 18, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jun 23, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jun 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jul 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jul 3, 2017

Uh oh!

AndreasMadsen commented Jul 3, 2017

Uh oh!

AndreasMadsen commented Jul 3, 2017

Uh oh!

AndreasMadsen left a comment

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Jul 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AndreasMadsen commented Jul 3, 2017 •

edited

Loading