fix(lib): give the listener port back before a dropped client returns

disconnect() told the listener thread to stop and dialled it awake, then
returned while that thread was still unwinding, so the TcpListener was
often still open when the caller moved on. A client built straight after
a drop then found its configured port taken and fell back to an ephemeral
one. The TUI login screen does exactly that when it replaces an attempt.

The listener thread now raises a flag once it has dropped its socket, and
after the wake-up dial has landed, disconnect() waits for that flag,
bounded at two seconds, so the port can be bound again by the time
disconnect() or Drop returns. A listener that cannot be woken, or is held
past the bound by a full handshake queue, finishes on its own as before.
The flag is an Arc<AtomicBool>, like the stop flag beside it: keeping the
thread's JoinHandle instead would leave Client neither UnwindSafe nor
RefUnwindSafe, which is a breaking change for the library.

Nicotine+ and Soulseek.NET both close the listen socket as part of
disconnecting; this makes that hold by the time the call returns.

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));
+            }
         }
     }
 

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<AtomicBool>,
+    listener_released: Arc<AtomicBool>,
 }
 
 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)),
         }
     }
 

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"
-    );
 }