fix: review findings on the protocol work
Correctness: - A child that stopped reading could freeze the whole client: the relay writes under the context lock and the socket was blocking, so one stalled peer took every download, upload and API call with it. Child writes now have a deadline, and a child that hits it loses its slot. - Relaying searches to children was gated by the leaf's answer budget, so a parent forwarded a fraction of the stream while advertising itself as healthy. The budget bounds our own share scan, which is what it was for; the tree's stream goes down whole. - A dropped client kept its listener port for the life of the process, so a retried login advertised an ephemeral port nobody was told about. The listener is now stopped and the operations thread let go. - Excluded phrases were compared against a lowercased path without being lowercased themselves, so a capitalised phrase never matched. They are normalised where they arrive. - Losing our own operatorship wiped the room's whole operator roster, though we are still a member who can see it. - A brokered ConnectToPeer of type D was logged and dropped; the peer is now told, since we never dial out to adopt a child. - A peer request made with no server connection returned Ok and parked forever; it says NotConnected, as its docs always claimed. - An empty interest was refused locally but still sent, leaving the server's list and ours permanently apart. Simplification: - Deleted the excluded-phrase helper nothing called, and the field doc that promised behaviour it never had. - The four roster handlers and four standing handlers were eight constructors carrying constants their own message code already implies: one newtype each, 100 lines less. - One place registers a search; the phrase list is borrowed, not cloned per relayed search.
This commit is contained in:
@@ -1,9 +1,10 @@
|
||||
use super::{
|
||||
Arc, Client, ClientContext, ClientOperation, ConnectionType, DownloadPeer,
|
||||
Listen, Peer, PeerRegistry, Receiver, Result, RwLock, RwLockExt, Sender,
|
||||
ServerActor, ServerMessage, Shares, SoulseekRs, TcpStream, error, info,
|
||||
mpsc, thread, trace, warn,
|
||||
Arc, AtomicBool, Client, ClientContext, ClientOperation, ConnectionType,
|
||||
DownloadPeer, Listen, Peer, PeerRegistry, Receiver, Result, RwLock,
|
||||
RwLockExt, Sender, ServerActor, ServerMessage, Shares, SoulseekRs,
|
||||
TcpStream, error, info, mpsc, thread, trace, warn,
|
||||
};
|
||||
use std::sync::atomic::Ordering;
|
||||
|
||||
/// 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.
|
||||
@@ -96,6 +97,7 @@ impl Client {
|
||||
} else {
|
||||
None
|
||||
};
|
||||
self.listener_stopped = Arc::new(AtomicBool::new(false));
|
||||
|
||||
let mut server_actor = ServerActor::new(
|
||||
self.address.clone(),
|
||||
@@ -118,9 +120,16 @@ impl Client {
|
||||
let client_sender = listen_sender;
|
||||
let context = self.context.clone();
|
||||
let own_username = self.username.clone();
|
||||
let stopped = self.listener_stopped.clone();
|
||||
|
||||
thread::spawn(move || {
|
||||
Listen::serve(&listener, client_sender, context, own_username);
|
||||
Listen::serve(
|
||||
&listener,
|
||||
client_sender,
|
||||
context,
|
||||
own_username,
|
||||
stopped,
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
@@ -147,14 +156,26 @@ impl Client {
|
||||
if let Some(handle) = self.server_handle.take() {
|
||||
let _ = handle.stop();
|
||||
}
|
||||
let registry = self
|
||||
.context
|
||||
.read_safe()
|
||||
.ok()
|
||||
.and_then(|ctx| ctx.peer_registry.clone());
|
||||
let registry = match self.context.write_safe() {
|
||||
Ok(mut ctx) => {
|
||||
// Dropping our end of the operations channel is what lets its
|
||||
// thread finish once the peers and the listener have gone.
|
||||
ctx.operations = None;
|
||||
ctx.peer_registry.clone()
|
||||
}
|
||||
Err(_) => None,
|
||||
};
|
||||
if let Some(registry) = registry {
|
||||
registry.stop_all();
|
||||
}
|
||||
// The listener thread is parked in accept(), so it only notices the
|
||||
// flag when a connection arrives: dial it once ourselves. Without
|
||||
// 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));
|
||||
}
|
||||
}
|
||||
|
||||
/// Log in and wait for the server's verdict.
|
||||
@@ -355,7 +376,17 @@ impl Client {
|
||||
}
|
||||
}
|
||||
ConnectionType::D => {
|
||||
error!("ConnectionType::D not implemented");
|
||||
// ponytail: we never dial out to adopt a child — children
|
||||
// reach us through the listener. Tell the peer rather than
|
||||
// leave it waiting; wire the outbound half if firewalled
|
||||
// children turn out to matter.
|
||||
if let (true, Some(token)) = (peer.brokered, peer.token) {
|
||||
Self::report_cant_connect(
|
||||
&client_context,
|
||||
token,
|
||||
&peer.username,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user