-
Notifications
You must be signed in to change notification settings - Fork 30k
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,http: set HTTPParser trigger to socket #18003
async_hooks,http: set HTTPParser trigger to socket #18003
Conversation
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 👍 Would like to see my first comment addressed before this lands though as it'll have a performance impact (and would also reduce the number of changes required).
lib/_http_server.js
Outdated
@@ -273,7 +277,9 @@ function Server(requestListener) { | |||
// http://wiki.squid-cache.org/SquidFaq/InnerWorkings#What_is_a_half-closed_filedescriptor.3F | |||
this.httpAllowHalfOpen = false; | |||
|
|||
this.on('connection', connectionListener); | |||
this.on('connection', (socket) => defaultTriggerAsyncIdScope( |
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.
Since this is hot path listener, this should be refactored to not have a closure. The fact that an Array is now created each time is also problematic.
Could we perhaps instead keep all the connectionListener
references as is, then rename connectionListener
to something like internalConnectionListener
and have the connectionListener
invoke the defaultTriggerAsyncIdScope
with internalConnectionListener
? That way there's no need to make changes in https
or http2
.
(Also, I'll open a PR shortly to update defaultTriggerAsyncIdScope
for performance.)
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.
This is what I had in mind: a530e8f — you should be able to apply it as a patch if you want.
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.
Hehe, sorry. Already done it locally, running tests ...
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.
No worries. Just put it together in case I wasn't being clear and/or you didn't have time to work on it :)
lib/internal/async_hooks.js
Outdated
@@ -248,6 +248,12 @@ function newUid() { | |||
return ++async_id_fields[kAsyncIdCounter]; | |||
} | |||
|
|||
function getOrSetAsyncId(object) { | |||
if (object.hasOwnProperty(async_id_symbol)) return object[async_id_symbol]; |
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.
Super minor nit: can we add a line break before return
? I find these same line if-return statements really hard to read.
test/async-hooks/test-graph.http.js
Outdated
})); | ||
server.listen(0, common.mustCall()); | ||
|
||
http.get(`http://127.0.0.1:${server.address().port}`, common.mustCall()); |
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.
should this be moved inside the listening event handler? Wonder if this could possibly end up being flaky
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 with Evan's nit addressed.
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 as long as benchmarks look ok. It looks like the last run errored out and needs to be rerun.
@maclover7 Yes, we didn't have Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/88/ |
@AndreasMadsen Fyi, that benchmark CI has been running for nearly 40 hours now… do you think it would be okay to cancel it? The HTTP benchmarks take a lot longer than the other ones for some reason… :/ |
@addaleax I've stopped it, not sure why it takes so long. I set it to only test |
The expected wall time is thus:
As such there are two solutions to this.
I looked at the 95% confidence interval to see if fewer iterations will do (it did 54 before I stopped it): benchmark results
If a confidence interval between 2% and 4% is acceptable, then 50 iterations are enough. This will result in an expected run time of 40 hours :/ In terms of parameters, it looks like
|
Just an FYI, #18004 landed so this needs to be slightly updated to account for the new function signature. |
This allows more easy tracking of where HTTP requests come from. Before this change the HTTPParser would have the HTTPServer as the triggerAsyncId. The HTTPParser will still have the executionAsyncId set to the HTTP Server so that information is still directly available. Indirectly, the TCP socket itself also has its triggerAsyncId set to the TCP Server.
Rebased with the performance improvements in #18004, so I will run a new benchmark. CI: https://ci.nodejs.org/job/node-test-pull-request/12484/ |
@AndreasMadsen Should that benchmark CI run that you triggered be expected to run 40 hours wall clock time, like you estimated? That might be nice to know to plan other benchmark CI runs… |
@addaleax Oh, yeah – sorry. I forgot about that estimate. Although, I'm not really sure what the alternative is right now. It would be nice if we were more strict about our benchmarks such that they don't take so much time. But that is a major initiative. |
There are 4 significant results at a 95% confidence level. Since this benchmark contains 144 combinations and there is a 5% risk for false positives, we should expect
|
Landed in 8479606 |
I think this will solve the issue: #18143 if not, we should revert this. |
If IPv6 is not supported on a machine, the IPv6 handle will first be created, this will then fail and default to an IPv4 handle. This causes the graph to change, as there now is an extra handle. PR-URL: #18143 Fixes: #18003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This allows more easy tracking of where HTTP requests come from. Before this change the HTTPParser would have the HTTPServer as the triggerAsyncId. The HTTPParser will still have the executionAsyncId set to the HTTP Server so that information is still directly available. Indirectly, the TCP socket itself also has its triggerAsyncId set to the TCP Server. PR-URL: nodejs#18003 Backport-PR-URL: nodejs#18179 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
If IPv6 is not supported on a machine, the IPv6 handle will first be created, this will then fail and default to an IPv4 handle. This causes the graph to change, as there now is an extra handle. PR-URL: nodejs#18143 Backport-PR-URL: nodejs#18179 Fixes: nodejs#18003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This allows more easy tracking of where HTTP requests come from. Before this change the HTTPParser would have the HTTPServer as the triggerAsyncId. The HTTPParser will still have the executionAsyncId set to the HTTP Server so that information is still directly available. Indirectly, the TCP socket itself also has its triggerAsyncId set to the TCP Server. Backport-PR-URL: #18179 PR-URL: #18003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
If IPv6 is not supported on a machine, the IPv6 handle will first be created, this will then fail and default to an IPv4 handle. This causes the graph to change, as there now is an extra handle. Backport-PR-URL: #18179 PR-URL: #18143 Fixes: #18003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Although this lands cleanly on v9.x-staging, the tests fail. If you would like to see it land in v9.x, can you please submit a backport PR? Thanks! |
@evanlucas |
@evanlucas Sorry, wrong PR. I have done the full "backport" (no changes) #18474 |
This allows more easy tracking of where HTTP requests come from. Before this change the HTTPParser would have the HTTPServer as the triggerAsyncId. The HTTPParser will still have the executionAsyncId set to the HTTP Server so that information is still directly available. Indirectly, the TCP socket itself also has its triggerAsyncId set to the TCP Server. Backport-PR-URL: #18474 PR-URL: #18003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
If IPv6 is not supported on a machine, the IPv6 handle will first be created, this will then fail and default to an IPv4 handle. This causes the graph to change, as there now is an extra handle. Backport-PR-URL: #18474 PR-URL: #18143 Fixes: #18003 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.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) [#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
This allows more easy tracking of where HTTP requests come from. Before
this change the HTTPParser would have the HTTPServer as the
triggerAsyncId.
The HTTPParser will still have the executionAsyncId set to the HTTP
Server so that information is still directly available. Indirectly, the
TCP socket itself also has its triggerAsyncId set to the TCP Server.
NOTE: this should properly be benchmarked
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
async_hooks, http
/cc @nodejs/async_hooks