feat: lift the client's concurrency ceiling from ~15 peers to 1024
The client had never been driven at scale. A stress harness that mixes searches, downloads, uploads and browses against a local soulfind found four ceilings, three of which lose work silently rather than merely being slow. Peers were capped at one per core. ActorSystem ran each actor's whole lifetime as one job on a fixed-size ThreadPool, and actor loops block until stopped, so the pool acted as a semaphore: past the cap an actor never started, never connected and never errored, and uploads stopped working entirely. The pool had exactly one consumer and every job it received was non-terminating, so it is gone; actors get their own thread. TransferResponse raced the bookkeeping it depends on. It was sent straight back to the peer while UpdateDownloadTokens was still queued for the client loop — but the response is what invites the peer's file connection, and that connection is matched by token. Under load the client loop lost, the file connection matched nothing, and the download sat on Queued forever. The response now goes out after the token is recorded. Download tokens were md5(filename) truncated to 20 bits, and the store deletes every entry matching a token, so two downloads sharing one destroyed each other — which fetching the same filename from two peers does every time. They now come from a counter. Port 0 from GetPeerAddress means the server does not know where a user listens. It was cached and dialled, and the upload waiting on it was dropped. It is no longer cached or served against; the connect attempt stays, because its failure is what makes an unreachable peer fall back to server brokering. Mixed profile at 1024 peers: 512/512 downloads, 256/256 uploads, 400-850 MiB/s in ~7s, against 71/96 downloads at 7.9 MiB/s and a 60s timeout before.
This commit is contained in:
Michel de Graaf
@@ -1,6 +1,7 @@
|
||||
use super::{
|
||||
Arc, Client, ClientContext, Download, DownloadMetadata, DownloadStatus,
|
||||
Receiver, Result, RwLock, RwLockExt, Sender, error, info, md5, mpsc,
|
||||
Receiver, Result, RwLock, RwLockExt, Sender, error, info, mpsc,
|
||||
next_download_token,
|
||||
};
|
||||
|
||||
impl Client {
|
||||
@@ -52,9 +53,8 @@ impl Client {
|
||||
}
|
||||
|
||||
/// Remove every download for `username`/`filename` regardless of status.
|
||||
/// Call this before re-issuing [`Client::download`] for a failed download,
|
||||
/// otherwise the stale entry (whose md5-derived token equals the retry's)
|
||||
/// shadows the fresh one and its completion is misrouted.
|
||||
/// Useful before re-issuing [`Client::download`] for a failed download, so
|
||||
/// the list does not accumulate dead entries for the same file.
|
||||
///
|
||||
/// Returns whether anything was removed.
|
||||
#[must_use]
|
||||
@@ -94,8 +94,7 @@ impl Client {
|
||||
) -> Result<(Download, Receiver<DownloadStatus>)> {
|
||||
info!("[client] Downloading {} from {}", filename, username);
|
||||
|
||||
let hash = md5::md5(&filename);
|
||||
let token = u32::from_str_radix(&hash[0..5], 16)?;
|
||||
let token = next_download_token();
|
||||
|
||||
let (download_sender, download_receiver): (
|
||||
Sender<DownloadStatus>,
|
||||
@@ -189,3 +188,29 @@ impl Client {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::next_download_token;
|
||||
use std::collections::HashSet;
|
||||
|
||||
// The download store is keyed by token and removes *every* entry matching
|
||||
// one, so two downloads sharing a token means queueing one silently evicts
|
||||
// the other — which is what fetching the same filename from two peers does
|
||||
// if the token depends on the filename.
|
||||
#[test]
|
||||
fn download_tokens_are_unique_and_below_the_upload_range() {
|
||||
let tokens: Vec<u32> =
|
||||
(0..10_000).map(|_| next_download_token()).collect();
|
||||
|
||||
assert_eq!(
|
||||
tokens.iter().collect::<HashSet<_>>().len(),
|
||||
tokens.len(),
|
||||
"every download must get its own token"
|
||||
);
|
||||
assert!(
|
||||
tokens.iter().all(|t| *t < 0x8000_0000),
|
||||
"download tokens must not reach into the upload range"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -19,7 +19,7 @@ use crate::{
|
||||
},
|
||||
shares::Shares,
|
||||
types::{Download, Search, SearchResult},
|
||||
utils::{lock::RwLockExt, md5, thread_pool::ThreadPool},
|
||||
utils::{lock::RwLockExt, md5},
|
||||
};
|
||||
use std::{
|
||||
collections::{HashMap, HashSet},
|
||||
@@ -65,13 +65,25 @@ fn next_connect_token() -> u32 {
|
||||
}
|
||||
|
||||
/// Upload tokens are minted in the high half of the space so they never collide
|
||||
/// with download tokens (md5-derived, always < 2^20).
|
||||
/// with download tokens, which are always < 2^31.
|
||||
static NEXT_UPLOAD_TOKEN: AtomicU32 = AtomicU32::new(0x8000_0000);
|
||||
|
||||
fn next_upload_token() -> u32 {
|
||||
NEXT_UPLOAD_TOKEN.fetch_add(1, Ordering::Relaxed)
|
||||
}
|
||||
|
||||
/// Source of download tokens, kept in the low half of the space.
|
||||
///
|
||||
/// A counter, not a hash of the filename: the download store is keyed by token
|
||||
/// and removes every entry matching one, so any two downloads sharing a token
|
||||
/// destroy each other — and the same filename from two peers is an ordinary
|
||||
/// thing to queue.
|
||||
static NEXT_DOWNLOAD_TOKEN: AtomicU32 = AtomicU32::new(1);
|
||||
|
||||
fn next_download_token() -> u32 {
|
||||
NEXT_DOWNLOAD_TOKEN.fetch_add(1, Ordering::Relaxed) % 0x8000_0000
|
||||
}
|
||||
|
||||
/// A file we have agreed to serve to a peer, awaiting their TransferResponse.
|
||||
struct UploadJob {
|
||||
downloader: String,
|
||||
@@ -386,11 +398,7 @@ impl ClientContext {
|
||||
impl ClientContext {
|
||||
#[must_use]
|
||||
pub fn new() -> Self {
|
||||
let max_threads =
|
||||
thread::available_parallelism().map_or(8, std::num::NonZero::get);
|
||||
|
||||
let thread_pool = Arc::new(ThreadPool::new(max_threads));
|
||||
let actor_system = Arc::new(ActorSystem::new(thread_pool));
|
||||
let actor_system = Arc::new(ActorSystem::new());
|
||||
|
||||
Self {
|
||||
peer_registry: None,
|
||||
|
||||
@@ -316,9 +316,26 @@ impl Client {
|
||||
obfuscated_port
|
||||
);
|
||||
|
||||
// Cache the address for the serve/search paths,
|
||||
// and collect any uploads waiting on it.
|
||||
let waiting_serves =
|
||||
// Port 0 is the server saying it does not know
|
||||
// where this user listens (no SetWaitPort yet,
|
||||
// or firewalled). It is not an address: caching
|
||||
// it poisons every later lookup, and an upload
|
||||
// dialled at it dies with "Can't assign
|
||||
// requested address" and is dropped on the
|
||||
// floor. Leave those uploads queued for a later
|
||||
// resolution instead.
|
||||
//
|
||||
// The connect attempt further down still runs:
|
||||
// its failure is exactly what makes an
|
||||
// unreachable peer fall back to the
|
||||
// server-brokered path.
|
||||
let waiting_serves = if port == 0 {
|
||||
warn!(
|
||||
"[client] server reports no listening port for {}",
|
||||
username
|
||||
);
|
||||
Vec::new()
|
||||
} else {
|
||||
match client_context.write_safe() {
|
||||
Ok(mut ctx) => {
|
||||
ctx.cache_peer_address(
|
||||
@@ -331,7 +348,8 @@ impl Client {
|
||||
.unwrap_or_default()
|
||||
}
|
||||
Err(_) => Vec::new(),
|
||||
};
|
||||
}
|
||||
};
|
||||
for token in waiting_serves {
|
||||
Self::spawn_serve(
|
||||
&client_context,
|
||||
@@ -432,7 +450,7 @@ impl Client {
|
||||
|
||||
context.add_download(Download {
|
||||
username: username.clone(),
|
||||
filename: transfer.filename,
|
||||
filename: transfer.filename.clone(),
|
||||
token: transfer.token,
|
||||
size: transfer.size,
|
||||
download_directory: download
|
||||
@@ -444,6 +462,24 @@ impl Client {
|
||||
});
|
||||
context.remove_download(old_token);
|
||||
}
|
||||
|
||||
// Only now invite the file connection: it is
|
||||
// matched by this token, which is recorded as
|
||||
// of the line above. Answering any earlier
|
||||
// races the peer's connection against our own
|
||||
// bookkeeping.
|
||||
let registry = context.peer_registry.clone();
|
||||
drop(context);
|
||||
if let Some(registry) = registry {
|
||||
let response =
|
||||
crate::message::server::MessageFactory::build_transfer_response_message(
|
||||
transfer,
|
||||
);
|
||||
let _ = registry.send_to_peer(
|
||||
&username,
|
||||
PeerMessage::SendMessage(response),
|
||||
);
|
||||
}
|
||||
}
|
||||
ClientOperation::UploadFailed(
|
||||
username,
|
||||
|
||||
Reference in New Issue
Block a user