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 rename #14152

Closed
wants to merge 4 commits into from
Closed

Async hooks rename #14152

wants to merge 4 commits into from

Conversation

AndreasMadsen
Copy link
Member

@AndreasMadsen AndreasMadsen commented Jul 10, 2017

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.

@nodejs-github-bot nodejs-github-bot added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 10, 2017
@@ -18,8 +18,8 @@ assert.throws(() => new AsyncResource('invalid_trigger_id', null),
/^RangeError: triggerAsyncId must be an unsigned integer$/);

assert.strictEqual(
typeof new AsyncResource('default_trigger_id').triggerAsyncId(),
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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.

nits


if (typeof type !== 'string' || type.length <= 0)
throw new TypeError('type must be a string with length > 0');
constructor(type, triggerAsyncId = initTriggerId()) {
Copy link
Contributor

@refack refack Jul 10, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

typeof new AsyncResource('default_trigger_id').triggerAsyncId(),
'number'
new AsyncResource('default_trigger_id').triggerAsyncId(),
async_hooks.executionAsyncId()
);

Copy link
Contributor

@refack refack Jul 10, 2017

Choose a reason for hiding this comment

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

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

@addaleax
Copy link
Member

addaleax commented Jul 10, 2017

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

refack commented Jul 10, 2017

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

@AndreasMadsen
Copy link
Member Author

@AndreasMadsen
Copy link
Member Author

AndreasMadsen commented Jul 12, 2017

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/

@@ -4,8 +4,14 @@ require('../common');
// This tests that AsyncResource throws an error if bad parameters are passed

const assert = require('assert');
const async_hooks = require('async_hooks');
const AsyncResource = require('async_hooks').AsyncResource;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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.

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

@AndreasMadsen
Copy link
Member Author

AndreasMadsen commented Jul 12, 2017

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

trevnorris commented Jul 12, 2017

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

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

Fixed a small merge conflict.

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

@AndreasMadsen
Copy link
Member Author

landed in 628485e and 31417b6

@trevnorris
Copy link
Contributor

@AndreasMadsen Thanks for the two thorough git commit messages.

@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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>
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.

6 participants