diff --git a/soulseek-rs-lib/src/client/connection.rs b/soulseek-rs-lib/src/client/connection.rs index 01bffb7..56103ec 100644 --- a/soulseek-rs-lib/src/client/connection.rs +++ b/soulseek-rs-lib/src/client/connection.rs @@ -5,6 +5,9 @@ use super::{ TcpStream, error, info, mpsc, thread, trace, warn, }; use std::sync::atomic::Ordering; +use std::time::{Duration, Instant}; + +const LISTENER_RELEASE_TIMEOUT: Duration = Duration::from_secs(2); /// Ceiling on the wait for a login verdict. Generous enough for a slow server /// and a retrying connect, short enough that an unattended caller ends. @@ -121,6 +124,8 @@ impl Client { let context = self.context.clone(); let own_username = self.username.clone(); let stopped = self.listener_stopped.clone(); + let released = Arc::new(AtomicBool::new(false)); + self.listener_released = released.clone(); thread::spawn(move || { Listen::serve( @@ -130,6 +135,8 @@ impl Client { own_username, stopped, ); + drop(listener); + released.store(true, Ordering::Relaxed); }); } @@ -151,7 +158,8 @@ impl Client { /// collide with our own ghost. /// /// Called automatically when the client is dropped. The peer listener's - /// bound port, if one was opened, is released with the process. + /// bound port, if one was opened, is free again when this returns, unless + /// the listener cannot be woken within two seconds. pub fn disconnect(&mut self) { if let Some(handle) = self.server_handle.take() { let _ = handle.stop(); @@ -173,8 +181,15 @@ impl Client { // this the port stays bound for the life of the process, and the next // client falls back to an ephemeral port nobody was told about. self.listener_stopped.store(true, Ordering::Relaxed); - if let Some(port) = self.bound_port.take() { - let _ = TcpStream::connect(("127.0.0.1", port)); + if let Some(port) = self.bound_port.take() + && TcpStream::connect(("127.0.0.1", port)).is_ok() + { + let deadline = Instant::now() + LISTENER_RELEASE_TIMEOUT; + while !self.listener_released.load(Ordering::Relaxed) + && Instant::now() < deadline + { + thread::sleep(Duration::from_millis(5)); + } } } diff --git a/soulseek-rs-lib/src/client/mod.rs b/soulseek-rs-lib/src/client/mod.rs index 7a146b1..eefc812 100644 --- a/soulseek-rs-lib/src/client/mod.rs +++ b/soulseek-rs-lib/src/client/mod.rs @@ -648,6 +648,7 @@ pub struct Client { /// Tells the peer listener to stop, so a disconnected client releases the /// port it bound. listener_stopped: Arc, + listener_released: Arc, } impl Drop for Client { @@ -682,6 +683,7 @@ impl Client { server_handle: None, session: SessionWatch::default(), listener_stopped: Arc::new(AtomicBool::new(false)), + listener_released: Arc::new(AtomicBool::new(false)), } } diff --git a/soulseek-rs-lib/tests/e2e.rs b/soulseek-rs-lib/tests/e2e.rs index 8e6cf70..01c905b 100644 --- a/soulseek-rs-lib/tests/e2e.rs +++ b/soulseek-rs-lib/tests/e2e.rs @@ -5594,50 +5594,30 @@ fn privileges_can_be_handed_to_another_user() { } #[test] -fn a_disconnected_client_gives_its_listener_port_back() { - // A client that goes out of scope must release the port it bound: the - // next one has to be able to advertise the port it was configured with, - // not fall back to an ephemeral one nobody was told about. No server is - // needed — binding the listener happens in connect(), before the dial. - // It binds listeners, so it waits for the server tests: a port handed - // to a soulfind that is still starting is otherwise up for grabs. +fn a_dropped_client_has_released_its_listener_port_when_drop_returns() { let _gate = SERVER_GATE .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); let port = free_port().expect("a port to hold"); - let settings = |port: u16| ClientSettings { - username: "e2e_port_release".to_string(), - password: "pw".to_string(), - server_address: PeerAddress::new("127.0.0.1".to_string(), 1), - enable_listen: true, - listen_port: port, - shared_directories: Vec::new(), - accept_children: false, - version: ClientVersion::default(), - }; - let mut first = Client::with_settings(settings(port)); - first.connect().expect("bind the listener"); - assert_eq!(first.listen_port(), Some(port)); - drop(first); + for round in 0..25 { + let mut client = Client::with_settings(ClientSettings { + username: "e2e_port_release_now".to_string(), + password: "pw".to_string(), + server_address: PeerAddress::new("127.0.0.1".to_string(), 1), + enable_listen: true, + listen_port: port, + shared_directories: Vec::new(), + accept_children: false, + version: ClientVersion::default(), + }); + client.connect().expect("bind the listener"); + assert_eq!(client.listen_port(), Some(port), "round {round}"); + drop(client); - // The listener thread notices the stop the moment it is woken, which the - // disconnect does by dialling it; give it a beat to unwind. - let deadline = Instant::now() + Duration::from_secs(5); - let mut second_port = None; - while Instant::now() < deadline && second_port != Some(port) { - let mut second = Client::with_settings(settings(port)); - second.connect().expect("bind again"); - second_port = second.listen_port(); - if second_port == Some(port) { - break; - } - drop(second); - std::thread::sleep(Duration::from_millis(200)); + assert!( + std::net::TcpListener::bind(("0.0.0.0", port)).is_ok(), + "round {round}: the port is still held once drop has returned" + ); } - assert_eq!( - second_port, - Some(port), - "the port should be free again once the first client is dropped" - ); }