Skip to content

Commit d84930f

Browse files
committed
Fix server hanging on client reconnect, remove shutdown command
Fixes #1 The server was hanging when a second client connected after the first disconnected. The root cause was that handle_client() used the same &UnixStream reference for both BufReader and writing, which caused issues with buffering and stream state. Fix: - handle_client() now takes ownership of UnixStream - Uses try_clone() to get separate handles for reading and writing - Added regression test for reconnection scenario Also removed the 'shutdown' method from the protocol - it's a security issue to allow clients to shut down the server remotely. Updated docs and help text accordingly.
1 parent b636ae1 commit d84930f

File tree

3 files changed

+57
-53
lines changed

3 files changed

+57
-53
lines changed

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ echo '{"method":"parse","script":"echo hello"}' | nc -U /tmp/bash-ast.sock
159159
echo '{"method":"to_bash","ast":{"type":"simple","words":[{"word":"echo"}],"redirects":[]}}' | nc -U /tmp/bash-ast.sock
160160
# → {"result":"echo"}
161161

162-
# Other methods: schema, ping, shutdown
162+
# Other methods: schema, ping
163163
```
164164

165165
### Library

src/main.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@ SERVER MODE:
7070
{"method":"to_bash","ast":{...}} Convert AST to bash
7171
{"method":"schema"} Get JSON Schema
7272
{"method":"ping"} Health check
73-
{"method":"shutdown"} Stop the server
7473
7574
Example client (bash):
7675
echo '{"method":"parse","script":"echo hi"}' | nc -U /tmp/bash-ast.sock

src/server.rs

Lines changed: 56 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,6 @@ pub enum Request {
7272
Schema,
7373
/// Health check / ping
7474
Ping,
75-
/// Request server shutdown (useful for testing)
76-
Shutdown,
7775
}
7876

7977
impl Request {
@@ -101,12 +99,6 @@ impl Request {
10199
matches!(self, Self::Ping)
102100
}
103101

104-
/// Check if this is a Shutdown request
105-
#[must_use]
106-
pub const fn is_shutdown(&self) -> bool {
107-
matches!(self, Self::Shutdown)
108-
}
109-
110102
/// Get the script from a Parse request
111103
#[must_use]
112104
pub fn script(&self) -> Option<&str> {
@@ -179,7 +171,6 @@ pub fn handle_request(request: &Request) -> Response {
179171
}
180172
}
181173
Request::Ping => Response::success("pong"),
182-
Request::Shutdown => Response::success("shutting down"),
183174
}
184175
}
185176

@@ -287,7 +278,7 @@ impl Server {
287278
Ok((stream, _addr)) => {
288279
// Set blocking for the client stream
289280
stream.set_nonblocking(false)?;
290-
self.handle_client(&stream);
281+
self.handle_client(stream);
291282
}
292283
Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
293284
// No connection available, sleep briefly and check shutdown
@@ -307,22 +298,25 @@ impl Server {
307298
}
308299

309300
/// Handle a single client connection
310-
fn handle_client(&self, stream: &UnixStream) {
311-
let reader = BufReader::new(stream);
301+
#[allow(clippy::unused_self)] // Method logically belongs to Server
302+
fn handle_client(&self, stream: UnixStream) {
303+
// Clone the stream to get separate handles for reading and writing.
304+
// This avoids issues with BufReader's internal buffering when sharing
305+
// a single reference for both read and write operations.
306+
let read_stream = match stream.try_clone() {
307+
Ok(s) => s,
308+
Err(e) => {
309+
eprintln!("Failed to clone stream: {e}");
310+
return;
311+
}
312+
};
313+
let reader = BufReader::new(read_stream);
312314
let mut writer = stream;
313315

314316
for line in reader.lines() {
315317
match line {
316318
Ok(line) if line.is_empty() => {}
317319
Ok(line) => {
318-
// Check for shutdown request
319-
if matches!(parse_request(&line), Ok(Request::Shutdown)) {
320-
let response = handle_line(&line);
321-
let _ = writeln!(writer, "{response}");
322-
self.shutdown.store(true, Ordering::Relaxed);
323-
break;
324-
}
325-
326320
let response = handle_line(&line);
327321
if writeln!(writer, "{response}").is_err() {
328322
break;
@@ -400,13 +394,6 @@ mod tests {
400394
assert!(req.is_ping());
401395
}
402396

403-
#[test]
404-
fn test_parse_request_shutdown() {
405-
let json = r#"{"method":"shutdown"}"#;
406-
let req = parse_request(json).unwrap();
407-
assert!(req.is_shutdown());
408-
}
409-
410397
#[test]
411398
fn test_parse_request_invalid_json() {
412399
let result = parse_request("not json");
@@ -673,16 +660,6 @@ mod tests {
673660
}
674661
}
675662

676-
#[test]
677-
fn test_handle_request_shutdown() {
678-
let req = Request::Shutdown;
679-
let resp = handle_request(&req);
680-
assert!(resp.is_success());
681-
if let Response::Success { result } = resp {
682-
assert_eq!(result, "shutting down");
683-
}
684-
}
685-
686663
// ==================== Handle Line Tests ====================
687664

688665
#[test]
@@ -853,35 +830,63 @@ mod tests {
853830
}
854831

855832
#[test]
856-
fn test_server_integration_shutdown_method() {
833+
fn test_server_integration_reconnect() {
834+
// Regression test for issue #1: server hangs on second connection
857835
setup();
858836
let socket_path = test_socket_path();
859837
let socket_path_clone = socket_path.clone();
860838

861839
let server = Server::with_path(&socket_path);
840+
let shutdown = server.shutdown_handle();
862841

863842
let server_thread = thread::spawn(move || {
864843
let _ = server.run();
865844
});
866845

867846
thread::sleep(Duration::from_millis(100));
868847

869-
let mut stream = UnixStream::connect(&socket_path_clone).unwrap();
870-
stream
871-
.set_read_timeout(Some(Duration::from_secs(5)))
872-
.unwrap();
848+
// First connection
849+
{
850+
let mut stream = UnixStream::connect(&socket_path_clone).unwrap();
851+
stream
852+
.set_read_timeout(Some(Duration::from_secs(2)))
853+
.unwrap();
854+
855+
let resp = send_request(&mut stream, r#"{"method":"ping"}"#);
856+
assert!(resp.is_success());
857+
// stream is dropped here, simulating client disconnect
858+
}
873859

874-
// Send shutdown request - server should exit gracefully
875-
let resp = send_request(&mut stream, r#"{"method":"shutdown"}"#);
876-
assert!(resp.is_success());
860+
// Brief pause to let server process the disconnect
861+
thread::sleep(Duration::from_millis(50));
877862

878-
// Server thread should exit
879-
let result = server_thread.join();
880-
assert!(result.is_ok());
863+
// Second connection - this was hanging before the fix
864+
{
865+
let mut stream = UnixStream::connect(&socket_path_clone).unwrap();
866+
stream
867+
.set_read_timeout(Some(Duration::from_secs(2)))
868+
.unwrap();
869+
870+
let resp = send_request(&mut stream, r#"{"method":"ping"}"#);
871+
assert!(
872+
resp.is_success(),
873+
"Second connection should work after first disconnects"
874+
);
875+
}
881876

882-
// Socket should be cleaned up
883-
thread::sleep(Duration::from_millis(50));
884-
assert!(!Path::new(&socket_path_clone).exists());
877+
// Third connection for good measure
878+
{
879+
let mut stream = UnixStream::connect(&socket_path_clone).unwrap();
880+
stream
881+
.set_read_timeout(Some(Duration::from_secs(2)))
882+
.unwrap();
883+
884+
let resp = send_request(&mut stream, r#"{"method":"parse","script":"echo hello"}"#);
885+
assert!(resp.is_success(), "Third connection should also work");
886+
}
887+
888+
shutdown.store(true, Ordering::Relaxed);
889+
let _ = server_thread.join();
885890
}
886891

887892
#[test]

0 commit comments

Comments
 (0)