fix: survive busy sessions without exhausting file descriptors
The peer listener died with "Too many open files (os error 24)" on any long-lived non-daemon session: only the daemon raised RLIMIT_NOFILE, and peer sockets were unbounded — no idle reaping, no deadlines on accepted transfer streams, and one peer could hold two connections under split registry identities. - raise the open-file limit at the top of run(), for every entry point, and warn from Client::connect when the soft limit is below 1024 - reap peer control connections idle for five minutes; a clean disconnect no longer discards the peer's queued uploads - give listener-supplied download streams and upload serving the same read/write deadlines dialled sockets already had - register inbound peers under their plain username so the registry holds one connection per peer, and announce them like pierced ones - hold the registry lock across actor spawn + insert and surface thread-spawn failure, so an instantly-dead peer cannot leave a zombie entry
This commit is contained in:
@@ -87,9 +87,34 @@ impl ActorSystem {
|
||||
/// The callback receives the actor handle before `on_start` is called
|
||||
pub fn spawn_with_handle<A: Actor, F>(
|
||||
&self,
|
||||
mut actor: A,
|
||||
actor: A,
|
||||
init: F,
|
||||
) -> ActorHandle<A::Message>
|
||||
where
|
||||
F: FnOnce(&mut A, ActorHandle<A::Message>) + Send + 'static,
|
||||
{
|
||||
match self.try_spawn_with_handle(actor, init) {
|
||||
Ok(handle) => handle,
|
||||
Err(e) => {
|
||||
error!("[actor_system] failed to spawn actor thread: {}", e);
|
||||
// A handle nobody serves: sends fail, which callers of the
|
||||
// infallible spawn already tolerate.
|
||||
let (sender, _) = channel();
|
||||
ActorHandle { sender }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Like [`Self::spawn_with_handle`], but hands a thread-spawn failure back
|
||||
/// to the caller instead of returning a handle no actor will ever serve.
|
||||
/// Storing such a dead handle is how a registry entry becomes a zombie
|
||||
/// that claims its username for the rest of the process.
|
||||
#[allow(clippy::unused_self)] // method for symmetry with the spawn family
|
||||
pub(crate) fn try_spawn_with_handle<A: Actor, F>(
|
||||
&self,
|
||||
mut actor: A,
|
||||
init: F,
|
||||
) -> std::io::Result<ActorHandle<A::Message>>
|
||||
where
|
||||
F: FnOnce(&mut A, ActorHandle<A::Message>) + Send + 'static,
|
||||
{
|
||||
@@ -97,7 +122,7 @@ impl ActorSystem {
|
||||
let handle = ActorHandle { sender };
|
||||
let handle_for_init = handle.clone();
|
||||
|
||||
let spawned = thread::Builder::new()
|
||||
thread::Builder::new()
|
||||
.name("soulseek-actor".to_string())
|
||||
.stack_size(ACTOR_STACK_SIZE)
|
||||
.spawn(move || {
|
||||
@@ -105,13 +130,9 @@ impl ActorSystem {
|
||||
actor.on_start();
|
||||
Self::run_actor_loop(&mut actor, receiver);
|
||||
actor.on_stop();
|
||||
});
|
||||
})?;
|
||||
|
||||
if let Err(e) = spawned {
|
||||
error!("[actor_system] failed to spawn actor thread: {}", e);
|
||||
}
|
||||
|
||||
handle
|
||||
Ok(handle)
|
||||
}
|
||||
|
||||
fn run_actor_loop<A: Actor>(
|
||||
|
||||
@@ -88,12 +88,23 @@ pub struct PeerActor {
|
||||
/// larger than the send buffer — a full share listing is easily megabytes —
|
||||
/// only partly fits; the rest drains on later ticks.
|
||||
pending_write: Vec<u8>,
|
||||
/// When bytes last moved on this connection, in either direction. A peer
|
||||
/// that goes quiet is eventually reaped: nothing else ever closes an idle
|
||||
/// control connection, and each one holds a descriptor and a thread for
|
||||
/// the life of the process.
|
||||
last_activity: Instant,
|
||||
}
|
||||
|
||||
/// Cap on queued outbound bytes. A peer that stops reading must not be able to
|
||||
/// grow this without bound; dropping that connection is the right answer.
|
||||
const MAX_PENDING_WRITE: usize = 32 * 1024 * 1024;
|
||||
|
||||
/// How long a connected peer may stay silent before its control connection is
|
||||
/// closed. Transfers run on their own sockets, and a peer that wants us again
|
||||
/// simply reconnects — every serious client reaps idle peers this way. Five
|
||||
/// minutes comfortably outlives a search's chatter and queue updates.
|
||||
const IDLE_DISCONNECT: Duration = Duration::from_mins(5);
|
||||
|
||||
impl PeerActor {
|
||||
#[must_use]
|
||||
pub fn new(
|
||||
@@ -128,6 +139,7 @@ impl PeerActor {
|
||||
id,
|
||||
serving_tokens: std::collections::HashSet::new(),
|
||||
pending_write: Vec::new(),
|
||||
last_activity: Instant::now(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -460,7 +472,9 @@ impl PeerActor {
|
||||
};
|
||||
|
||||
match self.reader.read_from_socket(stream) {
|
||||
Ok(()) => {}
|
||||
Ok(()) => {
|
||||
self.last_activity = Instant::now();
|
||||
}
|
||||
Err(ref e) if e.kind() == io::ErrorKind::WouldBlock => {}
|
||||
Err(ref e) if e.kind() == io::ErrorKind::TimedOut => {
|
||||
if let Ok(peer_lock) = self.peer.read_safe() {
|
||||
@@ -609,6 +623,9 @@ impl PeerActor {
|
||||
}
|
||||
};
|
||||
self.pending_write.drain(..written);
|
||||
if written > 0 {
|
||||
self.last_activity = Instant::now();
|
||||
}
|
||||
|
||||
if let Err(e) = outcome {
|
||||
error!(
|
||||
@@ -876,9 +893,75 @@ impl Actor for PeerActor {
|
||||
self.flush_pending_write();
|
||||
self.process_read();
|
||||
self.flush_pending_write();
|
||||
|
||||
if self.stream.is_some()
|
||||
&& self.last_activity.elapsed() > IDLE_DISCONNECT
|
||||
{
|
||||
debug!(
|
||||
"[peer:{}] idle for {}s, closing",
|
||||
self.peer_username(),
|
||||
IDLE_DISCONNECT.as_secs()
|
||||
);
|
||||
self.disconnect();
|
||||
}
|
||||
}
|
||||
}
|
||||
ConnectionState::Disconnected => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::peer::{ConnectionType, Peer};
|
||||
use std::net::{TcpListener, TcpStream};
|
||||
use std::sync::mpsc::Receiver;
|
||||
|
||||
/// A connected inbound actor over a real loopback socket, plus the far
|
||||
/// end (kept alive so reads see silence, not EOF).
|
||||
fn connected_actor() -> (PeerActor, Receiver<ClientOperation>, TcpStream) {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
let addr = listener.local_addr().unwrap();
|
||||
let stream = TcpStream::connect(addr).unwrap();
|
||||
stream.set_nonblocking(true).unwrap();
|
||||
let (far_end, _) = listener.accept().unwrap();
|
||||
|
||||
let (tx, rx) = std::sync::mpsc::channel();
|
||||
let peer = Peer::new(
|
||||
"bob".to_string(),
|
||||
ConnectionType::P,
|
||||
"127.0.0.1".to_string(),
|
||||
u32::from(addr.port()),
|
||||
None,
|
||||
0,
|
||||
0,
|
||||
0,
|
||||
);
|
||||
let mut actor =
|
||||
PeerActor::new(peer, Some(stream), None, tx, "me".to_string(), 7);
|
||||
actor.on_start();
|
||||
(actor, rx, far_end)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn tick_reaps_a_peer_idle_past_the_deadline() {
|
||||
let (mut actor, rx, _far_end) = connected_actor();
|
||||
|
||||
actor.tick();
|
||||
assert!(actor.stream.is_some(), "a fresh connection stays open");
|
||||
|
||||
actor.last_activity = Instant::now()
|
||||
.checked_sub(IDLE_DISCONNECT + Duration::from_secs(1))
|
||||
.unwrap();
|
||||
actor.tick();
|
||||
|
||||
assert!(actor.stream.is_none(), "an idle stream must be closed");
|
||||
match rx.try_recv() {
|
||||
Ok(ClientOperation::PeerDisconnected(7, username, None)) => {
|
||||
assert_eq!(username, "bob");
|
||||
}
|
||||
other => panic!("expected a clean PeerDisconnected, got {other:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -60,15 +60,23 @@ impl PeerRegistry {
|
||||
id,
|
||||
);
|
||||
|
||||
let handle =
|
||||
self.actor_system.spawn_with_handle(actor, |actor, handle| {
|
||||
actor.set_self_handle(handle);
|
||||
});
|
||||
|
||||
// Take the map lock before the actor exists: a peer that dies
|
||||
// instantly (refused dial, immediate hangup) reports its terminal
|
||||
// outcome to the client loop, whose eviction takes this same lock.
|
||||
// With the insert racing the spawn, eviction could run first, find
|
||||
// nothing, and the entry inserted afterwards became a permanent
|
||||
// zombie claiming the username.
|
||||
let mut peers = self
|
||||
.peers
|
||||
.lock_safe()
|
||||
.map_err(|e| format!("peer registry lock poisoned: {e}"))?;
|
||||
|
||||
let handle = self
|
||||
.actor_system
|
||||
.try_spawn_with_handle(actor, |actor, handle| {
|
||||
actor.set_self_handle(handle);
|
||||
})
|
||||
.map_err(|e| format!("failed to spawn peer actor thread: {e}"))?;
|
||||
// Stop any actor already registered under this username so it does not
|
||||
// become an orphan pinning a pool worker forever. Eviction on the
|
||||
// replaced actor's later shutdown is identity-aware (keyed on its id),
|
||||
@@ -233,4 +241,51 @@ mod tests {
|
||||
let _ = handle.unwrap().stop();
|
||||
assert!(!registry.contains("bob"));
|
||||
}
|
||||
|
||||
// A dial that is refused reports its terminal outcome almost instantly —
|
||||
// possibly while register_peer is still between spawn and insert. The
|
||||
// registry holds its lock across both, so the eviction that follows must
|
||||
// always find the entry; a miss left a permanent zombie claiming the
|
||||
// username.
|
||||
#[test]
|
||||
fn refused_dial_does_not_leave_a_zombie_entry() {
|
||||
use crate::client::ClientOperation;
|
||||
|
||||
let system = Arc::new(ActorSystem::new());
|
||||
let (tx, rx) = std::sync::mpsc::channel();
|
||||
let registry = PeerRegistry::new(system, tx, "me".to_string());
|
||||
|
||||
// A port with nothing listening behind it: bind, learn it, drop it.
|
||||
let port = {
|
||||
let probe = TcpListener::bind("127.0.0.1:0").unwrap();
|
||||
probe.local_addr().unwrap().port()
|
||||
};
|
||||
|
||||
let peer = Peer::new(
|
||||
"ghost".to_string(),
|
||||
ConnectionType::P,
|
||||
"127.0.0.1".to_string(),
|
||||
u32::from(port),
|
||||
None,
|
||||
0,
|
||||
0,
|
||||
0,
|
||||
);
|
||||
registry.register_peer(peer, None, None).unwrap();
|
||||
|
||||
// Play the client ops loop: take the terminal outcome, evict by id.
|
||||
match rx.recv_timeout(std::time::Duration::from_secs(10)) {
|
||||
Ok(ClientOperation::PeerConnectFailed(id, username)) => {
|
||||
assert_eq!(username, "ghost");
|
||||
if let Some(handle) = registry.remove_peer_if(&username, id) {
|
||||
let _ = handle.stop();
|
||||
}
|
||||
}
|
||||
other => panic!("expected PeerConnectFailed, got {other:?}"),
|
||||
}
|
||||
assert!(
|
||||
!registry.contains("ghost"),
|
||||
"a refused dial must not leave a registry entry"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user