-
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
dgram: extract cluster lazy loading method to make it testable #38563
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.
Instead of using the raw cluster
object, what do you think about using lazyLoadCluster()
?
function lazyLoadCluster() {
return cluster ??= require('cluster');
}
That way, the check for whether the module has been loaded is abstracted away and the original internal API too does not change.
Good idea. In this way the lazy load method can be tested. |
Fast-track has been requested by @jasnell. Please 👍 to approve. |
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.
I don't think any of this touches a hot code path, but just to be extra cautious, I'll kick off a benchmark CI.
Totally unnecessary benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1017/ Results
|
PR-URL: nodejs#38563 Refs: https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Landed in 1b11398 |
PR-URL: #38563 Refs: https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #38563 Refs: https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #38563 Refs: https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #38563 Refs: https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #38563 Refs: https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
There is no need to ensurecluster
loaded inbindServerHandle
becauseit's already loaded before calling it.
This PR makescluster
as an argument to remind developers thatcluster
needs to be loaded before calling this function.
Update:
The lazy load parts in
bindServerHandle
is unreachable becausecluster
has been loaded before calling it. Extract it to a seperated method to make the code testable.Refs:
https://coverage.nodejs.org/coverage-26e318a321a872bc/lib/dgram.js.html#L202