assert workers greater than zero

[adrian-wechner]

PR Type

Feature

PR Checklist

Check your PR fulfills the following:

• Tests for the changes have been added / updated.
• Documentation comments have been added / updated.
• A changelog entry has been made for the appropriate packages.
• Format code with the latest stable rustfmt

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

[codecov]

Codecov Report

Merging #167 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #167   +/-   ##
=======================================
  Coverage   60.44%   60.45%           
=======================================
  Files          73       73           
  Lines        4783     4784    +1     
=======================================
+ Hits         2891     2892    +1     
  Misses       1892     1892           

Impacted Files	Coverage Δ
actix-server/src/builder.rs	52.42% <100.00%> (+0.21%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 578a560...6067ed4. Read the comment docs.

[cetra3]

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!

[fakeshadow]

I too feel an assert would be a better way to handle this.

[robjtede]

From the original issue:

I use it personally in our configuration system, that by setting 0 for the workers config, that we translate it into the system's cpu count.

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:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bcebe9c88927917a30891053c5f5fff1

Also means we can keep the 0 assertion but provide a way to "reset" after the fact.

[cetra3]

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 WorkerCount with 0 as the number. I.e, enforce at compile time. I'm not sure how that could be done, maybe an assert on the From impl? But then they could always just manually create the WorkerCount::Fixed

[robjtede]

NonZeroUsize exists but would still require runtime checks. I think the assert in server initialization is enough.

[robjtede]

Want to get a version cut pretty soon. What do you think of this discussion @adrian-wechner ?

[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?

[robjtede]

I think this is perfectly reasonable first step.