Add scheduling policy to cluster.settings #49292

alexfernandez · 2023-08-22T21:28:32Z

As reported in #49240, when using ESM it is quite hard to modify cluster.schedulingPolicy, since the property becomes immutable on import and the environment variable cannot be set in code since the import is hoisted to the top. Setting the env variable in the console is often not desirable. The only workaround I found so far is to do a dynamic import() after setting the env variable in code, which is cumbersome.

The solution implemented here, as suggested by @bnoordhuis in a comment, is to add schedulingPolicy to the cluster.settings object:

import * as cluster from 'cluster';
cluster.setupPrimary({ schedulingPolicy: cluster.SCHED_NONE });

This code now works properly as expected.

@bnoordhuis also recommended that a cluster.getSettings() function was not desirable at this point; instead an internal method should be used. I'm not sure however how this internal method is marked, so I added it without documenting it for now.

Documented in doc/api/cluster.md. Test added in test/known_issues/test-cluster-import-scheduling-policy.mjs. All tests running.

Possible improvements:

Add test with CJS.
Document cluster.getSettings() so that it can be used in ESM, where cluster.settings will also be immutable on import and the user will not be able to tell what the final settings are.
Mark cluster.schedulingPolicy as deprecated since it is now redundant.

RFC, please review.

Fixes: #49240

lib/internal/cluster/primary.js

benjamingr · 2023-08-27T19:00:03Z

@nodejs/cluster @bnoordhuis

benjamingr · 2023-08-27T19:00:26Z

lib/internal/cluster/primary.js

@@ -158,6 +161,10 @@ function removeHandlesForWorker(worker) {
  });
 }

+cluster.getSettings = function() {


This needs docs

@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.

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.

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.

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.

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.

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.

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 🤔

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

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.

doc/api/cluster.md

mcollina

lgtm

nodejs-github-bot · 2023-10-23T06:57:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55139/

Fixes: nodejs#49240

Co-authored-by: Matteo Collina <[email protected]>

Trott · 2023-11-01T22:56:58Z

test/known_issues/test-cluster-import-scheduling-policy.mjs

+
+assert.strictEqual(cluster.schedulingPolicy, cluster.SCHED_RR);
+cluster.setupPrimary({ schedulingPolicy: cluster.SCHED_NONE });
+const settings = cluster.getSettings();


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

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

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?

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.

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.

nodejs-github-bot · 2023-11-11T16:27:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55573/

nodejs-github-bot · 2023-11-12T11:25:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55595/

nodejs-github-bot · 2024-05-11T14:27:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59108/

benjamingr

Still needs docs for getSettings (or to not expose it) before landing

…z/node into fix/schedulingPolicy-import

alexfernandez · 2024-05-12T11:42:08Z

Still needs docs for getSettings (or to not expose it) before landing

Documented getSettings() in 9d60e38.

jerome-benoit · 2024-05-12T11:52:11Z

lib/internal/cluster/primary.js

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

+cluster.getSettings = function() {
+  return { ...cluster.settings };


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

And node.js does to offer helpers to shallow and/or deep clone an object to make it explicit?

AFAIK using the spread is the idiomatic way of copying an object in JavaScript.

mcollina

lgtm

nodejs-github-bot · 2024-05-12T12:23:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59172/

nodejs-github-bot · 2024-05-12T19:04:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59185/

jerome-benoit · 2024-05-13T08:49:48Z

+cluster.getSettings = function() { 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.

The use of `Object.defineProperty()` could introduce transparent getter/setter with ESM support on existing property. And even warn for deprecation in the setter case for `setupPrimary()` usage instead.

…

-- Jérôme Benoit aka fraggle Piment Noir - https://piment-noir.org OpenPGP Key ID : 27B535D3 Key fingerprint : B799 BBF6 8EC8 911B B8D7 CDBC C3B1 92C6 27B5 35D3

alexfernandez · 2024-05-13T20:04:58Z

The use of Object.defineProperty() could introduce transparent getter/setter with ESM support on existing property. And even warn for deprecation in the setter case for setupPrimary() usage instead.

I'm not sure I understand your message, but I'm sure I cannot bring anything else to the table so I'm bowing out, thanks for the discussion.

jerome-benoit · 2024-05-16T18:02:27Z

The use of Object.defineProperty() could introduce transparent getter/setter with ESM support on existing property. And even warn for deprecation in the setter case for setupPrimary() usage instead. I'm not sure I understand your message, but I'm sure I cannot bring anything else to the table so I'm bowing out, thanks for the discussion.

cluster.settings -> cluster._settings, a non public writable property (at least non enumerable) cluster.settings = Object.defineProperty(cluster, ’settings', { get: function() { return cluster._settings }, }); If settings is set through its properties, cluster.settings.value1 = foo, cluster.settings.value1 = bar, it will work If settings is set as an plain literal object, adding a setter will be needed: cluster.settings = Object.defineProperty(cluster, ’settings', { get: function() { return cluster._settings }, set: function(settings) { // Sanity checks on settings type? // Warn about deprecation? cluster._settings = settings }, }); If all of that is doable in internal node.js JS code, you have an ESM compatible implementation of cluster.settings without introducing cluster API changes. Is it more clear now?

…

-- Jérôme Benoit aka fraggle Piment Noir - https://piment-noir.org OpenPGP Key ID : 27B535D3 Key fingerprint : B799 BBF6 8EC8 911B B8D7 CDBC C3B1 92C6 27B5 35D3

alexfernandez marked this pull request as draft August 22, 2023 21:28

nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run. labels Aug 22, 2023

alexfernandez mentioned this pull request Aug 22, 2023

Cannot set up cluster.schedulingPolicy in ESM #49240

Open

benjamingr reviewed Aug 27, 2023

View reviewed changes

lib/internal/cluster/primary.js Outdated Show resolved Hide resolved

benjamingr reviewed Aug 27, 2023

View reviewed changes

alexfernandez force-pushed the fix/schedulingPolicy-import branch from fc576fb to 6617c78 Compare August 28, 2023 00:36

alexfernandez marked this pull request as ready for review October 19, 2023 17:15

mcollina reviewed Oct 22, 2023

View reviewed changes

doc/api/cluster.md Outdated Show resolved Hide resolved

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

mcollina approved these changes Oct 23, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

This was referenced Oct 24, 2023

CI Reliability 2023-10-24 nodejs/reliability#696

Open

CI Reliability 2023-10-25 nodejs/reliability#697

Open

alexfernandez and others added 9 commits November 1, 2023 20:18

cluster: fold schedulingPolicy into cluster.settings

13ad0b9

Fixes: nodejs#49240

cluster: add internal cluster.getSettings() for test

7b40262

cluster: add test for cluster.setPrimary({schedulingPolicy})

cfde107

cluster: document cluster.setPrimary({schedulingPolicy})

55af83d

cluster: remove comment from bnoordhuis

4421342

As suggested by mcollina

0648d42

Co-authored-by: Matteo Collina <[email protected]>

As suggested by jerome-benoit: remove comment

0568aa6

Fix tests

c5a082b

Add pull request number

e4a3101

alexfernandez force-pushed the fix/schedulingPolicy-import branch from ca645e1 to e4a3101 Compare November 1, 2023 19:19

cluster: fix test, revert known_issue so it fails now

06e0916

Trott reviewed Nov 1, 2023

View reviewed changes

github-actions bot mentioned this pull request Nov 12, 2023

CI Reliability 2023-11-12 nodejs/reliability#715

Open

18 tasks

This was referenced Nov 13, 2023

CI Reliability 2023-11-13 nodejs/reliability#716

Open

CI Reliability 2023-11-14 nodejs/reliability#717

Open

CI Reliability 2023-11-15 nodejs/reliability#718

Open

cluster: add scheduling policy to cluster.settings

9f797a2

aduh95 force-pushed the fix/schedulingPolicy-import branch from b09b1d1 to 9f797a2 Compare May 11, 2024 14:26

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024

benjamingr requested changes May 11, 2024

View reviewed changes

aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 11, 2024

alexfernandez added 2 commits May 12, 2024 13:41

Merge branch 'fix/schedulingPolicy-import' of github.com:alexfernande…

0929cc4

…z/node into fix/schedulingPolicy-import

Document getSettings()

9d60e38

jerome-benoit reviewed May 12, 2024

View reviewed changes

mcollina approved these changes May 12, 2024

View reviewed changes

benjamingr approved these changes May 12, 2024

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scheduling policy to cluster.settings #49292

Add scheduling policy to cluster.settings #49292

alexfernandez commented Aug 22, 2023 •

edited by aduh95

Loading

benjamingr commented Aug 27, 2023

benjamingr Aug 27, 2023

alexfernandez Aug 28, 2023

Trott Nov 1, 2023

jerome-benoit May 12, 2024 •

edited

Loading

alexfernandez May 12, 2024

alexfernandez May 12, 2024 •

edited

Loading

jerome-benoit May 12, 2024

alexfernandez May 12, 2024

jerome-benoit May 12, 2024 •

edited

Loading

alexfernandez May 12, 2024

mcollina left a comment

nodejs-github-bot commented Oct 23, 2023

Trott Nov 1, 2023

Trott Nov 1, 2023

alexfernandez Nov 1, 2023 •

edited

Loading

Trott Nov 1, 2023

alexfernandez Nov 11, 2023

nodejs-github-bot commented Nov 11, 2023

nodejs-github-bot commented Nov 12, 2023

nodejs-github-bot commented May 11, 2024

benjamingr left a comment

alexfernandez commented May 12, 2024 •

edited

Loading

jerome-benoit May 12, 2024

benjamingr May 12, 2024

jerome-benoit May 12, 2024

alexfernandez May 12, 2024

mcollina left a comment

nodejs-github-bot commented May 12, 2024

nodejs-github-bot commented May 12, 2024

jerome-benoit commented May 13, 2024 via email •

edited

Loading

alexfernandez commented May 13, 2024

jerome-benoit commented May 16, 2024 via email •

edited

Loading

Add scheduling policy to cluster.settings #49292

Are you sure you want to change the base?

Add scheduling policy to cluster.settings #49292

Conversation

alexfernandez commented Aug 22, 2023 • edited by aduh95 Loading

benjamingr commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-benoit May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfernandez May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-benoit May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfernandez Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Nov 11, 2023

nodejs-github-bot commented Nov 12, 2023

nodejs-github-bot commented May 11, 2024

benjamingr left a comment

Choose a reason for hiding this comment

alexfernandez commented May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 12, 2024

nodejs-github-bot commented May 12, 2024

jerome-benoit commented May 13, 2024 via email • edited Loading

alexfernandez commented May 13, 2024

jerome-benoit commented May 16, 2024 via email • edited Loading

alexfernandez commented Aug 22, 2023 •

edited by aduh95

Loading

jerome-benoit May 12, 2024 •

edited

Loading

alexfernandez May 12, 2024 •

edited

Loading

jerome-benoit May 12, 2024 •

edited

Loading

alexfernandez Nov 1, 2023 •

edited

Loading

alexfernandez commented May 12, 2024 •

edited

Loading

jerome-benoit commented May 13, 2024 via email •

edited

Loading

jerome-benoit commented May 16, 2024 via email •

edited

Loading