Fix worker count default #16
Conversation
Reviewer's GuideThis PR refactors the server’s worker-count logic to use the standard library’s CPU-core detection with a fallback and minimum enforcement, exposes a new getter, removes the num_cpus crate, updates documentation to reflect the CPU-based default and cleans up module docs, and adds unit tests covering server worker counts and payload behavior. Sequence diagram for WireframeServer::new() worker initializationsequenceDiagram
participant Caller
participant WS as WireframeServer
participant std_thread as "std::thread"
Caller->>+WS: new(factory)
WS->>+std_thread: available_parallelism()
std_thread-->>-WS: Result<NonZeroUsize, Error>
alt Ok(non_zero_val) from available_parallelism()
WS-->>WS: workers = non_zero_val.get()
else Error from available_parallelism() or count is zero
WS-->>WS: workers = 1
end
WS-->>Caller: WireframeServer instance
Sequence diagram for WireframeServer::workers() clamping logicsequenceDiagram
participant Caller
participant WS as WireframeServer
Caller->>+WS: workers(count)
WS-->>WS: self.workers = count.max(1)
WS-->>-Caller: WireframeServer instance
Updated class diagram for WireframeServerclassDiagram
class WireframeServer~F~ {
-factory: F
-listener: Option~TcpListener~
-workers: usize
+new(factory: F) WireframeServer~F~
+workers(count: usize) WireframeServer~F~
+worker_count() usize
+bind~A: ToSocketAddrs~(addr: A) Result~WireframeServer~F~, std..io..Error~
+run() Result~(), std..io..Error~
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis update clarifies the default worker count behaviour for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WireframeServer
User->>WireframeServer: create new instance
WireframeServer-->>User: instance with default worker count (CPU cores)
User->>WireframeServer: call worker_count()
WireframeServer-->>User: return current worker count
User->>WireframeServer: call workers(0)
WireframeServer-->>User: enforce minimum worker count of 1
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/middleware.rs (1)
24-26:newdoes not benefit from beingconstMarking
Next::newasconstprovides no practical gain because:
- The function takes a non-
'staticreference, so it cannot be evaluated in aconstcontext outside ofstatic/constinitialisers.Nextis only ever constructed at run-time by middleware; compile-time construction isn’t expected.Const-qualifying this API surface may mislead users and imposes additional const-stability constraints for future changes.
- #[must_use] - pub const fn new(service: &'a S) -> Self { + #[must_use] + pub fn new(service: &'a S) -> Self {Ensure no public docs or downstream crates rely on the
constcapability before dropping it.src/server.rs (1)
65-69: Consider#[inline]on the trivial getter
worker_countis a tiny accessor that will likely be called in hot paths (e.g., tests, metrics). Marking it#[inline(always)](or at least#[inline]) lets LLVM fold it away completely.- #[must_use] - pub const fn worker_count(&self) -> usize { + #[inline] + #[must_use] + pub const fn worker_count(&self) -> usize {tests/server.rs (4)
1-1: Public-API visibility checkThe test imports
wireframe::{app::WireframeApp, server::WireframeServer}. Double-check that theappandservermodules are re-exported at the crate root (e.g.pub mod app; pub mod server;). Otherwise the integration tests will not compile when the crate is published.
4-7: Replaceunwrap()withexpect()for clearer diagnosticsA failing
unwrap()produces a generic panic, obscuring the root cause. Adding an explanatory message tightens feedback during CI runs:- let server = WireframeServer::new(|| WireframeApp::new().unwrap()); + let server = WireframeServer::new(|| WireframeApp::new().expect("WireframeApp::new failed"));
5-7: Assert the exact default, not just positivityThe constructor deterministically sets the worker count to
num_cpus::get().max(1). Checking equality guards against silent regressions:- assert!(server.worker_count() >= 1); + assert_eq!(server.worker_count(), num_cpus::get().max(1));
10-13: Consider adding upper-bound / large-value testsThe new clamping focuses on the minimum, but a quick test with a large explicit value (e.g.
.workers(128)) would verify that the method also honours valid counts > 1, increasing branch coverage ofworkers().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)docs/rust-binary-router-library-design.md(1 hunks)src/extractor.rs(3 hunks)src/middleware.rs(1 hunks)src/server.rs(1 hunks)tests/server.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/server.rs (1)
src/server.rs (2)
new(42-48)worker_count(67-69)
🔇 Additional comments (4)
src/extractor.rs (1)
108-129: Unit tests relocated properly – looks goodMoving the
#[cfg(test)]module out of theimplblock clarifies separation between production and test code, and the tests themselves remain concise and correct.
No further action needed.src/server.rs (1)
59-63: Good defensive clamping inworkersThe explicit
count.max(1)guarantees at least one worker and prevents accidental mis-configuration.README.md (1)
43-44: Nice clarificationExplicitly stating the CPU-core default helps users reason about scalability out of the box.
docs/rust-binary-router-library-design.md (1)
672-674: Documentation is consistent with implementationThe note about defaulting worker tasks to CPU cores matches the new constructor logic and README.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/middleware.rs:31` </location>
<code_context>
/// Call the next service with the given request.
///
/// # Errors
- ///
- /// Asynchronously invokes the next service in the middleware chain with the given request.
</code_context>
<issue_to_address>
Remove or populate the empty `# Errors` section
The `# Errors` header is empty; please either remove it or provide relevant error information.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/server.rs (1)
4-8: Avoidunwrap()to keep the test (and code) portable and panic-free
std::thread::available_parallelism()may legitimately return an error on some platforms (e.g. when/proc/cpuinfois unavailable inside certain containers). A failing call here would panic and mask the real intention of the test.- let expected = std::thread::available_parallelism().unwrap().get(); + let expected = std::thread::available_parallelism() + .map_or(1, |n| n.get()); // fall back to a single worker if unavailable(This also highlights that the production constructor in
src/server.rscontains the sameunwrap; consider the same graceful fallback there.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.toml(0 hunks)src/middleware.rs(1 hunks)src/server.rs(1 hunks)tests/server.rs(1 hunks)
💤 Files with no reviewable changes (1)
- Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/middleware.rs
- src/server.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/server.rs (1)
src/server.rs (2)
new(46-52)worker_count(72-74)
🔇 Additional comments (2)
tests/server.rs (2)
11-15: Good boundary-value testThe assertion correctly verifies that calling
.workers(0)is clamped to the documented minimum of one worker. No issues spotted.
18-22: Large-value path covered – looks solidThe test nicely exercises an upper-range value and documents that the builder happily accepts it. 👍
|
Caution An unexpected error occurred while opening a pull request: Reference already exists - https://docs.github.com/rest/git/refs#create-a-reference |
1 similar comment
|
Caution An unexpected error occurred while opening a pull request: Reference already exists - https://docs.github.com/rest/git/refs#create-a-reference |
…erver` Docstrings generation was requested by @leynos. * #16 (comment) The following files were modified: * `src/extractor.rs` * `src/middleware.rs` * `src/server.rs`
|
Note Generated docstrings for this pull request at #19 |
…erver` (#19) Docstrings generation was requested by @leynos. * #16 (comment) The following files were modified: * `src/extractor.rs` * `src/middleware.rs` * `src/server.rs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…erver` (#19) Docstrings generation was requested by @leynos. * #16 (comment) The following files were modified: * `src/extractor.rs` * `src/middleware.rs` * `src/server.rs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
leynos
force-pushed
the
codex/ensure-minimum-of-one-worker-in-wireframeserver
branch
from
226b417 to
e9222a4
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The
WireframeServer::newdocs include a# Panicssection that’s misleading—usingmap_or(1, ...)will never panic, so consider removing or updating that section. - In the middleware examples you refer to
your_crate::{Next, Service…}, but the actual crate name iswireframe—update the examples so they compile as-is. - The
WireframeServer::newdoc block has a lot of repeated descriptive text and examples; consider consolidating duplicated sentences to keep it concise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `WireframeServer::new` docs include a `# Panics` section that’s misleading—using `map_or(1, ...)` will never panic, so consider removing or updating that section.
- In the middleware examples you refer to `your_crate::{Next, Service…}`, but the actual crate name is `wireframe`—update the examples so they compile as-is.
- The `WireframeServer::new` doc block has a lot of repeated descriptive text and examples; consider consolidating duplicated sentences to keep it concise.
## Individual Comments
### Comment 1
<location> `src/server.rs:48` </location>
<code_context>
+ ///
+ /// The server is initialised with a default worker count equal to the number of available CPU cores, or 1 if this cannot be determined. The TCP listener is unset and must be configured with `bind` before running the server.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the number of available CPUs cannot be determined and the fallback to 1 fails.
+ ///
+ /// # Examples
</code_context>
<issue_to_address>
Panic section in docs is misleading
Since `map_or` ensures a fallback to 1, this constructor cannot panic. Please update or remove the `# Panics` section accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai Please create the following Github issues: The WireframeServer::new docs include a # Panics section that’s misleading—using map_or(1, ...) will never panic, so consider removing or updating that section. In the middleware examples you refer to your_crate::{Next, Service…}, but the actual crate name is wireframe—update the examples so they compile as-is. The WireframeServer::new doc block has a lot of repeated descriptive text and examples; consider consolidating duplicated sentences to keep it concise. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Sure thing — I’ll open three separate GitHub issues capturing each documentation task you listed. 🏁 Script executed: Length of output: 2036 @leynos I tried to open the three GitHub issues, but the CLI needs a How would you like to proceed?
Let me know which option you prefer and I’ll take care of it right away. |
|
@coderabbitai Please try again |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4566 @leynos All set! 🎉
Let me know if anything else needs tweaking. |
1 participant
Summary
WireframeServer::newclamps worker count to at least oneTesting
cargo fmt --all -- --checkcargo clippy -- -D warningscargo testmarkdownlint README.md docs/roadmap.md docs/rust-binary-router-library-design.mdnixie README.md docs/*.mdhttps://chatgpt.com/codex/tasks/task_e_684c35be1600832285dd1dfe9a0b9e70
Summary by Sourcery
Ensure the server defaults to at least one worker using the standard parallelism API, remove the num_cpus crate, document the CPU-based default, add a worker_count getter, and include basic tests for server and payload behavior.
New Features:
worker_count()getter onWireframeServerto retrieve the configured worker count.Bug Fixes:
WireframeServer::newand theworkers()builder method.Enhancements:
num_cpusdependency withstd::thread::available_parallelism()for default worker determination.Documentation:
Tests:
Payloadunit tests foradvanceandremainingmethods.Summary by CodeRabbit