Async hooks rename by AndreasMadsen · Pull Request #14152 · nodejs/node

AndreasMadsen · 2017-07-10T12:38:49Z

Checklist

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

Affected core subsystem(s)

async_hooks

Rename init to emitInitNative and all emit*S to emit*Script. I know some are against these purely stylish changes so I included a small consistency fix to AsyncResource and emitInit that is much more obvious with the renames.

AndreasMadsen · 2017-07-10T12:40:16Z

test/async-hooks/test-embedder.api.async-resource.js

This change wasn't needed. But when I looked for the AsyncResource('default_trigger_id') test, I found that it could be made more strict.

refack

nits

refack · 2017-07-10T14:04:51Z

lib/async_hooks.js

So just an FYI since this effectively changes the signature of the ctor (now AsyncResource.length == 1 ~~it would have been~~ some do consider it semver-major, but since this congruent with the docs, I'd call this a bug fix.

Also this is still officially experimental, which should give us more flexibility.

refack · 2017-07-10T14:07:07Z

test/async-hooks/test-embedder.api.async-resource.js

~~Could you add assert.strinctEqual(AsyncResource.length, 1);~~
Retracted

addaleax · 2017-07-10T14:10:15Z

I’m strongly against considering changes semver-major because they modify the value of fn.length, experimental API or not, unless we have an actual reason to expect breakage.

(edit: and, by extension, we should not need to test the value of fn.length)

refack · 2017-07-10T14:12:52Z

I’m strongly against considering changes semver-major because they modify the value of fn.length, experimental API or not, unless we have an actual reason to expect breakage.

I agree, but some do consider it as such 🤷‍♂️ Should bring it up with the release team or the CTC.

AndreasMadsen · 2017-07-12T05:54:12Z

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

AndreasMadsen · 2017-07-12T12:07:47Z

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

AndreasMadsen · 2017-07-12T16:22:59Z

There were a huge amount of unrelated CI failures. It looks like most of it have been fixed in master now. I have rebased on master, hopefully that will fix it.

~~CI: https://ci.nodejs.org/job/node-test-pull-request/9091/~~ (jenkins MacOS failure)
CI: https://ci.nodejs.org/job/node-test-pull-request/9099/

trevnorris · 2017-07-12T20:22:40Z

test/parallel/test-async-hooks-asyncresource-constructor.js

nit: this looks a little odd syntactically. maybe something like:

const async_hooks = require('async_hooks'); const { AsyncResource } = async_hooks;

trevnorris

Have a non-blocking nit. Please don't forget to squash the two commits, and maybe add a message body with more explanation.

AndreasMadsen · 2017-07-12T21:28:54Z

Please don't forget to squash the two commits, and maybe add a message body with more explanation.

Really, you want the two commits squashed? Logically they are very completely separate.

trevnorris · 2017-07-12T22:32:52Z

@AndreasMadsen No. It's a friendly reminder to make sure the commit

[squash] enable hooks when testing asserts

is squashed into whatever commit it belongs. It's also not necessarily a reminder for you, but whomever it is that lands the PR.

AndreasMadsen · 2017-07-13T07:47:43Z

I fixed the nit and added some more text in the commit message. If the CI is green I will just go ahead and merge.

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

There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed.

AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead.

AndreasMadsen · 2017-07-13T10:06:19Z

Fixed a small merge conflict.

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

AndreasMadsen · 2017-07-13T12:17:17Z

landed in 628485e and 31417b6

trevnorris · 2017-07-13T21:54:56Z

@AndreasMadsen Thanks for the two thorough git commit messages.

There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed. PR-URL: #14152 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: Anna Henningsen <anna@addaleax.net>

AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead. PR-URL: #14152 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: Anna Henningsen <anna@addaleax.net>

There are two categories of emit functions in async_hooks, those used by c++ (native) and those used by JavaScript (script). Previously these were named N for native and S for script. Finally, there was an odd case where emitInitN was called just init. This makes it more explicit by using the names emitInitNative and emitInitScript. The other emit functions are also renamed. PR-URL: #14152 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: Anna Henningsen <anna@addaleax.net>

AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead. PR-URL: #14152 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: Anna Henningsen <anna@addaleax.net>

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

AndreasMadsen commented Jul 10, 2017

View reviewed changes

addaleax approved these changes Jul 10, 2017

View reviewed changes

refack approved these changes Jul 10, 2017

View reviewed changes

jasnell approved these changes Jul 10, 2017

View reviewed changes

refack approved these changes Jul 12, 2017

View reviewed changes

trevnorris reviewed Jul 12, 2017

View reviewed changes

trevnorris approved these changes Jul 12, 2017

View reviewed changes

AndreasMadsen added 4 commits July 13, 2017 12:05

async_hooks: make AsyncResource match emitInit

e50fa6b

AsyncResource previously called emitInitNative. Since AsyncResource is just an abstraction on top of the emitEventScript functions, it should call emitInitScript instead.

[squash] enable hooks when testing asserts

be5dbc5

[squash] fix double require

1125791

AndreasMadsen closed this Jul 13, 2017

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

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

Uh oh!

Comments

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

AndreasMadsen Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

refack commented Jul 10, 2017

Uh oh!

AndreasMadsen commented Jul 12, 2017

Uh oh!

AndreasMadsen commented Jul 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

trevnorris Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AndreasMadsen commented Jul 13, 2017

Uh oh!

AndreasMadsen commented Jul 13, 2017

Uh oh!

AndreasMadsen commented Jul 13, 2017

Uh oh!

trevnorris commented Jul 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AndreasMadsen commented Jul 10, 2017 •

edited

Loading

refack Jul 10, 2017 •

edited

Loading

refack Jul 10, 2017 •

edited

Loading

addaleax commented Jul 10, 2017 •

edited

Loading

AndreasMadsen commented Jul 12, 2017 •

edited

Loading

AndreasMadsen commented Jul 12, 2017 •

edited

Loading

trevnorris commented Jul 12, 2017 •

edited

Loading