From 55f65668fd7462a97c49d24b5fac4537f61bc65e Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 14 Apr 2020 21:00:01 +0200 Subject: [PATCH 1/5] protocols/kad/query/peers/closest: Add test for at_capacity on stalled --- protocols/kad/src/query/peers/closest.rs | 73 ++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index f4e37e3d12e..b8a4a2e0b3b 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -691,4 +691,77 @@ mod tests { QuickCheck::new().tests(10).quickcheck(prop as fn(_) -> _) } + + #[test] + fn stalled_iter_at_capacity_use_max_of_parallelism_and_num_results() { + let target: KeyBytes = Key::from(PeerId::random()).into(); + + let mut iter = ClosestPeersIter::with_config( + ClosestPeersIterConfig { + num_results: 4, + parallelism: 8, + ..ClosestPeersIterConfig::default() + }, + target, + std::iter::empty(), + ); + iter.state = State::Stalled; + iter.num_waiting = 7; // Smaller than `parallelism`, but larger than `num_results`. + + assert!(!iter.at_capacity()); + } + + /// When not making progress for `parallelism` time a [`ClosestPeersIter`] becomes + /// [`State::Stalled`]. When stalled an iterator is allowed to make more parallel requests up to + /// `num_results`. If `num_results` is smaller than `parallelism` make sure to still allow up to + /// `parallelism` requests in-flight. + #[test] + fn stalled_iter_allows_equal_or_more_than_parallelism_in_flight() { + let now = Instant::now(); + + let mut num_requests_in_flight = 0; + + let parallelism = 8; + let num_results = parallelism / 2; + let target: KeyBytes = Key::from(PeerId::random()).into(); + + let mut closest_peers = random_peers(100).map(Key::from).collect::>(); + closest_peers.sort_unstable_by(|a, b| { + target.distance(a).cmp(&target.distance(b)) + }); + let mut pool = closest_peers.split_off(K_VALUE.get()); + + let mut iter = ClosestPeersIter::with_config( + ClosestPeersIterConfig { + num_results, + parallelism, + ..ClosestPeersIterConfig::default() + }, + target, + closest_peers.into_iter(), + ); + + // Have the request for the closest known peer be in-flight for the remainder of the test. + // Thereby the iterator never finishes (it ignores timeouts). + iter.next(now); + num_requests_in_flight += 1; + + while matches!(iter.state, State::Iterating{ .. }) { + if let PeersIterState::Waiting(Some(peer)) = iter.next(now) { + let peer = peer.clone().into_owned(); + iter.on_success( + &peer, + pool.pop().map(|p| vec![p.preimage().clone()]).unwrap().into_iter(), + ); + } else { + panic!("Expected iterator to yield another peer."); + } + } + + while matches!(iter.next(now), PeersIterState::Waiting(Some(_))) { + num_requests_in_flight += 1; + } + + assert_eq!(usize::max(parallelism, num_results), num_requests_in_flight); + } } From b4f76417d9fdddbd66c79e7f63c810a4788b6c38 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 14 Apr 2020 21:02:04 +0200 Subject: [PATCH 2/5] protocols/kad/query/peers/closest: Make at_capacity use max When not making progress for `parallelism` time a `ClosestPeersIter` becomes `State::Stalled`. When stalled an iterator is allowed to make more parallel requests up to `num_results`. If `num_results` is smaller than `parallelism` make sure to still allow up to `parallelism` requests in-flight. --- protocols/kad/src/query/peers/closest.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index b8a4a2e0b3b..f8fe5d6d767 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -369,7 +369,9 @@ impl ClosestPeersIter { /// k closest nodes it has not already queried". fn at_capacity(&self) -> bool { match self.state { - State::Stalled => self.num_waiting >= self.config.num_results, + State::Stalled => self.num_waiting >= usize::max( + self.config.num_results, self.config.parallelism + ), State::Iterating { .. } => self.num_waiting >= self.config.parallelism, State::Finished => true } From f38d0c773d25532df4b3e0d59976267c1036e840 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 14 Apr 2020 23:17:41 +0200 Subject: [PATCH 3/5] protocols/kad/query/peers/closest: Fix test with alpha initial peers --- protocols/kad/src/query/peers/closest.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index f8fe5d6d767..9d9a305dde1 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -727,7 +727,7 @@ mod tests { let num_results = parallelism / 2; let target: KeyBytes = Key::from(PeerId::random()).into(); - let mut closest_peers = random_peers(100).map(Key::from).collect::>(); + let mut closest_peers = random_peers(1000).map(Key::from).collect::>(); closest_peers.sort_unstable_by(|a, b| { target.distance(a).cmp(&target.distance(b)) }); @@ -753,7 +753,8 @@ mod tests { let peer = peer.clone().into_owned(); iter.on_success( &peer, - pool.pop().map(|p| vec![p.preimage().clone()]).unwrap().into_iter(), + // Split off 10 nodes - non of them being any closer. + pool.split_off(pool.len() - 10).into_iter().map(|p| p.preimage().clone()), ); } else { panic!("Expected iterator to yield another peer."); From 49b908ccd9101ebb4dd75479c21208681439c5c1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 16 Apr 2020 09:30:17 +0200 Subject: [PATCH 4/5] protocols/kad/query/closest: Remove one and prop the other Remove in-depth test and make smaller test property based. --- protocols/kad/src/query/peers/closest.rs | 85 +++++------------------- 1 file changed, 18 insertions(+), 67 deletions(-) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index 9d9a305dde1..46d0ff90285 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -695,76 +695,27 @@ mod tests { } #[test] - fn stalled_iter_at_capacity_use_max_of_parallelism_and_num_results() { - let target: KeyBytes = Key::from(PeerId::random()).into(); - - let mut iter = ClosestPeersIter::with_config( - ClosestPeersIterConfig { - num_results: 4, - parallelism: 8, - ..ClosestPeersIterConfig::default() - }, - target, - std::iter::empty(), - ); - iter.state = State::Stalled; - iter.num_waiting = 7; // Smaller than `parallelism`, but larger than `num_results`. - - assert!(!iter.at_capacity()); - } - - /// When not making progress for `parallelism` time a [`ClosestPeersIter`] becomes - /// [`State::Stalled`]. When stalled an iterator is allowed to make more parallel requests up to - /// `num_results`. If `num_results` is smaller than `parallelism` make sure to still allow up to - /// `parallelism` requests in-flight. - #[test] - fn stalled_iter_allows_equal_or_more_than_parallelism_in_flight() { - let now = Instant::now(); - - let mut num_requests_in_flight = 0; - - let parallelism = 8; - let num_results = parallelism / 2; - let target: KeyBytes = Key::from(PeerId::random()).into(); - - let mut closest_peers = random_peers(1000).map(Key::from).collect::>(); - closest_peers.sort_unstable_by(|a, b| { - target.distance(a).cmp(&target.distance(b)) - }); - let mut pool = closest_peers.split_off(K_VALUE.get()); - - let mut iter = ClosestPeersIter::with_config( - ClosestPeersIterConfig { - num_results, - parallelism, - ..ClosestPeersIterConfig::default() - }, - target, - closest_peers.into_iter(), - ); - - // Have the request for the closest known peer be in-flight for the remainder of the test. - // Thereby the iterator never finishes (it ignores timeouts). - iter.next(now); - num_requests_in_flight += 1; - - while matches!(iter.state, State::Iterating{ .. }) { - if let PeersIterState::Waiting(Some(peer)) = iter.next(now) { - let peer = peer.clone().into_owned(); - iter.on_success( - &peer, - // Split off 10 nodes - non of them being any closer. - pool.split_off(pool.len() - 10).into_iter().map(|p| p.preimage().clone()), - ); - } else { - panic!("Expected iterator to yield another peer."); + fn stalled_iter_at_capacity_max_of_parallelism_and_num_results() { + fn prop(mut iter: ClosestPeersIter) { + iter.state = State::Stalled; + + for i in 0..usize::max(iter.config.parallelism, iter.config.num_results) { + iter.num_waiting = i; + assert!( + !iter.at_capacity(), + "Iterator should not be at capacity if less than \ + `max(parallelism, num_results)` requests are waiting.", + ) } - } - while matches!(iter.next(now), PeersIterState::Waiting(Some(_))) { - num_requests_in_flight += 1; + iter.num_waiting = usize::max(iter.config.parallelism, iter.config.num_results); + assert!( + iter.at_capacity(), + "Iterator should be at capacity if `max(parallelism, num_results)` requests are \ + waiting.", + ) } - assert_eq!(usize::max(parallelism, num_results), num_requests_in_flight); + QuickCheck::new().tests(10).quickcheck(prop as fn(_)) } } From a5246e94b779b6e2f029b684b6fa71fc616ba029 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 16 Apr 2020 15:11:31 +0200 Subject: [PATCH 5/5] protocols/kad/src/query/peers/closest.rs: Apply suggestion Co-Authored-By: Roman Borschel --- protocols/kad/src/query/peers/closest.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/query/peers/closest.rs b/protocols/kad/src/query/peers/closest.rs index 46d0ff90285..987d0c36c85 100644 --- a/protocols/kad/src/query/peers/closest.rs +++ b/protocols/kad/src/query/peers/closest.rs @@ -695,7 +695,7 @@ mod tests { } #[test] - fn stalled_iter_at_capacity_max_of_parallelism_and_num_results() { + fn stalled_at_capacity() { fn prop(mut iter: ClosestPeersIter) { iter.state = State::Stalled;