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

Refactor async_hooks initTriggerId #17273

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
8 participants
@AndreasMadsen
Copy link
Member

commented Nov 23, 2017

  • 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 #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

@AndreasMadsen

This comment has been minimized.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Nov 23, 2017

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

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2017

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

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

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

}


function setInitTriggerId(triggerAsyncId) {
function defaultTriggerAsyncIdScope(triggerAsyncId, block) {

This comment has been minimized.

Copy link
@mcollina

mcollina Dec 6, 2017

Member

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.

lib/net.js Outdated
err = self._handle.connect(req, address, port);
else
err = self._handle.connect6(req, address, port);
});

This comment has been minimized.

Copy link
@mcollina

mcollina Dec 6, 2017

Member

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);
}
@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

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

lib/net.js Outdated
req.oncomplete = afterShutdown;
req.handle = self._handle;
return self._handle.shutdown(req);
}

This comment has been minimized.

Copy link
@mcollina

mcollina Dec 7, 2017

Member

Can you move this to a top-level function?

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

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

@mcollina

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

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

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

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

@mcollina
Copy link
Member

left a comment

LGTM

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

@MylesBorins MylesBorins force-pushed the nodejs:master branch from b7405ab to 7f086dd Dec 8, 2017

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2017

@addaleax Could you take a look?

@AndreasMadsen AndreasMadsen requested a review from addaleax Dec 11, 2017

@AndreasMadsen

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2017

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

AndreasMadsen added some commits Nov 22, 2017

async_hooks: rename initTriggerId
rename initTriggerId to defaultTriggerAsyncId such it matches the rest
of our naming.
async_hooks: separate missing from default context
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.
async_hooks: use scope for defaultTriggerAsyncId
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.

evanlucas added a commit that referenced this pull request Jan 15, 2018

async_hooks: rename initTriggerId
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 added a commit that referenced this pull request Jan 15, 2018

async_hooks: separate missing from default context
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 added a commit that referenced this pull request Jan 15, 2018

async_hooks: use scope for defaultTriggerAsyncId
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>

gibfahn added a commit to gibfahn/node that referenced this pull request Jan 16, 2018

async_hooks: rename initTriggerId
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>

gibfahn added a commit to gibfahn/node that referenced this pull request Jan 16, 2018

async_hooks: separate missing from default context
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>

gibfahn added a commit to gibfahn/node that referenced this pull request Jan 16, 2018

async_hooks: use scope for defaultTriggerAsyncId
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>

gibfahn added a commit to gibfahn/node that referenced this pull request Jan 17, 2018

async_hooks: rename initTriggerId
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>

gibfahn added a commit to gibfahn/node that referenced this pull request Jan 17, 2018

async_hooks: separate missing from default context
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>

gibfahn added a commit to gibfahn/node that referenced this pull request Jan 17, 2018

async_hooks: use scope for defaultTriggerAsyncId
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>

MylesBorins added a commit that referenced this pull request Jan 19, 2018

async_hooks: rename initTriggerId
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>

MylesBorins added a commit that referenced this pull request Jan 19, 2018

async_hooks: separate missing from default context
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>

MylesBorins added a commit that referenced this pull request Jan 19, 2018

async_hooks: use scope for defaultTriggerAsyncId
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>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

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

PR-URL: nodejs#17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

async_hooks: separate missing from default context
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
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

async_hooks: use scope for defaultTriggerAsyncId
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
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

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

PR-URL: nodejs#17273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

async_hooks: separate missing from default context
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
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

async_hooks: use scope for defaultTriggerAsyncId
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
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn added a commit that referenced this pull request Mar 6, 2018

2018-03-06 Version 8.10.0 'Carbon' (LTS)
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

gibfahn added a commit that referenced this pull request Mar 7, 2018

2018-03-06 Version 8.10.0 'Carbon' (LTS)
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

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

2018-03-06 Version 8.10.0 'Carbon' (LTS)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.