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

Conversation

alexfernandez
Copy link

@alexfernandez alexfernandez commented Aug 22, 2023

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

@alexfernandez alexfernandez marked this pull request as draft August 22, 2023 21:28
@nodejs-github-bot 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
@benjamingr
Copy link
Member

@nodejs/cluster @bnoordhuis

@@ -158,6 +161,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.

@alexfernandez alexfernandez marked this pull request as ready for review October 19, 2023 17:15
doc/api/cluster.md Outdated Show resolved Hide resolved
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

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


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.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 force-pushed the fix/schedulingPolicy-import branch from b09b1d1 to 9f797a2 Compare May 11, 2024 14:26
@aduh95 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
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

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

@aduh95 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
Copy link
Author

alexfernandez commented May 12, 2024

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

Documented getSettings() in 9d60e38.

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

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jerome-benoit
Copy link
Contributor

jerome-benoit commented May 13, 2024 via email

@alexfernandez
Copy link
Author

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
Copy link
Contributor

jerome-benoit commented May 16, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set up cluster.schedulingPolicy in ESM
7 participants