Skip to content
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

Open
wants to merge 13 commits 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
45 changes: 45 additions & 0 deletions doc/api/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,45 @@ Spawn a new worker process.

This can only be called from the primary process.

## `cluster.getSettings()`

<!-- YAML
added: REPLACEME
-->

* Returns: {Object}
* `execArgv` {string\[]} List of string arguments passed to the Node.js
executable. **Default:** `process.execArgv`.
* `exec` {string} File path to worker file. **Default:** `process.argv[1]`.
* `args` {string\[]} String arguments passed to worker.
**Default:** `process.argv.slice(2)`.
* `cwd` {string} Current working directory of the worker process. **Default:**
`undefined` (inherits from parent process).
* `serialization` {string} Specify the kind of serialization used for sending
messages between processes. Possible values are `'json'` and `'advanced'`.
See [Advanced serialization for `child_process`][] for more details.
**Default:** `false`.
* `schedulingPolicy` {string} Scheduling policy to use between processes.
**Default:** `cluster.SCHED_RR`. See \[`cluster.schedulingPolicy`]\[] for
details.
* `silent` {boolean} Whether or not to send output to parent's stdio.
**Default:** `false`.
* `stdio` {Array} Configures the stdio of forked processes. Because the
cluster module relies on IPC to function, this configuration must contain an
`'ipc'` entry. When this option is provided, it overrides `silent`. See
[`child_process.spawn()`][]'s [`stdio`][].
* `uid` {number} Sets the user identity of the process. (See setuid(2).)
* `gid` {number} Sets the group identity of the process. (See setgid(2).)
* `inspectPort` {number|Function} Sets inspector port of worker.
This can be a number, or a function that takes no arguments and returns a
number. By default each worker gets its own port, incremented from the
primary's `process.debugPort`.
* `windowsHide` {boolean} Hide the forked processes console window that would
normally be created on Windows systems. **Default:** `false`.

After calling [`.setupPrimary()`][] (or [`.fork()`][]) this function will return
the cluster settings, including the default values.

## `cluster.isMaster`

<!-- YAML
Expand Down Expand Up @@ -900,6 +939,9 @@ values are `'rr'` and `'none'`.
<!-- YAML
added: v0.7.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49292
description: The `schedulingPolicy` option is supported now.
- version:
- v13.2.0
- v12.16.0
Expand Down Expand Up @@ -931,6 +973,9 @@ changes:
messages between processes. Possible values are `'json'` and `'advanced'`.
See [Advanced serialization for `child_process`][] for more details.
**Default:** `false`.
* `schedulingPolicy` {string} Scheduling policy to use between processes.
**Default:** `cluster.SCHED_RR`. See \[`cluster.schedulingPolicy`]\[] for
details.
* `silent` {boolean} Whether or not to send output to parent's stdio.
**Default:** `false`.
* `stdio` {Array} Configures the stdio of forked processes. Because the
Expand Down
11 changes: 8 additions & 3 deletions lib/internal/cluster/primary.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -65,6 +64,7 @@ cluster.setupPrimary = function(options) {
exec: process.argv[1],
execArgv: process.execArgv,
silent: false,
schedulingPolicy: cluster.schedulingPolicy,
...cluster.settings,
...options,
};
Expand All @@ -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);

Expand Down Expand Up @@ -158,6 +159,10 @@ function removeHandlesForWorker(worker) {
});
}

cluster.getSettings = function() {
Copy link
Member

Choose a reason for hiding this comment

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

This needs docs

Copy link
Author

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.

Copy link
Member

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.

Copy link
Contributor

@jerome-benoit jerome-benoit May 12, 2024

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.

Copy link
Author

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 object settings does not play well with ESM, which makes the settings object immutable. I believe the old settings should therefore be made deprecated, but have not done so because I proposed it and nobody seconded the option.

Copy link
Author

@alexfernandez alexfernandez May 12, 2024

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.

Copy link
Contributor

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.

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.

Copy link
Author

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 ;)

Good luck with that.

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.

I await your proposal.

Anyhow, ESM support in cluster is still buggy. Worker as ESM module fails to init.

It works fine for me 🤔

Copy link
Contributor

@jerome-benoit jerome-benoit May 12, 2024

Choose a reason for hiding this comment

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

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.

I await your proposal.

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?

Anyhow, ESM support in cluster is still buggy. Worker as ESM module fails to init.

It works fine for me 🤔

Not if you use a dedicated ESM file as a cluster worker: #48578

Copy link
Author

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?

You are aware that this change breaks backwards compatibility in CJS as it does not allow modifying cluster.settings, as it used to do.

Anyhow, ESM support in cluster is still buggy. Worker as ESM module fails to init.

It works fine for me 🤔

Not if you use a dedicated ESM file as a cluster worker: #48578

Oh, of course my PR doesn't fix all issues. It fixes setting schedulingPolicy as stated.

return { ...cluster.settings };
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't cluster.settings already an object literal? Why using spread on it?

Copy link
Member

Choose a reason for hiding this comment

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

To copy it

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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;
Expand Down
13 changes: 13 additions & 0 deletions test/known_issues/test-cluster-import-scheduling-policy.mjs
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();
Copy link
Member

Choose a reason for hiding this comment

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

I could be mistaken, but since getSettings() is an internal API, does this test need the --expose-internals flag?

Copy link
Member

Choose a reason for hiding this comment

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

If this doesn't need --expose-internals, then I imagine we're exposing getSettings() publicly unintentionally......

Copy link
Author

@alexfernandez alexfernandez Nov 1, 2023

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

lib/cluster.js imports lib/internal/cluster/primary.js and re-exports it. So the options would seem to be to either re-engineer lib/cluster.js so it re-exports a property and not the whole module--shouldn't be too hard to do but you'll need to do it in a few places because it treats lib/internal/child.js the same way--or to find another place to export getSettings() from (such as utils.js but that doesn't import primary currently). I think the first option is the way to go, but other people may have more informed opinions.

Copy link
Author

Choose a reason for hiding this comment

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

Why not export getSettings() from cluster itself? And document it, marking cluster.settings as deprecated. Exporting attributes is not a good practice in the age of imports anyway.

assert.strictEqual(settings.schedulingPolicy, cluster.SCHED_RR);
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 @@ -35,6 +35,7 @@ assert.deepStrictEqual(cluster.settings, {
exec: process.argv[1],
execArgv: process.execArgv,
silent: false,
schedulingPolicy: 2,
});
console.log('ok sets defaults');

Expand All @@ -58,5 +59,6 @@ assert.deepStrictEqual(cluster.settings, {
exec: 'overridden',
execArgv: ['baz', 'bang'],
silent: false,
schedulingPolicy: 2,
});
console.log('ok preserves current settings');