Conversation
Summary by CodeRabbit
WalkthroughRefactor the TCP server's worker logic by dividing the monolithic connection acceptance and handling loop into three modular functions: one for accepting connections ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TcpListener
participant Server (accept_loop)
participant Task (spawn_connection_task)
participant App
Client->>TcpListener: Connect
loop accept_loop
TcpListener->>Server (accept_loop): Accept connection
Server (accept_loop)->>Task (spawn_connection_task): Spawn per-connection task
Task (spawn_connection_task)->>App: Process connection (with panic handling)
alt Panic occurs
Task (spawn_connection_task)->>Task (spawn_connection_task): Log panic with peer address
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
⏰ Context from checks skipped due to timeout of 240000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
Reviewer's GuideThis PR refactors the worker task by extracting connection spawning and panic handling into a dedicated helper, moving the accept loop logic into its own async function, and slimming down the main worker_task to a simple delegator. It also adds targeted tests for the new helper and updated shutdown behavior. Sequence diagram for connection acceptance and handling after refactorsequenceDiagram
participant Worker as worker_task
participant Acceptor as accept_loop
participant Listener as TcpListener
participant Spawner as spawn_connection_task
participant Handler as process_stream
Worker->>Acceptor: accept_loop(...)
loop Until shutdown
Acceptor->>Listener: accept()
alt Connection accepted
Acceptor->>Spawner: spawn_connection_task(stream, ...)
Spawner->>Handler: process_stream(stream, ...)
Handler-->>Spawner: (may panic)
Spawner-->>Acceptor: (logs panic if any)
else Accept error
Acceptor->>Acceptor: retry with backoff
end
end
Class diagram for refactored worker task and helpersclassDiagram
class worker_task {
+async fn worker_task(listener, factory, on_success, on_failure, shutdown, tracker)
}
class accept_loop {
+async fn accept_loop(listener, factory, on_success, on_failure, shutdown, tracker)
}
class spawn_connection_task {
+fn spawn_connection_task(stream, factory, on_success, on_failure, tracker)
}
class process_stream {
+async fn process_stream(stream, factory, on_success, on_failure)
}
worker_task --> accept_loop : delegates to
accept_loop --> spawn_connection_task : spawns per connection
spawn_connection_task --> process_stream : calls
Class diagram for test additions for spawn_connection_taskclassDiagram
class tests {
+async fn spawn_connection_task_logs_panic(factory)
+async fn test_accept_loop_shutdown_signal(factory)
}
tests ..> spawn_connection_task : tests
tests ..> accept_loop : tests
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![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/server.rs:387` </location>
<code_context>
+ F: Fn() -> WireframeApp + Send + Sync + Clone + 'static,
+ T: Preamble,
+{
+ let peer_addr = stream.peer_addr().ok();
+ tracker.spawn(async move {
+ use futures::FutureExt as _;
</code_context>
<issue_to_address>
Handle peer_addr extraction errors more explicitly.
Logging a warning or error when peer_addr is None would improve debuggability.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let peer_addr = stream.peer_addr().ok();
tracker.spawn(async move {
use futures::FutureExt as _;
=======
let peer_addr = match stream.peer_addr() {
Ok(addr) => Some(addr),
Err(e) => {
tracing::warn!("Failed to extract peer_addr from TcpStream: {}", e);
None
}
};
tracker.spawn(async move {
use futures::FutureExt as _;
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/server.rs (2)
443-443: Replaceeprintln!withtracing::error!for consistency.Use the tracing framework for error logging to maintain consistency with the rest of the codebase.
- eprintln!("accept error: {e}"); + tracing::error!(error = %e, "Failed to accept connection");
512-512: Replaceeprintln!withtracing::error!for consistency.Use the tracing framework for error logging to maintain consistency.
- eprintln!("preamble callback error: {e}"); + tracing::error!(error = %e, "Preamble callback failed");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/server.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/server.rs
🔇 Additional comments (2)
src/server.rs (2)
375-409: LGTM! Well-structured panic isolation.The function properly isolates connection panics and provides comprehensive logging with peer address context. The panic message extraction handles both
&strandStringpanic payloads correctly.
897-918: LGTM! Comprehensive test coverage for the refactored functions.The test rename accurately reflects the refactored function, and the new panic logging test thoroughly verifies that connection panics are caught and logged with peer address context without crashing the worker.
Also applies to: 962-1009
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
spawn_connection_taskaccept_loopworker_taskfocused on coordinationTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688bf5ca37788322a5e98a3a4d0cab5c
Summary by Sourcery
Refactor worker_task by splitting off connection spawning and panic handling into spawn_connection_task, moving the accept loop into accept_loop, and updating tests to cover shutdown behavior and panic logging
Bug Fixes:
Enhancements:
Tests: