Account for existing but empty cpus file
#2151
Conversation
angelapwen
force-pushed
the
fix-cpu-group-bug
branch
from
dc7e2b5
to
8275137
Compare
angelapwen
changed the title
cpus file with whitespace charscpus file
| @@ -453,9 +453,9 @@ function getCgroupCpuCountFromCpus( | |||
|
|
|||
| let cpuCount = 0; | |||
| // Comma-separated numbers and ranges, for eg. 0-1,3 | |||
| const cpusString = fs.readFileSync(cpusFile, "utf-8"); | |||
| const cpusString = fs.readFileSync(cpusFile, "utf-8").trim(); | |||
There was a problem hiding this comment.
My understanding is that if the file is empty, we'll return 0, but I think we probably want to return undefined?
There was a problem hiding this comment.
That could work too — we're filtering out all counts that are undefined, or <= 0, so they kind of have the same meaning. But I don't mind making it return undefined if that's more clear.
There was a problem hiding this comment.
Ah, that's good that we were already filtering out <=0. I think I'd find undefined slightly more clear, but it's not critical.
There was a problem hiding this comment.
The updated logic returns undefined 😄
src/util.ts
Outdated
| for (const token of cpusString.split(",")) { | ||
| if (!token.includes("-")) { | ||
| if (token.length > 0 && !token.includes("-")) { |
There was a problem hiding this comment.
What does it mean if the token is empty here? If it's not valid, we might want to log then bail out and return undefined. If it's valid, is the else block doing the right thing?
There was a problem hiding this comment.
If the file is empty here (or just has a newline character as in the linked issue) the trim should make it an empty string. Then .split() will also just return a single string that's empty. Previously we were incrementing the count because an empty string doesn't include -. You're right that we shouldn't be entering the else block though... we should just be bailing out. I'll update this and look at adding the unit test 👍
There was a problem hiding this comment.
Okay — I added unit tests for the case where the file exists and the number of CPUs should be calculated, the case where the file doesn't exist, and the case where the file exists but only has a newline char in it!
If a CPU file exists but is empty, previously we reported this file with a CPU count of 1, which resulted in a single-threaded run.
angelapwen
force-pushed
the
fix-cpu-group-bug
branch
from
8275137
to
ef0a773
Compare
If a CPU file exists but is empty, previously we reported this file with a CPU count of 1, which resulted in a single-threaded run.
Fixes #2139.
Merge / deployment checklist