From 685bdaf2df5f06fe7d18f036a00a7a9b07ab3cb3 Mon Sep 17 00:00:00 2001 From: Jane Lusby Date: Tue, 16 Jun 2020 13:12:09 -0700 Subject: [PATCH] don't require absense of cancel handles Prior to this change, we required that services that are canceled do not have a cancel handle in the `cancel_handles` list, based on the assumption that the handle must have been removed in the process of canceling this service. This doesn't holding up though, because it is currently possible for us to have the same peer connect to us multiple times, the second connect removes the cancel handle of the original connect and inserts it's own cancel handle in its place. In this scenario, when the first service is polled for readiness it will see that it has been canceled and go to clean itself up, but when it asserts that it doesn't have a cancel handle it will see the cancel handle of the second connect event, which uses the same key as the first connect, and fail its debug assertion. This change removes that debug assert on the assumption that it is okay for a peer to connect multiple times consecutively, and that the correct behavior in that case is to just cancel the first connection and continue as normal. --- zebra-network/src/peer_set/set.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/zebra-network/src/peer_set/set.rs b/zebra-network/src/peer_set/set.rs index a7e2c49f..f8fbd554 100644 --- a/zebra-network/src/peer_set/set.rs +++ b/zebra-network/src/peer_set/set.rs @@ -152,7 +152,7 @@ where Some(j) => Some(j), // We swapped an unrelated service. }; // No Heisenservices: they must be ready or unready. - debug_assert!(!self.cancel_handles.contains_key(key)); + assert!(!self.cancel_handles.contains_key(key)); } else if let Some(handle) = self.cancel_handles.remove(key) { let _ = handle.send(()); } @@ -200,18 +200,22 @@ where Poll::Ready(Some(Ok((key, svc)))) => { trace!(?key, "service became ready"); let _cancel = self.cancel_handles.remove(&key); - debug_assert!(_cancel.is_some(), "missing cancel handle"); + assert!(_cancel.is_some(), "missing cancel handle"); self.ready_services.insert(key, svc); } Poll::Ready(Some(Err((key, UnreadyError::Canceled)))) => { trace!(?key, "service was canceled"); - debug_assert!(!self.cancel_handles.contains_key(&key)) + // This debug assert is invalid because we can have a + // service be canceled due us connecting to the same service + // twice. + // + // assert!(!self.cancel_handles.contains_key(&key)) } Poll::Ready(Some(Err((key, UnreadyError::Inner(e))))) => { let error = e.into(); debug!(%error, "service failed while unready, dropped"); let _cancel = self.cancel_handles.remove(&key); - debug_assert!(_cancel.is_some(), "missing cancel handle"); + assert!(_cancel.is_some(), "missing cancel handle"); } } }