async_hooks: rename currentId to currentAsyncId #13490

AndreasMadsen · 2017-06-06T08:09:18Z

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

Rename rename async_hooks.currentId() to async_hooks.currentAsyncId() as requested by @trevnorris. I just did a search and replace, it appears to work.

Since async_hooks is an experimental API and we actually haven't published the docs, then I think this is a semver-minor.

To be completely fair to the "make -j4 test passes" I actually couldn't run it all, I'm getting this error:

Building addon /Users/Andreas/Sites/node/node-review/test/addons-napi/test_globals/
gyp: binding.gyp not found (cwd: /Users/Andreas/Sites/node/node-review/test/addons-napi/test_globals) while trying to load binding.gyp
make[1]: *** [test/addons-napi/.buildstamp] Error 1
make: *** [test] Error 2

I don't think it is related. I'm going to see what the CI says.

AndreasMadsen · 2017-06-06T08:10:37Z

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

/cc @nodejs/async_hooks

vsemozhetbyt · 2017-06-06T08:20:53Z

doc/api/async_hooks.md

FWIW, the line length is 82 now (may be relevant if we have md linting soon).

vsemozhetbyt · 2017-06-06T08:21:55Z

doc/api/async_hooks.md

Line length is 82.

vsemozhetbyt · 2017-06-06T08:22:21Z

doc/api/async_hooks.md

line length is 81.

addaleax · 2017-06-06T08:34:33Z

lib/async_hooks.js

I know we’re okay with some breakage as long as this is fresh & experimental, but maybe
Object.defineProperty(module.exports, 'currentId', { value: currentAsyncId });?

why defineProperty?

Because that defaults to non-enumerable, just to avoid a bit of confusion for people inspecting the require('async_hooks') object :)

I've created a internalUtil.deprecate alias.

refack · 2017-06-06T11:59:54Z

doc/api/deprecations.md

[question] do we deprecate > Stability: 1 - Experimental API's?
I'm not convinced we need to. Not sure if it's better for users or just noise 🤔

It is only noise if they try to use async_hooks.currentId.

I was thinking about documentation noise, but I yield to @addaleax's opinion that doing it "the usual way" makes sense.

refack · 2017-06-06T12:01:22Z

doc/api/deprecations.md

If we decide to keep this I suggest either /s/was/is/ or *Note*: change was made while `async_hooks` was an experimental API.

refack · 2017-06-06T12:03:57Z

lib/async_hooks.js

I think we should hide it, i.e. { configurable: false, enumerable: false } so it will work but won't be discoverable by inspection.

refack · 2017-06-06T12:08:49Z

I'm ±0 on need to deprecate. Not sure if it's better for users or just noise 🤔.
A well worded line in the changelog might be just as good (and doing SEO for this page, like putting renaming async_hooks.currentId to async_hooks.currentAsyncId as the title so googling async_hooks.currentId will hit this page)

AndreasMadsen · 2017-06-06T12:32:17Z

I'm very much against documenting an API that was never documented because we changed the API. If we are going to alias the method we should print a warning and suggest the better alternative. I don't care if we call it a deprecation or something else.

refack · 2017-06-06T12:44:16Z

FYI: #13483

So I suggest remove the deprecation code, leave just the "niceness" property, and rename this PR (for SEO)

addaleax · 2017-06-06T12:47:41Z

So I suggest remove the deprecation code

Yeah no, I’m -1 on that, it makes sense to deprecate things that will be going away at some point.

Also, I really don’t know what you mean by “rename this PR” … the current title seems to be accurate?

refack · 2017-06-06T12:49:42Z

Yeah no, I’m -1 on that, it makes sense to deprecate things that will be going away at some point.

Also, I really don’t know what you mean by “rename this PR” … the current title seems to be accurate?

Ack.
As for title I'm only assuming if we rename to renaming async_hooks.currentId to async_hooks.currentAsyncId it will help with SEO.

trevnorris

Thanks much for doing this. Would like to see one more API change along with this.

trevnorris · 2017-06-06T16:34:40Z

doc/api/async_hooks.md

To keep the API uniform I'd like to change triggerId() to currentTriggerId(). If that can't happen then I'd say we drop this.

Something that confuses me about the currentId renaming is that triggerId also refers to an asyncId. So in my mind currentTriggerAsyncId() is what makes sense. Of cause that is a horribly long name and it doesn't change that currentAsyncId() isn't really specific. It just says it is an asyncId that is current, but the same could be said about triggerId.

My previous understanding was that current had some special meaning, and a logical renaming of triggerId would then be triggerAsyncId(). But your comment invalidates that understanding.

@AndreasMadsen , maybe async_hooks.currentContext().asyncId and async_hooks.currentContext().triggerId more make sense? But it will add a new object..

The most technically correct name for currentId() would be asyncIdOfCurrentExecutionScope().

you're right in that triggerId is an asyncId. it's the asyncId of the resource that is responsible for the callback being called. e.g.

const async_hooks = require('async_hooks'); const net = require('net'); const print = process._rawDebug; const server = net.createServer(connection => { print(connection._handle.getAsyncId() === async_hooks.triggerId()); print(server._handle.getAsyncId() === async_hooks.currentId()); server.close(); }).listen(8080, () => net.connect(8080).end());

(unfortunately there's a bug right now which doesn't properly set the triggerId for the 'connection' callback)

The currentId is the server's asyncId because the server is responsible for the current execution context. The triggerId is set to the connection's asyncId because the 'connection' callback would never been called if the new connection hadn't been made.

Is currentExecutionAsyncId() and currentTriggerAsyncId() then the names? If so, may I suggest we remove the current part, I think it is implied from a global getter:

executionAsyncId()

triggerAsyncId()

maybe async_hooks.currentContext().asyncId and async_hooks.currentContext().triggerId more make sense? But it will add a new object..

I don't think that solves anything, it just prefixes the names a bit more.

@AndreasMadsen I'm not picky. If others think the names are more descriptive then I'm fine with them.

/cc @addaleax @refack

I’m not picky either, but I can say that I’ve never regretted using a long identifier if it is just a tiny bit more expressive or easier to understand; muscle memory and/or autocomplete take care of that once you’re used to typing them.

executionAsyncId() and triggerAsyncId() sound fine to me, with or without the current prefix and a slight preference for having the Async middle bit.

Great. @AndreasMadsen seems like executionAsyncId() and triggerAsyncId() are the names to use. Thanks for taking care of this.

trevnorris · 2017-06-07T20:27:55Z

@addaleax

Yeah no, I’m -1 on that, it makes sense to deprecate things that will be going away at some point.

How long would you recommend we keep the deprecated API around? I don't think it would be necessary to keep it around once v8.x goes LTS.

addaleax · 2017-06-07T20:36:28Z

How long would you recommend we keep the deprecated API around? I don't think it would be necessary to keep it around once v8.x goes LTS.

@trevnorris We could probably remove it before that, yes … but why? We can drop it from master right after this PR if we want, so the code we’ll be working with is clean, and we won’t really care if there’s some extra code in the release lines unless it gives merge conflicts (which this one won’t, or at most in a trivially fixed way), right?

trevnorris · 2017-06-07T20:47:25Z

@addaleax

We could probably remove it before that, yes … but why? We can drop it from master right after this PR if we want

not sure where my mind was. nm me.

AndreasMadsen · 2017-06-09T08:51:32Z

@addaleax @refack @trevnorris

I renamed currentId to executionAsyncId and triggerId to triggerAsyncId. Be aware this also affects:

JS AsyncResources.triggerId
C++ AsyncHooksGetCurrentId
C++ AsyncHooksGetTriggerId

Note: Unless it is something very minor I can't spend the time on it, I just don't have the time. Feel free to push to my branch or create a new PR.

AndreasMadsen · 2017-06-09T09:45:25Z

src/node.h

Not related to this PR, but shouldn't this be AsyncHooksGetExecutionAsyncId?

I did it this way to match the if (triggerId === undefined) { triggerId = initTriggerId(); … bit in the JS AsyncResource class. AsyncHooksGetTriggerAsyncId() uses env->get_init_trigger_id() which defaults to env->current_async_id(), and that also matches what initTriggerId() in JS does, so if anything needs to be changed here it should also be changed in JS.

This does look correct to me, though… @trevnorris ?

Oh, right. It is the AsyncHooksGetTriggerAsyncId implementation that is wrong then, it should return async_uid_fields[kCurrentTriggerId] not async_uid_fields[kInitTriggerId].

The actual behavior in AsyncResource is fine.

AndreasMadsen · 2017-06-09T13:07:58Z

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

refack · 2017-06-09T18:07:58Z

One of the windows runs went pathological, so I killed it:
https://ci.nodejs.org/job/node-test-binary-windows/9117/RUN_SUBSET=0,VS_VERSION=vs2015,label=win2012r2/
But console look interesting, it has been spewing this for the last 4 hours (log is 3GB long)

==== C stack trace ===============================

	lh_node_usage_stats_bio [0x00007FF721840991+200801]
	lh_node_usage_stats_bio [0x00007FF72183A11D+174061]
	lh_node_usage_stats_bio [0x00007FF721839EFE+173518]
	lh_node_usage_stats_bio [0x00007FF72183A0F3+174019]
	lh_node_usage_stats_bio [0x00007FF721839FF6+173766]
	RtlProcessFlsData [0x00007FFE06F5D9F7+295]
	LdrShutdownThread [0x00007FFE06F5BA49+73]
	RtlExitUserThread [0x00007FFE06F585DE+62]
	FreeLibraryAndExitThread [0x00007FFE041E2C7C+76]
	FreeLibraryAndExitThread [0x00007FFE05079CDA+10]
	lh_node_usage_stats_bio [0x00007FF72182F39D+129645]
	lh_node_usage_stats_bio [0x00007FF72182F4E5+129973]
	lh_node_usage_stats_bio [0x00007FF72182F320+129520]
	BaseThreadInitThunk [0x00007FFE050713D2+34]
	RtlUserThreadStart [0x00007FFE06F554E4+52]

/cc @nodejs/build (maybe it needs cleanup)

AndreasMadsen · 2017-06-10T18:01:23Z

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

AndreasMadsen · 2017-06-11T07:50:27Z

A new test landed in master, the CI auto rebased and that caused some issues.

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

AndreasMadsen · 2017-06-11T11:44:10Z

One test failure on arm-ubuntu:

not ok 1521 parallel/test-child-process-stdio-big-write-end
  ---
  duration_ms: 120.221
  severity: fail
  stack: |-
    timeout

I don't think it is related.

AndreasMadsen · 2017-06-11T11:45:13Z

@refack @addaleax @trevnorris please review again. I keep having to fix rebase conflicts, so I would like to land this.

refack

LGTM
(rubber stamp completeness)

addaleax · 2017-06-11T11:53:15Z

Since async_hooks is an experimental API and we actually haven't published the docs, then I think this is a semver-minor.

Just so you know, we have published the docs: https://nodejs.org/api/async_hooks.html

AndreasMadsen · 2017-06-11T11:54:52Z

Just so you know, we have published the docs: https://nodejs.org/api/async_hooks.html

It was true at the time :). With the deprecated aliases, I don't think it is an issue.

AndreasMadsen · 2017-06-13T12:32:04Z

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

AndreasMadsen · 2017-06-13T14:19:29Z

All these test failures make me nervous. I think they are all unrelated, but could one of you give me a second opinion.

AndreasMadsen · 2017-06-14T08:07:48Z

Looks like ARM is online again.

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

AndreasMadsen · 2017-06-14T10:38:54Z

only one failure (#13577), and it is not related. I will merge this.

AndreasMadsen · 2017-06-14T10:42:30Z

landed in de762b7

currentId is renamed to executionAsyncId triggerId is renamed to triggerAsyncId AsyncResource.triggerId is renamed to AsyncResource.triggerAsyncId AsyncHooksGetCurrentId is renamed to AsyncHooksGetExecutionAsyncId AsyncHooksGetTriggerId is renamed to AsyncHooksGetTriggerAsyncId PR-URL: #13490 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

- currentId() => executionAsyncId() - triggerId() => triggerAsyncId() see nodejs/node#13490

…telyTyped#18530) - currentId() => executionAsyncId() - triggerId() => triggerAsyncId() see nodejs/node#13490

- currentId() => executionAsyncId() - triggerId() => triggerAsyncId() see nodejs/node#13490

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

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

vsemozhetbyt reviewed Jun 6, 2017

View reviewed changes

doc/api/async_hooks.md Outdated

Copy link

Contributor

vsemozhetbyt Jun 6, 2017

Choose a reason for hiding this comment

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

FWIW, the line length is 82 now (may be relevant if we have md linting soon).

vsemozhetbyt reviewed Jun 6, 2017

View reviewed changes

doc/api/async_hooks.md Outdated

Copy link

Contributor

vsemozhetbyt Jun 6, 2017

Choose a reason for hiding this comment

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

Line length is 82.

vsemozhetbyt reviewed Jun 6, 2017

View reviewed changes

doc/api/async_hooks.md Outdated

Copy link

Contributor

vsemozhetbyt Jun 6, 2017

Choose a reason for hiding this comment

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

line length is 81.

addaleax approved these changes Jun 6, 2017

View reviewed changes

refack approved these changes Jun 6, 2017

View reviewed changes

trevnorris suggested changes Jun 6, 2017

View reviewed changes

AndreasMadsen commented Jun 9, 2017

View reviewed changes

addaleax approved these changes Jun 11, 2017

View reviewed changes

refack approved these changes Jun 11, 2017

View reviewed changes

AndreasMadsen added 6 commits June 13, 2017 14:30

[squash] lint

038b070

[squash] rebase fixes

767008f

[squash] alias C++ renames

96818e4

[squash] ABI compatiable, use c++14 deprecation syntax

248bc1d

[squash] cleanup v8 namespace

1566af0

[squash] no C++14 synatex

86bff35

AndreasMadsen closed this Jun 14, 2017

AndreasMadsen deleted the currenct-id-rename branch June 14, 2017 10:42

refack mentioned this pull request Jun 16, 2017

Now: Jenkins is in a weird mood nodejs/build#761

Closed

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

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

Flarna mentioned this pull request Jul 31, 2017

[node] Update async_hooks according to renaming done in 8.2.0 DefinitelyTyped/DefinitelyTyped#18530

Merged

8 tasks

sheetalkamat pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Aug 1, 2017

[node] Update async_hooks according to renaming done in 8.2.0 (#18530)

34d4afe

- currentId() => executionAsyncId() - triggerId() => triggerAsyncId() see nodejs/node#13490

refack mentioned this pull request Aug 9, 2017

async_hooks experimental status #14717

Closed

KSXGitHub pushed a commit to KSXGitHub/DefinitelyTyped.node that referenced this pull request May 12, 2018

[node] Update async_hooks according to renaming done in 8.2.0 (#18530)

2e4f511

- currentId() => executionAsyncId() - triggerId() => triggerAsyncId() see nodejs/node#13490

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Uh oh!

async_hooks: rename currentId to currentAsyncId #13490

async_hooks: rename currentId to currentAsyncId #13490

Uh oh!

Conversation

AndreasMadsen commented Jun 6, 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 commented Jun 6, 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!

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!

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!

refack commented Jun 6, 2017

Uh oh!

AndreasMadsen commented Jun 6, 2017

Uh oh!

refack commented Jun 6, 2017

Uh oh!

addaleax commented Jun 6, 2017

Uh oh!

refack commented Jun 6, 2017

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen Jun 6, 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!

JiaLiPassion Jun 6, 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!

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!

trevnorris commented Jun 7, 2017

Uh oh!

addaleax commented Jun 7, 2017

Uh oh!

trevnorris commented Jun 7, 2017

Uh oh!

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

Uh oh!

Uh oh!

AndreasMadsen commented Jun 6, 2017 •

edited

Loading

AndreasMadsen Jun 6, 2017 •

edited

Loading

JiaLiPassion Jun 6, 2017 •

edited

Loading

AndreasMadsen commented Jun 9, 2017 •

edited

Loading