-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
async_hooks,process: remove internalNextTick #19147
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,process: remove internalNextTick #19147
Conversation
lib/_http_agent.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After checking all the call sites, it's not possible to receive a socket here if err is truthy. (I've also double-checked this by adding an assert during debugging.)
lib/_http_outgoing.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is conn[async_id_symbol] === this.socket[async_id_symbol]? Also, this changes the socketAsyncId so if this is correct the comment should be updated too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conn === this.socket so it's equivalent. Not quite clear on the second part of the feedback — the comment above it still applies.
|
I think we should benchmark the http throughput before landing this. edit: also, it is kinda an implementation detail that |
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/129/ (only |
I've adjusted the function to special handle |
|
Nothing meaningful from the benchmark. I think these changes are very unlikely to make a difference. |
AndreasMadsen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Instead of having mostly duplicate code in form of internalNextTick, instead use the existing defaultAsyncTriggerIdScope with a slight modification which allows undefined triggerAsyncId to be passed in, which then just triggers the callback with the provided arguments.
7958cea to
37f6ef0
Compare
|
Lite CI (after rebase): https://ci.nodejs.org/job/node-test-pull-request-lite/237/ |
|
Landed in 4d07434 |
Instead of having mostly duplicate code in form of internalNextTick, instead use the existing defaultAsyncTriggerIdScope with a slight modification which allows undefined triggerAsyncId to be passed in, which then just triggers the callback with the provided arguments. PR-URL: #19147 Refs: #19104 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
Should this be backported to |
|
Yes, APM providers will appreciate if this were backported. |
Instead of having mostly duplicate code in form of internalNextTick, instead use the existing defaultAsyncTriggerIdScope with a slight modification which allows undefined triggerAsyncId to be passed in, which then just triggers the callback with the provided arguments. PR-URL: nodejs#19147 Refs: nodejs#19104 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
Should this be backported to 8.x? If so, we need a separate backport PR. |
Instead of having mostly duplicate code in form of
internalNextTick, instead use the existingdefaultAsyncTriggerIdScopewith a slight modification which allows undefinedtriggerAsyncIdto be passed in, which then just triggers the callback with the provided arguments.(The ability to pass in
undefinedremoves the need for a bunch of conditional checks for whether the asyncId is set on a given resource.)Refs: #19104
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks, http, dgram, net, process, test