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
assert workers greater than zero #167
assert workers greater than zero #167
Conversation
Codecov Report
@@ Coverage Diff @@
## master #167 +/- ##
=======================================
Coverage 60.44% 60.45%
=======================================
Files 73 73
Lines 4783 4784 +1
=======================================
+ Hits 2891 2892 +1
Misses 1892 1892
Continue to review full report at Codecov.
|
I feel like a better change here would be to panic if it's not above 0 rather than allowing that to be set, rather than this behaviour. It's a little too "magical" for my liking! |
I too feel an assert would be a better way to handle this. |
From the original issue:
Having dabbled in devops, I do think there's value in being able to use an env var set to 0 and allow that to mean "take the CPU count". What do folks think about this as a proposal: #[non_exhaustive]
pub enum WorkerCount {
Fixed(usize),
NumCpus,
}
impl WorkerCount {
fn count(&self) -> usize {
match self {
WorkerCount::Fixed(num) => *num,
WorkerCount::NumCpus => num_cpus::get(),
}
}
}
// maintain abilitiy to just type number of workers
impl From<usize> for WorkerCount {
fn from(num: usize) -> WorkerCount {
WorkerCount::Fixed(num)
}
}
impl HttpServer {
pub fn workers(self, num: impl Into<WorkerCount>) -> Self {
let workers: WorkerCount = num.into();
let workers = workers.count();
assert!(workers > 0);
// ....
}
} This is akin to the the KeepAlive options for HttpServer which can accept KeepAlive::OS in addition to a numeric fixed keep alive value, and solves the problem of being able to declare in the environment to use num_cpus more easily (no conditional function calls, only conditional variable assignments). See playground for example: Also means we can keep the 0 assertion but provide a way to "reset" after the fact. |
Yep, that will allow you to include either a number or to specify the default. It would be great though if there wasn't a way to construct the |
|
Want to get a version cut pretty soon. What do you think of this discussion @adrian-wechner ? |
Hi there, personally I still think the best thing to do, is to reset to default as it feels to me the most natural thing to do in that moment. But I'd be equally fine with any other solution as any other soluton would require to check by myself to default. Maybe a simple warning to the log is the better solution? This way everybody can still do whatever, nothing breaks, and if debug is enabled at least is getting some retro. What do you think of that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is perfectly reasonable first step.
PR Type
Feature
PR Checklist
Check your PR fulfills the following:
Overview
Before setting workers(0) will actually spin 0 threads. With this change, when setting 0 it defaults back to available logical cpus using num_cpus::get(). Use case, is having a configuration system which might request the default value and can do this by passing 0 as the worker parameter.
Closes #166