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

[v9.x backport] Refactor async_hooks initTriggerId #18079

Closed
wants to merge 3 commits into from
Closed

[v9.x backport] Refactor async_hooks initTriggerId #18079

wants to merge 3 commits into from

Conversation

AndreasMadsen
Copy link
Member

@AndreasMadsen AndreasMadsen commented Jan 10, 2018

Backport of #17273

For performance reasons #18004 should land after.

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 initTriggerId to defaultTriggerAsyncId such it matches the rest
of our naming.

PR-URL: #17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
When context is missing the executionAsyncId will be zero. For the
default triggerAsyncId the zero value was used to default to the
executionAsyncId. While this was not technically wrong because the
functions are different themself, it poorly separated the two concepts.

PR-URL: #17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Jan 10, 2018
@AndreasMadsen AndreasMadsen added the async_hooks Issues and PRs related to the async hooks subsystem. label Jan 10, 2018
@AndreasMadsen
Copy link
Member Author

@AndreasMadsen AndreasMadsen changed the title Backport 17273 to v9.x [v9.x backport] Refactor async_hooks initTriggerId Jan 10, 2018
Previously the getter would mutate the kDefaultTriggerAsncId value. This
refactor changes the setter to bind the current kDefaultTriggerAsncId to
a scope, such that the getter doesn't have to mutate its own value.

PR-URL: #17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@AndreasMadsen
Copy link
Member Author

Fixed lint issue.

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

evanlucas pushed a commit that referenced this pull request Jan 15, 2018
rename initTriggerId to defaultTriggerAsyncId such it matches the rest
of our naming.

Backport-PR-URL: #18079
PR-URL: #17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 15, 2018
When context is missing the executionAsyncId will be zero. For the
default triggerAsyncId the zero value was used to default to the
executionAsyncId. While this was not technically wrong because the
functions are different themself, it poorly separated the two concepts.

Backport-PR-URL: #18079
PR-URL: #17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 15, 2018
Previously the getter would mutate the kDefaultTriggerAsncId value. This
refactor changes the setter to bind the current kDefaultTriggerAsncId to
a scope, such that the getter doesn't have to mutate its own value.

Backport-PR-URL: #18079
PR-URL: #17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas
Copy link
Contributor

Landed in 506d85b...a880e27. Thanks!

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. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants