Refactor async_hooks initTriggerId by AndreasMadsen · Pull Request #17273 · nodejs/node · GitHub

The first commit just renames initTriggerId to defaultTriggerAsyncId. I think default is much more descriptive and we try to never use just id in async_hooks but instead asyncId.
The second commit changes the magic value from 0 to -1. This is because 0 in the executionAsyncId context refers to a missing context, while 0 in defaultTriggerAsyncId previously meant default context. This allows the defaultTriggerAsyncId to mean missing context too.
The third commit is the main change. It changes defaultTriggerAsyncId from being set by the setter and reset by the getter, to instead have defaultTriggerAsyncId be bound to a scope. The getter then doesn't change the value. This is much more intuitive than getters mutating its own value. This is actually related to the work done by @refack in async-hooks: internalize setInitTriggerId #14302 but this is a much more complete version of that.

Note: All this is internal logic. However, in node v8.x and v9.x the initTriggerId is unfortunately exported (although deprecated), so properly we need to do a manual backport for those versions.

edit: Oh, the entire motivation behind this is that I want to improve some of our set triggerAsyncIds. However, in those cases, it is not transparent if a new handle is always created thus the kDefaultTriggerAsyncId value might leak. Binding kDefaultTriggerAsyncId to a scope prevents a leak.

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

/cc @nodejs/async_hooks

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

Seems reasonable in concept but I don't understand the fine details enough to give a review.

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

/cc @addaleax @jasnell could you review this?

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

/cc @addaleax – please view :)
/cc @nodejs/async_hooks – in general.

I would add an opaque parameter here that we pass back to block(opaque), so that we can avoid to allocate closures where it was not needed before.

I would rewrite this as:

err = defaultTriggerAsyncIdScope(self[async_id_symbol], { self, addressType, req, address, port }, connectByType)

where connectByType is defined in the top level as:

function connectByType({ self, addressType, req, address, port }) {
  if (addressType === 4)
    return self._handle.connect(req, address, port);
  else
    return self._handle.connect6(req, address, port);
}

@mcollina Thanks. Ended up doing something slightly different to prevent the extra closures.

Can you move this to a top-level function?

@mcollina Done :) Are there any benefits of using destructuring assignments versus good old .apply(null, args)?

I don't think you can use .apply here, but you could use .bind(). By passing an opaque object/array, we avoid bind() altogether, and we can leverage the escape analysis in V8 that we are going to get in V8 6.3. Destructuring is just syntactic sugar, you can remove it if you want.
I have no data to say which of the approaches is better in respect to performance: my intent is to remove the closure allocations.

cc @bmeurer

Removed the dgram closure. This also makes the triggerAsyncId more consistent, as it doesn't make sense to have triggerAsyncId set to self[async_id_symbol] in only some error conditions :D

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/11943/
Benchmark CI (dgram): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/72/ – Just because I'm curious.

@addaleax Could you take a look?

@refack Could you take a look. Forgot that you opened the original PR #14302

rename initTriggerId to defaultTriggerAsyncId such it matches the rest of our naming.

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.

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.

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>

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>

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>

rename initTriggerId to defaultTriggerAsyncId such it matches the rest of our naming. PR-URL: nodejs#17273 Backport-PR-URL: nodejs#18179 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: nodejs#17273 Backport-PR-URL: nodejs#18179 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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: nodejs#17273 Backport-PR-URL: nodejs#18179 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

rename initTriggerId to defaultTriggerAsyncId such it matches the rest of our naming. Backport-PR-URL: #18179 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. Backport-PR-URL: #18179 PR-URL: #17273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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: #18179 PR-URL: #17273 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

AndreasMadsen added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap labels Nov 23, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 23, 2017

mcollina reviewed Dec 6, 2017

View reviewed changes

mcollina reviewed Dec 7, 2017

View reviewed changes

mcollina approved these changes Dec 7, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

AndreasMadsen requested a review from addaleax December 11, 2017 23:15

AndreasMadsen added lts-watch-v8.x author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed backport-requested-v8.x labels Dec 12, 2017

AndreasMadsen added 4 commits December 19, 2017 10:37

async_hooks: rename initTriggerId

d4ae984

rename initTriggerId to defaultTriggerAsyncId such it matches the rest of our naming.

async_hooks: use scope for defaultTriggerAsyncId

8a50186

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.

[squash] support opaque parameter

2b3db6e

This was referenced Jan 10, 2018

[async_hooks] stable API - tracking issue nodejs/diagnostics#124

Closed

Remove async_hooks setTrigger API #14238

Closed

evanlucas added backported-to-v9.x and removed backport-requested-v9.x labels Jan 15, 2018

This was referenced Jan 16, 2018

Semver Minor Tracking nodejs/Release#298

Closed

Backport async hooks to v8.x #18179

Closed

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added the backported-to-v8.x label Jul 31, 2018

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Refactor async_hooks initTriggerId by AndreasMadsen · Pull Request #17273 · nodejs/node · GitHub

Uh oh!

Uh oh!

Refactor async_hooks initTriggerId - #17273

Refactor async_hooks initTriggerId#17273AndreasMadsen wants to merge 7 commits intonodejs:masternodejs/node:masterfrom AndreasMadsen:async-hooks-trigger-id-scopeCopy head branch name to clipboard

Conversation

AndreasMadsen commented Nov 23, 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 Nov 23, 2017

Uh oh!

Fishrock123 commented Nov 23, 2017

Uh oh!

AndreasMadsen commented Nov 28, 2017

Uh oh!

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

Uh oh!

Uh oh!

mcollina Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Dec 7, 2017

Uh oh!

mcollina Dec 7, 2017

Choose a reason for hiding this comment

Uh oh!

AndreasMadsen commented Dec 7, 2017

Uh oh!

mcollina commented Dec 7, 2017

Uh oh!

AndreasMadsen commented Dec 7, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

AndreasMadsen commented Dec 11, 2017

Uh oh!

AndreasMadsen commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Refactor async_hooks initTriggerId#17273
AndreasMadsen wants to merge 7 commits into
nodejs:masterfrom
AndreasMadsen:async-hooks-trigger-id-scope

AndreasMadsen commented Nov 23, 2017 •
edited
Loading

AndreasMadsen commented Dec 5, 2017 •
edited
Loading

AndreasMadsen commented Dec 7, 2017 •
edited
Loading