Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions examples/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ path = "named-pipe-ready.rs"
name = "named-pipe-multi-client"
path = "named-pipe-multi-client.rs"

[[example]]
name = "graceful-shutdown"
path = "graceful-shutdown.rs"

[[example]]
name = "dump"
path = "dump.rs"
Expand Down
88 changes: 88 additions & 0 deletions examples/graceful-shutdown.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
//! Graceful shutdown example using `tokio::signal`.
//!
//! This starts a TCP echo server that shuts down cleanly when it receives
//! Ctrl+C (SIGINT). In-flight connections are allowed to finish before
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs say “In-flight connections are allowed to finish”, but on shutdown each connection breaks as soon as shutdown_rx.recv() completes, which interrupts active sessions rather than letting them complete naturally.

Severity: low

Other Locations
  • examples/graceful-shutdown.rs:60

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The Augment AI reviewer is correct! The example does not allow the clients to gracefully finish. It just waits for 100ms before dropping the Tokio runtime. The example should either collect all JoinResults and await them or use shutdown_tx.closed() to be notified when the subscribers are all doppped.

//! the process exits.
//!
//! Start the server:
//!
//! cargo run --example graceful-shutdown
//!
//! Then connect with:
//!
//! nc 127.0.0.1 6142
//!
//! Press Ctrl+C on the server to trigger a graceful shutdown.

#![warn(rust_2018_idioms)]

use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
use tokio::net::TcpListener;
use tokio::sync::broadcast;

use std::error::Error;

#[tokio::main]
async fn main() -> Result<(), Box<dyn Error>> {
let listener = TcpListener::bind("127.0.0.1:6142").await?;
println!("listening on 127.0.0.1:6142");

// A broadcast channel to notify all tasks when shutdown is requested.
let (shutdown_tx, _) = broadcast::channel::<()>(1);

loop {
tokio::select! {
// Wait for a new connection.
result = listener.accept() => {
let (socket, addr) = result?;
println!("accepted connection from {addr}");

let mut shutdown_rx = shutdown_tx.subscribe();

tokio::spawn(async move {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JoinHandle from tokio::spawn is dropped, so main can’t reliably know when all connection tasks have finished before returning; remaining tasks may be cancelled when the Tokio runtime shuts down.

Severity: medium

Other Locations
  • examples/graceful-shutdown.rs:84

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The Augment AI reviewer is correct! The example does not allow the clients to gracefully finish. It just waits for 100ms before dropping the Tokio runtime. The example should either collect all JoinResults and await them or use shutdown_tx.closed() to be notified when the subscribers are all doppped.

let (reader, mut writer) = socket.into_split();
let mut reader = BufReader::new(reader);
let mut line = String::new();

loop {
tokio::select! {
// Echo lines back to the client.
result = reader.read_line(&mut line) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The use of reader.read_line(&mut line) without a length limit is vulnerable to Denial of Service (DoS) attacks. A malicious client can send a very large amount of data without a newline character, causing the server to allocate an unbounded amount of memory for the line string, eventually leading to an Out-of-Memory (OOM) crash. In a production-ready server, you should use a method that enforces a maximum line length, such as using tokio_util::codec::LinesCodec or manually limiting the number of bytes read.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:bug; feedback: The Gemini Ai reviewer is correct! A bad client could cause denial of service attack by sending a huge line. But since this is an example of a graceful shutdown it is OK to leave it as is.

match result {
Ok(0) | Err(_) => break,
Ok(_) => {
let _ = writer.write_all(line.as_bytes()).await;
line.clear();
}
Comment on lines +53 to +56
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The result of writer.write_all() is currently ignored. If an error occurs during the write operation (e.g., the client has disconnected), the loop will continue, potentially leading to more errors on subsequent operations. It's better to handle this error and break the loop to close the connection cleanly.

                                    Ok(_) => {
                                        if writer.write_all(line.as_bytes()).await.is_err() {
                                            // Client disconnected.
                                            break;
                                        }
                                        line.clear();
                                    }

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-to-have; category:bug; feedback: The Gemini Ai reviewer is correct! A failed write indicates that the client is gone and the server should stop trying to write to it.

}
}
// Stop this connection when shutdown is signaled.
_ = shutdown_rx.recv() => {
let _ = writer.write_all(b"server shutting down\n").await;
break;
}
}
}

println!("connection from {addr} closed");
});
}

// Wait for Ctrl+C.
_ = tokio::signal::ctrl_c() => {
println!("\nshutdown signal received, closing listener");
break;
}
}
}

// Notify all active connections.
let _ = shutdown_tx.send(());

// Drop the sender and wait briefly for connections to finish.
drop(shutdown_tx);
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
Comment on lines +82 to +84
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using a fixed-duration sleep for graceful shutdown is not robust. Connections might take longer than the specified duration to close, leading to abrupt termination. A more reliable approach is to wait for all connection handlers to actually finish. You can achieve this by waiting for all broadcast::Receiver handles to be dropped, which indicates that the connection handlers have terminated. The Sender::closed() method provides a future that completes when all receivers are gone.

    // Wait for all active connections to finish processing.
    // The `closed()` future will complete when all receivers have been dropped.
    shutdown_tx.closed().await;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The example does not allow the clients to gracefully finish. It just waits for 100ms before dropping the Tokio runtime. The example should either collect all JoinResults and await them or use shutdown_tx.closed() to be notified when the subscribers are all doppped.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spawned tasks not awaited, shutdown not actually graceful

Medium Severity

The documentation promises "in-flight connections are allowed to finish before the process exits," but the JoinHandles from tokio::spawn are discarded, and the code relies on an arbitrary 100ms sleep instead of actually awaiting task completion. When main returns, the Tokio runtime is dropped and any still-running tasks are cancelled. This means the example — whose entire purpose is to teach graceful shutdown — does not actually implement graceful shutdown. A JoinSet or collected JoinHandles awaited after signaling would be the correct approach.

Additional Locations (2)

Fix in Cursor Fix in Web

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! The example does not allow the clients to gracefully finish. It just waits for 100ms before dropping the Tokio runtime. The example should either collect all JoinResults and await them or use shutdown_tx.closed() to be notified when the subscribers are all doppped.


println!("shutdown complete");
Ok(())
}