Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: reduce code duplication by using a factory #13755

Closed
wants to merge 7 commits into from

Conversation

BridgeAR
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jun 18, 2017
function hookFactory(symbol) {
// Called from native. The asyncId stack handling is taken care of there
// before this is called.
return function emitAfterN(asyncId) {
Copy link
Contributor

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?

@@ -325,7 +328,7 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {
triggerAsyncId = initTriggerId();
}

// I'd prefer allowing these checks to not exist, or only throw in a debug
// TODO(trevnorris): I'd prefer allowing these checks to not exist, or only throw in a debug
Copy link
Contributor

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.

Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

Minor change request, but other than that LGTM

@@ -49,6 +49,9 @@ const init_symbol = Symbol('init');
const before_symbol = Symbol('before');
const after_symbol = Symbol('after');
const destroy_symbol = Symbol('destroy');
const emitBeforeN = hookFactory(before_symbol);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: change this to hookFactoryEmit or emitHookFactory.

Copy link
Contributor

Choose a reason for hiding this comment

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

@trevnorris PTAL...

@@ -325,8 +328,8 @@ function emitInitS(asyncId, type, triggerAsyncId, resource) {
triggerAsyncId = initTriggerId();
}

// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

good call changing this to a TODO.

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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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')

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

PTAL

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM
1 question

@@ -49,6 +49,9 @@ const init_symbol = Symbol('init');
const before_symbol = Symbol('before');
const after_symbol = Symbol('after');
const destroy_symbol = Symbol('destroy');
const emitBeforeN = emitHookFactory(before_symbol);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@refack refack self-assigned this Jun 23, 2017
}
} catch (e) {
fatalError(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx

@BridgeAR
Copy link
Member Author

Comment addressed

@@ -49,6 +49,9 @@ const init_symbol = Symbol('init');
const before_symbol = Symbol('before');
const after_symbol = Symbol('after');
const destroy_symbol = Symbol('destroy');
const emitBeforeN = emitHookFactory(before_symbol, 'emitBeforeN');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

restoreTmpHooks();
}
};
Object.defineProperty(fn, 'name', {
Copy link
Member

Choose a reason for hiding this comment

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

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

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 3, 2017

@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
Copy link
Member Author

BridgeAR commented Jul 3, 2017

@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
Copy link
Member

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
Copy link
Member

@BridgeAR BridgeAR mentioned this pull request Jul 3, 2017
4 tasks
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM

@AndreasMadsen
Copy link
Member

landed in 5c6c029

addaleax pushed a commit that referenced this pull request Jul 11, 2017
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 addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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 pushed a commit that referenced this pull request Jul 18, 2017
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>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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
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>
@refack refack removed their assignment Oct 20, 2018
@BridgeAR BridgeAR deleted the dry-code branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants