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

os: add fallback for undefined CPUs #25493

Closed
wants to merge 1 commit into from
Closed

os: add fallback for undefined CPUs #25493

wants to merge 1 commit into from

Conversation

JungMinu
Copy link
Member

Currently, for an unsupported OS, a call to os.cpus() throws an error within os.cpus() itself where it tries to get the length of undefined. This PR fixes the issue by adding fallback for undefined CPUs.

Fixes: #25483

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@JungMinu sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added the os Issues and PRs related to the os subsystem. label Jan 14, 2019
@JungMinu JungMinu self-assigned this Jan 14, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I’m wondering if it might make sense to add a “real” fallback, i.e. a dummy result that lists 1 CPU or so?

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

I'd rather this threw. It's not accurate to return no cpus or one fake cpu. If this function can't find cpus it shouldn't make something up.

@addaleax
Copy link
Member

@devsnek For context, this is a bugfix for a regression introduced in 51cea61. I think intentionally introducing a throw should be considered semver-major.

lib/os.js Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jan 15, 2019

@devsnek For context, this is a bugfix for a regression introduced in 51cea61. I think intentionally introducing a throw should be considered semver-major.

Does this affect any supported platforms?

Assuming this only affects unsupported platforms, I'd like to see us introduce the throw, but in a major release. (If it only affects unsupported platforms, I wouldn't consider it semver-major. But doing it in a major release anyway is the kind thing to do, so I'm 👍with that.) The behavior here will be difficult to test robustly unless it applies to a supported platform and configuration that we can test in CI. So removing the bits of code we can't test and that don't affect support platforms/configurations seems like the right thing to do.

For an unsupported OS, a call to os.cpus() throws an error
within os.cpus() itself where it tries to get the length of it.
This fixes the issue by adding fallback for undefined CPUs.

Fixes: #25483
PR-URL: #25493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@JungMinu JungMinu closed this Jan 16, 2019
@JungMinu JungMinu deleted the fix-25483 branch January 16, 2019 03:46
BridgeAR pushed a commit that referenced this pull request Jan 16, 2019
For an unsupported OS, a call to os.cpus() throws an error
within os.cpus() itself where it tries to get the length of it.
This fixes the issue by adding fallback for undefined CPUs.

Fixes: #25483
PR-URL: #25493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
For an unsupported OS, a call to os.cpus() throws an error
within os.cpus() itself where it tries to get the length of it.
This fixes the issue by adding fallback for undefined CPUs.

Fixes: nodejs#25483
PR-URL: nodejs#25493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
For an unsupported OS, a call to os.cpus() throws an error
within os.cpus() itself where it tries to get the length of it.
This fixes the issue by adding fallback for undefined CPUs.

Fixes: nodejs#25483
PR-URL: nodejs#25493
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os Issues and PRs related to the os subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

os.cpus: Cannot read property 'length' of undefined
10 participants