doc: include undefined in os.cpus() return values

[Trott]

Document that os.cpus() can return undefined if information about
cores is not available. This can happen particularly on unsupported
platforms like Android.

Fixes: #19022

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1645/pipeline

[Trott]

👍 here to fast-track.

[sam-github]

I'm not sure the current behaviour of undefined instead of throwing an exception is correct. But, its how it works now, so the API needs to be documented, warts and all. It can be fixed later.

The thing is, this can only happen on an unsupported platform, and the docs you add make it sound like it could happen anywhere. So, properly defensive coders will start having to code around this issue, because they can't know (not from the docs) under what conditions a computer wouldn't know that it is running on CPUs.

I can see how you didn't want to add a special note about Android here. But on the other hand, unnecessarily raising the fear of undefined in our API isn't great, either.

Sorry, I think it needs time for more comment before landing.

[Trott]

@sam-github I've edited the text to hopefully address the concerns you raise. PTAL

(By the way, based on your comments, I removed the fast-track label. In general, you should feel free to do that yourself to minimize the chance that someone overlooks your concerns.)

[starkwang]

Android is not a supported platform for now. It maybe unnecessary to document the behavior of these unsupported platforms.

Is it possible for os.cpus() to return undefined on our supported platforms? If yes, I'm +1 on this document change.

[sam-github]

I'm OK with the current text. I'm honestly not that clear on what our duty should be to doc platforms we don't support should be. If our APIs don't work the same on PalmOS, I don't think we should litter the docs with notes about that. But Android is kindof Linux... so... what's its deal? Is it linux-x64, but with the fs mounted in such a way we can't get CPU info? Or is it a custom build of node, in which case we shouldn't try to docs its idiosyncracies? What would we do if some of our APIs don't work in Electron? I don't think we'd start docing that. Just raising these issues now, so when the next oddity comes along people don't just point at the os.cpus() docs as a precedent.

[Trott]

@sam-github TBH I'm on the fence myself. When we say something is unsupported, part of the idea (in my mind, anyway) is that we don't need to right documentation about it beyond the fact that it's unsupported. But I also agree that Android is a bit of a special case. It's unsupported but it's also something we encourage people to submit patches for and stuff, so I think a fair number of us would love to be able to support it, much more so than other unsupported OSes I think.

[Trott]

Is it possible for os.cpus() to return undefined on our supported platforms? If yes, I'm +1 on this document change.

@starkwang I don't know the answer to that question. Honestly, looking at the code in os.js and node_os.cc, I'm not even sure how it happens on unsupported platforms. Seems like it should return an empty array if it can't find any info.

@trusktr Can you confirm the return value of os.cpus() on your Android? undefined or [] or something else?

[Trott]

Oh, I think I see what happens. getCPUs() returns undefined, and then os.cpus() tries to get the length, and it results in an error. Or at least, that's what seems plausible to me from the code.

In that case, this doc change is wrong, and honestly I do think this is a case where we shrug and say, "Sorry, Android is unsupported. Patches are welcome, and we'd love to support it if someone can help us get it set up in our CI infrastructure."

I'm going to close this, but will happily re-open it if it turns out my analysis in this comment is wrong.

[Trott]

Ah! The relevant code was changed 3 days ago on the master branch! #24264 51cea61

Up until then, os.cpus() could indeed return undefined. But now it can't. One more reason not to document this for platforms that we can't test on in CI. Behavior can change and we wouldn't know it and then the docs would be wrong!

[vsemozhetbyt]

FWIW, this is what is returned on my phone with Node.js 11.1.0 from Termux team:

2 screenshots: