From 8b92ddd8e393edd5909ff348593b2bfd39a1df3f Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 9 Feb 2021 17:26:15 -0300 Subject: [PATCH 1/5] assert when peerset is buffered --- zebra-network/src/peer_set/set.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index e2b8b70e44d..f7bd8befe9c 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -423,6 +423,12 @@ where .ready_services .get_index_mut(index) .expect("preselected index must be valid"); + + // `PeerSet` calls `poll_ready` multiple times on its `Client` services. + // `Buffer`ed polls can fill up their Buffer reservations, causing hangs. + // The next `assert` will make sure the service type is never `Buffer`. #1593 + assert!(!format!("{:?}", type_of(&service)).contains("Buffer")); + trace!(preselected_index = index, ?key); match service.poll_ready(cx) { Poll::Ready(Ok(())) => return Poll::Ready(Ok(())), @@ -472,3 +478,9 @@ where } } } + +// Get the type of a variable as a string +use std::any::type_name; +fn type_of(_: T) -> &'static str { + type_name::() +} From 626a3151f9769fdb80db3d38f625862f51bb7fd2 Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 9 Feb 2021 20:20:14 -0300 Subject: [PATCH 2/5] add suggestions from code review Co-authored-by: teor --- zebra-network/src/peer_set/set.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index f7bd8befe9c..2e03b0864f9 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -424,10 +424,11 @@ where .get_index_mut(index) .expect("preselected index must be valid"); - // `PeerSet` calls `poll_ready` multiple times on its `Client` services. // `Buffer`ed polls can fill up their Buffer reservations, causing hangs. - // The next `assert` will make sure the service type is never `Buffer`. #1593 - assert!(!format!("{:?}", type_of(&service)).contains("Buffer")); + // See #1593 for details. + assert!(!format!("{:?}", type_of(&service)).contains("Buffer"), + "Clients must not use tower::Buffer, because PeerSet uses + poll_ready multiple times before each call, which can cause hangs with buffers"); trace!(preselected_index = index, ?key); match service.poll_ready(cx) { @@ -481,6 +482,8 @@ where // Get the type of a variable as a string use std::any::type_name; +// replace with type_name_of_val when it stabilises +// https://github.com/rust-lang/rust/issues/66359 fn type_of(_: T) -> &'static str { type_name::() } From e3562647ccb8b84694b499c68dd76ffbfb35731d Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 9 Feb 2021 22:34:32 -0300 Subject: [PATCH 3/5] use type_name() and remove type_of() --- zebra-network/src/peer_set/set.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index 2e03b0864f9..c9cfd56a240 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -1,5 +1,6 @@ use std::net::SocketAddr; use std::{ + any::type_name, collections::HashMap, fmt::Debug, future::Future, @@ -424,11 +425,13 @@ where .get_index_mut(index) .expect("preselected index must be valid"); - // `Buffer`ed polls can fill up their Buffer reservations, causing hangs. + // Using `poll_ready` without `call` fills up `Buffer` reservations, causing hangs. // See #1593 for details. - assert!(!format!("{:?}", type_of(&service)).contains("Buffer"), - "Clients must not use tower::Buffer, because PeerSet uses - poll_ready multiple times before each call, which can cause hangs with buffers"); + assert!( + !type_name::().contains("Buffer"), + "Clients must not use tower::Buffer, because PeerSet uses `poll_ready` + multiple times before each `call`, which causes buffer hangs" + ); trace!(preselected_index = index, ?key); match service.poll_ready(cx) { @@ -479,11 +482,3 @@ where } } } - -// Get the type of a variable as a string -use std::any::type_name; -// replace with type_name_of_val when it stabilises -// https://github.com/rust-lang/rust/issues/66359 -fn type_of(_: T) -> &'static str { - type_name::() -} From 82bd8a77247d939e7bb561ba5213380e9959108f Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 9 Feb 2021 22:45:54 -0300 Subject: [PATCH 4/5] move the assert outside the loop --- zebra-network/src/peer_set/set.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index c9cfd56a240..d90dbab7d38 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -406,6 +406,14 @@ where Pin> + Send + 'static>>; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { + // Using `poll_ready` without `call` fills up `Buffer` reservations, causing hangs. + // See #1593 for details. + assert!( + !type_name::().contains("Buffer"), + "Clients must not use tower::Buffer, because PeerSet uses `poll_ready` + multiple times before each `call`, which causes buffer hangs" + ); + self.poll_background_errors(cx)?; // Process peer discovery updates. let _ = self.poll_discover(cx)?; @@ -425,14 +433,6 @@ where .get_index_mut(index) .expect("preselected index must be valid"); - // Using `poll_ready` without `call` fills up `Buffer` reservations, causing hangs. - // See #1593 for details. - assert!( - !type_name::().contains("Buffer"), - "Clients must not use tower::Buffer, because PeerSet uses `poll_ready` - multiple times before each `call`, which causes buffer hangs" - ); - trace!(preselected_index = index, ?key); match service.poll_ready(cx) { Poll::Ready(Ok(())) => return Poll::Ready(Ok(())), From 3e2f7b562d0e642b3a1c2f4157edef8a074fbeec Mon Sep 17 00:00:00 2001 From: Alfredo Garcia Date: Tue, 9 Feb 2021 22:50:37 -0300 Subject: [PATCH 5/5] remove newline --- zebra-network/src/peer_set/set.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index d90dbab7d38..72e94e9716c 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -432,7 +432,6 @@ where .ready_services .get_index_mut(index) .expect("preselected index must be valid"); - trace!(preselected_index = index, ?key); match service.poll_ready(cx) { Poll::Ready(Ok(())) => return Poll::Ready(Ok(())),