fix(distributed): a server with no parent figures leaves our cap alone
The speed-derived child limit read a missing ratio as a refusal, so on any server that never announces ParentSpeedRatio — soulfind among them — the client carried no children at all, however it was configured. A ratio we were never told is not a decision: the server's figures set the limit only when it has given them, and otherwise the configured cap stands, which is how slskd runs throughout. A speed below the announced minimum still means none, which is the safety property the rule exists for. Also stops repeating AcceptChildren: it is a standing state on the server, so it goes out when the answer changes rather than on every message that happens to prompt a recount.
This commit is contained in:
@@ -32,17 +32,24 @@ use crate::{debug, trace};
|
|||||||
pub const MAX_CHILDREN: usize = 10;
|
pub const MAX_CHILDREN: usize = 10;
|
||||||
|
|
||||||
/// The child limit the server's figures imply, following Nicotine+:
|
/// The child limit the server's figures imply, following Nicotine+:
|
||||||
/// `speed / ratio / 100`, capped, and none at all below the minimum speed the
|
/// `speed / ratio / 100`, capped, and none at all when our recorded speed is
|
||||||
/// server set. `None` when the server has not reported our speed yet, which
|
/// below the minimum the server set.
|
||||||
/// leaves the caller's own cap standing.
|
///
|
||||||
|
/// `None` means the server's figures do not decide it — either it has not
|
||||||
|
/// reported our speed yet, or it never announced a ratio at all (soulfind
|
||||||
|
/// does not) — and the caller's own cap stands, which is how slskd runs the
|
||||||
|
/// whole time.
|
||||||
#[must_use]
|
#[must_use]
|
||||||
pub fn limit_from_speed(
|
pub fn limit_from_speed(
|
||||||
own_speed: Option<u32>,
|
own_speed: Option<u32>,
|
||||||
min_speed: u32,
|
min_speed: u32,
|
||||||
ratio: u32,
|
ratio: u32,
|
||||||
) -> Option<usize> {
|
) -> Option<usize> {
|
||||||
|
if ratio == 0 {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
let speed = own_speed?;
|
let speed = own_speed?;
|
||||||
if ratio == 0 || speed < min_speed {
|
if speed < min_speed {
|
||||||
return Some(0);
|
return Some(0);
|
||||||
}
|
}
|
||||||
Some((speed / ratio / 100) as usize)
|
Some((speed / ratio / 100) as usize)
|
||||||
@@ -336,8 +343,8 @@ mod tests {
|
|||||||
);
|
);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
limit_from_speed(Some(10_000), 1_024, 0),
|
limit_from_speed(Some(10_000), 1_024, 0),
|
||||||
Some(0),
|
None,
|
||||||
"a zero ratio means the server is not allowing children"
|
"a server that never announced a ratio does not decide the limit"
|
||||||
);
|
);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
limit_from_speed(None, 1_024, 50),
|
limit_from_speed(None, 1_024, 50),
|
||||||
@@ -364,6 +371,12 @@ mod tests {
|
|||||||
|
|
||||||
children.set_limits(Some(100), 1_024, 50);
|
children.set_limits(Some(100), 1_024, 50);
|
||||||
assert!(!children.has_room(), "a slow client carries none");
|
assert!(!children.has_room(), "a slow client carries none");
|
||||||
|
|
||||||
|
children.set_limits(Some(100), 1_024, 0);
|
||||||
|
assert!(
|
||||||
|
children.has_room(),
|
||||||
|
"a server with no figures leaves our own cap standing"
|
||||||
|
);
|
||||||
assert_eq!(ends.len(), 2);
|
assert_eq!(ends.len(), 2);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -321,8 +321,11 @@ pub fn announce_move(
|
|||||||
) {
|
) {
|
||||||
let server = ctx.server_sender.clone();
|
let server = ctx.server_sender.clone();
|
||||||
// What the server needs to know is whether it may offer us to another
|
// What the server needs to know is whether it may offer us to another
|
||||||
// peer right now, not whether we serve children in principle.
|
// peer right now, not whether we serve children in principle. The stance
|
||||||
|
// carries it, so record it as told: a separate AcceptChildren right
|
||||||
|
// behind this one would say nothing new.
|
||||||
let has_room = ctx.children.has_room();
|
let has_room = ctx.children.has_room();
|
||||||
|
let _ = ctx.accept_children_change();
|
||||||
announce(server.as_ref(), branch, has_parent, has_room);
|
announce(server.as_ref(), branch, has_parent, has_room);
|
||||||
ctx.children.broadcast_stance(&branch.root, branch.level);
|
ctx.children.broadcast_stance(&branch.root, branch.level);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -466,6 +466,9 @@ pub struct ClientContext {
|
|||||||
leaf: distributed::Leaf,
|
leaf: distributed::Leaf,
|
||||||
/// The children hanging from us, when the client serves any.
|
/// The children hanging from us, when the client serves any.
|
||||||
pub(crate) children: children::Children,
|
pub(crate) children: children::Children,
|
||||||
|
/// The last `AcceptChildren` answer we gave the server, so the same
|
||||||
|
/// answer is not repeated: it is a standing state, not a heartbeat.
|
||||||
|
announced_accept_children: Option<bool>,
|
||||||
/// The upload speed the server records for us, once it has told us. The
|
/// The upload speed the server records for us, once it has told us. The
|
||||||
/// child limit is derived from it.
|
/// child limit is derived from it.
|
||||||
own_average_speed: Option<u32>,
|
own_average_speed: Option<u32>,
|
||||||
@@ -642,6 +645,7 @@ impl ClientContext {
|
|||||||
operations: None,
|
operations: None,
|
||||||
leaf: distributed::Leaf::new(""),
|
leaf: distributed::Leaf::new(""),
|
||||||
children: children::Children::default(),
|
children: children::Children::default(),
|
||||||
|
announced_accept_children: None,
|
||||||
own_average_speed: None,
|
own_average_speed: None,
|
||||||
parent_min_speed: 0,
|
parent_min_speed: 0,
|
||||||
parent_speed_ratio: 0,
|
parent_speed_ratio: 0,
|
||||||
@@ -829,6 +833,16 @@ impl ClientContext {
|
|||||||
self.own_interests.clone()
|
self.own_interests.clone()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Take the `AcceptChildren` answer to send, or `None` when the server
|
||||||
|
/// already has this one.
|
||||||
|
pub fn accept_children_change(&mut self) -> Option<bool> {
|
||||||
|
let has_room = self.children.has_room();
|
||||||
|
(self.announced_accept_children != Some(has_room)).then(|| {
|
||||||
|
self.announced_accept_children = Some(has_room);
|
||||||
|
has_room
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
/// The upload speed the server records for us, if it has said.
|
/// The upload speed the server records for us, if it has said.
|
||||||
#[must_use]
|
#[must_use]
|
||||||
pub const fn own_average_speed(&self) -> Option<u32> {
|
pub const fn own_average_speed(&self) -> Option<u32> {
|
||||||
|
|||||||
@@ -1253,8 +1253,16 @@ impl Client {
|
|||||||
/// (`AcceptChildren`, code 100). Sent whenever that answer changes: the
|
/// (`AcceptChildren`, code 100). Sent whenever that answer changes: the
|
||||||
/// server only offers us to peers while it is true.
|
/// server only offers us to peers while it is true.
|
||||||
fn announce_child_capacity(client_context: &Arc<RwLock<ClientContext>>) {
|
fn announce_child_capacity(client_context: &Arc<RwLock<ClientContext>>) {
|
||||||
let (sender, has_room) = match client_context.read_safe() {
|
let (sender, has_room) = match client_context.write_safe() {
|
||||||
Ok(ctx) => (ctx.server_sender.clone(), ctx.children.has_room()),
|
Ok(mut ctx) => {
|
||||||
|
// Only on a change: the server holds this as a standing
|
||||||
|
// state, and repeating it on every message we happen to
|
||||||
|
// receive would be noise.
|
||||||
|
let Some(has_room) = ctx.accept_children_change() else {
|
||||||
|
return;
|
||||||
|
};
|
||||||
|
(ctx.server_sender.clone(), has_room)
|
||||||
|
}
|
||||||
Err(_) => return,
|
Err(_) => return,
|
||||||
};
|
};
|
||||||
let Some(sender) = sender else { return };
|
let Some(sender) = sender else { return };
|
||||||
|
|||||||
@@ -1126,3 +1126,15 @@ fn leaving_a_room_drops_its_ticker_board() {
|
|||||||
"a board for a room we left is stale"
|
"a board for a room we left is stale"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn the_server_is_told_our_child_capacity_only_when_it_changes() {
|
||||||
|
// AcceptChildren is a standing state on the server, not a heartbeat.
|
||||||
|
let mut ctx = ClientContext::new();
|
||||||
|
assert_eq!(
|
||||||
|
ctx.accept_children_change(),
|
||||||
|
Some(false),
|
||||||
|
"the first answer is always worth sending"
|
||||||
|
);
|
||||||
|
assert_eq!(ctx.accept_children_change(), None, "unchanged, so silent");
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user