Improve server readiness signaling #220
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary by CodeRabbit
WalkthroughExtend Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Server as WireframeServer
participant Client as TCP Client
Test->>Server: Create with oneshot::Sender for readiness
Test->>Server: Drop oneshot::Receiver (simulate send failure)
Server->>Test: Attempt to send readiness signal (fails, logs warning)
Test->>Server: Connect TCP client
Client->>Server: Establish connection
Server->>Client: Accept connection (despite readiness signal failure)
sequenceDiagram
participant PanicWorld as PanicWorld
participant PanicServer as PanicServer
participant Server as WireframeServer
participant Client as TCP Client
PanicWorld->>PanicServer: spawn()
PanicServer->>Server: Start with panic-inducing callback
PanicServer->>Server: Await readiness
Client->>Server: Attempt connection (triggers panic)
Server->>PanicServer: Panic handled, error logged
PanicWorld->>PanicServer: Drop (shutdown)
PanicServer->>Server: Send shutdown, await task completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🧬 Code Graph Analysis (3)tests/server.rs (3)
src/server.rs (1)
tests/world.rs (1)
🔇 Additional comments (6)
✨ 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 enhances server readiness signaling by clarifying ready_tx thread-safety and single-use behavior, warning on failed signals, introducing a generic panic-catching helper for worker tasks, and refactoring the PanicWorld test scaffold into a reusable PanicServer abstraction. Sequence diagram for server readiness signaling and warningsequenceDiagram
participant Server
participant ReadyReceiver as oneshot::Receiver
participant Logger as tracing
Server->>ReadyReceiver: tx.send(())
alt send fails (receiver dropped)
Server->>Logger: warn("Failed to send readiness signal: receiver dropped")
end
Sequence diagram for worker task panic handling with catch_and_log_unwindsequenceDiagram
participant WorkerTask
participant catch_and_log_unwind
participant Logger as tracing
WorkerTask->>catch_and_log_unwind: spawn(process_stream, peer_addr)
alt panic occurs
catch_and_log_unwind->>Logger: error("connection task panicked", panic_msg, peer_addr)
end
File-Level Changes
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 found some issues that need to be addressed.
- Using futures::executor::block_on inside PanicServer’s Drop can deadlock if dropped within an async context—consider refactoring to await the handle in async or use JoinHandle::abort/await instead of blocking in Drop.
- The
if let Some(tx) = self.ready_tx && tx.send(()).is_err()pattern requires Rust 1.65+ for let‐chains; please ensure your CI toolchain supports this or revert to a nested if‐let for broader compatibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using futures::executor::block_on inside PanicServer’s Drop can deadlock if dropped within an async context—consider refactoring to await the handle in async or use JoinHandle::abort/await instead of blocking in Drop.
- The `if let Some(tx) = self.ready_tx && tx.send(()).is_err()` pattern requires Rust 1.65+ for let‐chains; please ensure your CI toolchain supports this or revert to a nested if‐let for broader compatibility.
## Individual Comments
### Comment 1
<location> `tests/world.rs:55` </location>
<code_context>
}
+}
+
+impl Drop for PanicServer {
+ fn drop(&mut self) {
+ if let Some(tx) = self.shutdown.take() {
+ let _ = tx.send(());
+ }
+ let _ = futures::executor::block_on(&mut self.handle);
+ }
+}
</code_context>
<issue_to_address>
Potential for panics or hangs in Drop implementation of PanicServer.
Using block_on in Drop may cause tests to hang or panic if the server task does not complete. Adding a timeout or handling panics can help prevent indefinite hangs and make failures clearer.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
impl Drop for PanicServer {
fn drop(&mut self) {
if let Some(tx) = self.shutdown.take() {
let _ = tx.send(());
}
let _ = futures::executor::block_on(&mut self.handle);
}
}
=======
impl Drop for PanicServer {
fn drop(&mut self) {
use std::time::Duration;
use futures::FutureExt;
if let Some(tx) = self.shutdown.take() {
let _ = tx.send(());
}
// Set a timeout for the server task to finish
let timeout = Duration::from_secs(5);
let handle_fut = (&mut self.handle).timeout(timeout);
match futures::executor::block_on(handle_fut) {
Ok(Ok(_)) => {} // Server task finished successfully
Ok(Err(e)) => {
eprintln!("PanicServer task panicked: {:?}", e);
}
Err(_) => {
eprintln!("PanicServer task did not shut down within timeout");
}
}
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/world.rs:75` </location>
<code_context>
+ ///
+ /// # Panics
+ /// Panics if binding the server fails or the server task fails.
+ pub async fn start_panic_server(&mut self) { self.server.replace(PanicServer::spawn().await); }
/// Connect to the running server once.
</code_context>
<issue_to_address>
No test coverage for server readiness signaling failure.
Please add a test that drops the receiver before the server signals readiness to ensure the warning path is exercised and the server behaves as expected.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
pub struct PanicWorld {
server: Option<PanicServer>,
attempts: usize,
}
=======
pub struct PanicWorld {
server: Option<PanicServer>,
attempts: usize,
}
#[cfg(test)]
mod tests {
use super::*;
use std::sync::Arc;
use tokio::sync::Mutex;
use tokio::time::{timeout, Duration};
/// Test that dropping the readiness receiver before the server signals readiness
/// triggers the warning path and the server behaves as expected.
#[tokio::test]
async fn test_server_readiness_receiver_dropped() {
// Setup a custom PanicServer that exposes the readiness sender
struct TestPanicServer {
addr: std::net::SocketAddr,
// Optionally store the join handle if needed
}
impl TestPanicServer {
async fn spawn_with_readiness_sender() -> (Self, tokio::sync::oneshot::Sender<()>) {
use tokio::net::TcpListener;
use tokio::sync::oneshot;
let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind failed");
let addr = listener.local_addr().unwrap();
let (tx, rx) = oneshot::channel::<()>();
// Drop the receiver immediately to simulate the failure
drop(rx);
// Spawn the server task, which will try to send on tx
tokio::spawn(async move {
// Simulate server startup delay
tokio::time::sleep(Duration::from_millis(100)).await;
// This send will fail, triggering the warning path
let _ = tx.send(());
// Keep the server alive for a short time
let _ = listener.accept().await;
});
(Self { addr }, tx)
}
}
// Spawn the test server with readiness sender
let (server, _tx) = TestPanicServer::spawn_with_readiness_sender().await;
// Try to connect to the server after a short delay to ensure it's running
tokio::time::sleep(Duration::from_millis(200)).await;
let stream_result = TcpStream::connect(server.addr).await;
// The server should still accept connections even though readiness signaling failed
assert!(stream_result.is_ok(), "Server did not accept connection after readiness receiver dropped");
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/server.rs:342` </location>
<code_context>
let listener = self.listener.expect("`bind` must be called before `run`");
- if let Some(tx) = self.ready_tx {
- let _ = tx.send(());
+ if let Some(tx) = self.ready_tx
+ && tx.send(()).is_err()
+ {
</code_context>
<issue_to_address>
Consider simplifying the readiness signal handling and inlining the panic handler to reduce boilerplate and improve readability.
```suggestion
RunWithShutdown::run_with_shutdown – simplify readiness signal and panic‐catcher
1. Replace the chained `if let … &&` with a straightforward two‐step match on `Option`
and avoid moving `self.ready_tx` by calling `take()`.
```rust
// before
if let Some(tx) = self.ready_tx
&& tx.send(()).is_err()
{
tracing::warn!("Failed to send readiness signal: receiver dropped");
}
// after
if let Some(tx) = self.ready_tx.take() {
if tx.send(()).is_err() {
tracing::warn!("Failed to send readiness signal: receiver dropped");
}
}
```
2. Inline the panic‐handler into `worker_task` (only used once) and drop the free fn.
This collapses boilerplate and keeps error‐context nearby:
```rust
// Remove the `catch_and_log_unwind` helper entirely, and in worker_task:
t.spawn(async move {
use futures::FutureExt;
let fut = std::panic::AssertUnwindSafe(
process_stream(stream, factory, success, failure),
)
.catch_unwind();
if let Err(panic) = fut.await {
let panic_msg = panic
.downcast_ref::<&str>().map(|s| *s)
.or_else(|| panic.downcast_ref::<String>().map(String::as_str))
.unwrap_or("<non‐string panic>");
tracing::error!(panic = %panic_msg, ?peer_addr, "connection task panicked");
}
});
```
Both changes keep the same functionality but reduce nested logic, free‐function boilerplate, and improve local readability.
</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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ready_txcatch_and_log_unwindhelper and use it inworker_taskPanicWorldusing aPanicServerhelperTesting
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_688567eb84e0832281113f65dfff5132
Summary by Sourcery
Improve server readiness signaling by clarifying semantics, warning on send failures, and centralizing panic handling; refactor tests accordingly
Enhancements:
Documentation:
Tests: