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.

Sign up for GitHub

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
+68 −3
Open

Add scheduling policy to cluster.settings #49292

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
13ad0b9
cluster: fold schedulingPolicy into cluster.settings
alexfernandez Aug 22, 2023
7b40262
cluster: add internal cluster.getSettings() for test
alexfernandez Aug 22, 2023
cfde107
cluster: add test for cluster.setPrimary({schedulingPolicy})
alexfernandez Aug 22, 2023
55af83d
cluster: document cluster.setPrimary({schedulingPolicy})
alexfernandez Aug 22, 2023
4421342
cluster: remove comment from bnoordhuis
alexfernandez Aug 28, 2023
0648d42
As suggested by mcollina
alexfernandez Oct 22, 2023
0568aa6
As suggested by jerome-benoit: remove comment
alexfernandez Oct 22, 2023
c5a082b
Fix tests
alexfernandez Nov 1, 2023
e4a3101
Add pull request number
alexfernandez Nov 1, 2023
06e0916
cluster: fix test, revert known_issue so it fails now
alexfernandez Nov 1, 2023
9f797a2
cluster: add scheduling policy to `cluster.settings`
alexfernandez Aug 22, 2023
0929cc4
Merge branch 'fix/schedulingPolicy-import' of github.com:alexfernande…
alexfernandez May 12, 2024
9d60e38
Document getSettings()
alexfernandez May 12, 2024
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
6 changes: 6 additions & 0 deletions doc/api/cluster.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,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 +934,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
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
View file
Open in desktop
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 •
edited
Loading

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
View file
Open in desktop
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');