-
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
Add scheduling policy to cluster.settings #49292
base: main
Are you sure you want to change the base?
Changes from all commits
13ad0b9
7b40262
cfde107
55af83d
4421342
0648d42
0568aa6
c5a082b
e4a3101
06e0916
9f797a2
0929cc4
9d60e38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,6 @@ cluster.SCHED_RR = SCHED_RR; // Primary distributes connections. | |
let ids = 0; | ||
let initialized = false; | ||
|
||
// XXX(bnoordhuis) Fold cluster.schedulingPolicy into cluster.settings? | ||
let schedulingPolicy = process.env.NODE_CLUSTER_SCHED_POLICY; | ||
if (schedulingPolicy === 'rr') | ||
schedulingPolicy = SCHED_RR; | ||
|
@@ -65,6 +64,7 @@ cluster.setupPrimary = function(options) { | |
exec: process.argv[1], | ||
execArgv: process.execArgv, | ||
silent: false, | ||
schedulingPolicy: cluster.schedulingPolicy, | ||
...cluster.settings, | ||
...options, | ||
}; | ||
|
@@ -86,9 +86,10 @@ cluster.setupPrimary = function(options) { | |
return process.nextTick(setupSettingsNT, settings); | ||
|
||
initialized = true; | ||
schedulingPolicy = cluster.schedulingPolicy; // Freeze policy. | ||
schedulingPolicy = settings.schedulingPolicy; // Freeze policy. | ||
assert(schedulingPolicy === SCHED_NONE || schedulingPolicy === SCHED_RR, | ||
`Bad cluster.schedulingPolicy: ${schedulingPolicy}`); | ||
`Bad settings.schedulingPolicy: ${schedulingPolicy}`); | ||
cluster.schedulingPolicy = schedulingPolicy; // Show to the world. | ||
|
||
process.nextTick(setupSettingsNT, settings); | ||
|
||
|
@@ -158,6 +159,10 @@ function removeHandlesForWorker(worker) { | |
}); | ||
} | ||
|
||
cluster.getSettings = function() { | ||
return { ...cluster.settings }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To copy it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And node.js does to offer helpers to shallow and/or deep clone an object to make it explicit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK using the spread is the idiomatic way of copying an object in JavaScript. |
||
}; | ||
|
||
cluster.fork = function(env) { | ||
cluster.setupPrimary(); | ||
const id = ++ids; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
// Refs: https://github.com/nodejs/node/issues/49240 | ||
// When importing cluster in ESM, cluster.schedulingPolicy cannot be set; | ||
// and the env variable doesn't work since imports are hoisted to the top. | ||
// Therefore the scheduling policy is still cluster.SCHED_RR. | ||
import '../common/index.mjs'; | ||
import assert from 'node:assert'; | ||
import * as cluster from 'cluster'; | ||
|
||
|
||
assert.strictEqual(cluster.schedulingPolicy, cluster.SCHED_RR); | ||
cluster.setupPrimary({ schedulingPolicy: cluster.SCHED_NONE }); | ||
const settings = cluster.getSettings(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could be mistaken, but since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this doesn't need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, I'm afraid getSettings() is being made public. I guess I would need to do some wizardry in lib/cluster.js to make it really internal 🤔 Should I do it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not export |
||
assert.strictEqual(settings.schedulingPolicy, cluster.SCHED_RR); |
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 needs docs
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.
@bnoordhuis asked for
getSettings()
to be internal in #49240 (comment), I understand it should not be documented then? From my side I think it makes sense to make it public and document it, but prefer to wait for his opinion.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.
Yeah, internal stuff should not be in the docs. If there is anything not self-evident about it, please leave code comments, but that goes for everything.
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.
The current implementation is public property. If that method is expected to be internal, you need to make it non enumerable or whatever node.js uses to distinguish between public and internal APIs. But I still do not understand why folding a property into an existing object literal property needs a new getter to be added. The two are orthogonal. And if you make an PR for the folding and one for the API change, the former will not be encumbered by API changes discussion that always takes time.
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 have documented the property
getSettings()
so it can be made public. It needs the new getter because the old way to expose the whole objectsettings
does not play well with ESM, which makes thesettings
object immutable. I believe the oldsettings
should therefore be made deprecated, but have not done so because I proposed it and nobody seconded the option.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.
Hmmm, what induces you to think that this PR is an ugly hack? It is indeed a pretty elegant solution if I may so myself, only deprecating the public use of
settings
is in fact missing to be 100% right at least for me.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.
A real elegant solution would have not changed a public API to support ESM ;)
And I've not written that PR is an ugly hack ;) I've written that supporting ESM/CJS in code using cluster API (the API consumer) and expected to run on various node.js versions will require ugly hacks. And it could be avoided.
Anyhow, ESM support in cluster is still buggy. Worker as ESM module fails to init.
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.
Good luck with that.
I await your proposal.
It works fine for me 🤔
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.
cluster.prototype.settings = function() {}
/Object.defineProperty(cluster, 'settings', { get: function () {}})
should works with ESM and CJS.Is it prohibited in node.js internal JS code?
Not if you use a dedicated ESM file as a cluster worker: #48578
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.
You are aware that this change breaks backwards compatibility in CJS as it does not allow modifying
cluster.settings
, as it used to do.Oh, of course my PR doesn't fix all issues. It fixes setting
schedulingPolicy
as stated.