Skip to content

cluster: add schedulingPolicy to settings #41731

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/api/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,8 @@ changes:
primary's `process.debugPort`.
* `windowsHide` {boolean} Hide the forked processes console window that would
normally be created on Windows systems. **Default:** `false`.
* `schedulingPolicy` {string} policy to distribute load among the
secondary processes.

After calling [`.setupPrimary()`][] (or [`.fork()`][]) this settings object will
contain the settings, including the default values.
Expand Down
39 changes: 21 additions & 18 deletions lib/internal/cluster/primary.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,32 +42,37 @@ cluster.SCHED_RR = SCHED_RR; // Primary distributes connections.
let ids = 0;
let debugPortOffset = 1;
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;
else if (schedulingPolicy === 'none')
schedulingPolicy = SCHED_NONE;
else if (process.platform === 'win32') {
// Round-robin doesn't perform well on
// Windows due to the way IOCP is wired up.
schedulingPolicy = SCHED_NONE;
} else
schedulingPolicy = SCHED_RR;

cluster.schedulingPolicy = schedulingPolicy;
Copy link
Contributor

@mscdex mscdex Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep this (cluster.schedulingPolicy) and consider its value to avoid breaking things.

let schedulingPolicy = SCHED_RR;

const SCHEDULE_POLICY_NAME_MAP = new SafeMap()
.set('rr', SCHED_RR)
.set('none', SCHED_NONE);

function processSchedulingPolicy(settings = {}) {
const schedulingPolicy = process.env.NODE_CLUSTER_SCHED_POLICY || settings.schedulingPolicy;
if (schedulingPolicy === 'rr' || schedulingPolicy === 'none')
return schedulingPolicy;
if (process.platform === 'win32') {
// Round-robin doesn't perform well on
// Windows due to the way IOCP is wired up.
return 'none';
}
return 'rr';
}

cluster.setupPrimary = function(options) {
const settings = {
args: ArrayPrototypeSlice(process.argv, 2),
exec: process.argv[1],
execArgv: process.execArgv,
silent: false,
schedulingPolicy: cluster.schedulingPolicy || processSchedulingPolicy(options),
...cluster.settings,
...options
};

assert(settings.schedulingPolicy);

// Tell V8 to write profile data for each process to a separate file.
// Without --logfile=v8-%p.log, everything ends up in a single, unusable
// file. (Unusable because what V8 logs are memory addresses and each
Expand All @@ -85,9 +90,7 @@ cluster.setupPrimary = function(options) {
return process.nextTick(setupSettingsNT, settings);

initialized = true;
schedulingPolicy = cluster.schedulingPolicy; // Freeze policy.
assert(schedulingPolicy === SCHED_NONE || schedulingPolicy === SCHED_RR,
`Bad cluster.schedulingPolicy: ${schedulingPolicy}`);
schedulingPolicy = SCHEDULE_POLICY_NAME_MAP.get(cluster.schedulingPolicy); // Freeze policy.
Copy link
Contributor

@mscdex mscdex Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be keeping the assert() to ensure settings.schedulingPolicy maps to a supported value in SCHEDULE_POLICY_NAME_MAP?

Copy link

@Mifrill Mifrill Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, it should be handled via SafeMap above:
https://github.com/yashLadha/node/blob/9027b55935784c5e40ac54db5919895ad1636f16/lib/internal/cluster/primary.js#L47-L49
so the value of settings.schedulingPolicy can be only supported in SCHEDULE_POLICY_NAME_MAP. Hence the assert is redundant in this particular case.


process.nextTick(setupSettingsNT, settings);

Expand Down
2 changes: 2 additions & 0 deletions test/parallel/test-cluster-setup-primary-cumulative.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ assert.deepStrictEqual(cluster.settings, {
args: process.argv.slice(2),
exec: process.argv[1],
execArgv: process.execArgv,
schedulingPolicy: 'rr',
silent: false,
});
console.log('ok sets defaults');
Expand All @@ -57,6 +58,7 @@ assert.deepStrictEqual(cluster.settings, {
args: ['foo', 'bar'],
exec: 'overridden',
execArgv: ['baz', 'bang'],
schedulingPolicy: 'rr',
silent: false,
});
console.log('ok preserves current settings');