feat(ping): don't close connections upon failures

Previously, the `libp2p-ping` module came with a policy to close a connection after X failed pings. This is only one of many possible policies on how users would want to do connection management. We remove this policy without a replacement. If users wish to restore this functionality, they can easily implement such policy themselves: The default value of `max_failures` was 1. To restore the previous functionality users can simply close the connection upon the first received ping error. In this same patch, we also simplify the API of `ping::Event` by removing the layer of `ping::Success` and instead reporting the RTT to the peer directly. Related: #3591. Pull-Request: #3947.
libp2p · May 24, 2023 · 25bc30f · 25bc30f
1 parent a5cd0d0
commit 25bc30f
Show file tree

Hide file tree

Showing 12 changed files with 144 additions and 196 deletions.
diff --git a/examples/ipfs-private/src/main.rs b/examples/ipfs-private/src/main.rs
@@ -249,35 +249,33 @@ async fn main() -> Result<(), Box<dyn Error>> {
                         match event {
                             ping::Event {
                                 peer,
-                                result: Result::Ok(ping::Success::Ping { rtt }),
+                                result: Result::Ok(rtt),
+                                ..
                             } => {
                                 println!(
                                     "ping: rtt to {} is {} ms",
                                     peer.to_base58(),
                                     rtt.as_millis()
                                 );
                             }
-                            ping::Event {
-                                peer,
-                                result: Result::Ok(ping::Success::Pong),
-                            } => {
-                                println!("ping: pong from {}", peer.to_base58());
-                            }
                             ping::Event {
                                 peer,
                                 result: Result::Err(ping::Failure::Timeout),
+                                ..
                             } => {
                                 println!("ping: timeout to {}", peer.to_base58());
                             }
                             ping::Event {
                                 peer,
                                 result: Result::Err(ping::Failure::Unsupported),
+                                ..
                             } => {
                                 println!("ping: {} does not support ping protocol", peer.to_base58());
                             }
                             ping::Event {
                                 peer,
                                 result: Result::Err(ping::Failure::Other { error }),
+                                ..
                             } => {
                                 println!("ping: ping::Failure with {}: {error}", peer.to_base58());
                             }

diff --git a/examples/rendezvous/src/bin/rzv-discover.rs b/examples/rendezvous/src/bin/rzv-discover.rs
@@ -105,7 +105,8 @@ async fn main() {
                     }
                     SwarmEvent::Behaviour(MyBehaviourEvent::Ping(ping::Event {
                         peer,
-                        result: Ok(ping::Success::Ping { rtt }),
+                        result: Ok(rtt),
+                        ..
                     })) if peer != rendezvous_point => {
                         log::info!("Ping to {} is {}ms", peer, rtt.as_millis())
                     }

diff --git a/examples/rendezvous/src/bin/rzv-identify.rs b/examples/rendezvous/src/bin/rzv-identify.rs
@@ -106,7 +106,8 @@ async fn main() {
             }
             SwarmEvent::Behaviour(MyBehaviourEvent::Ping(ping::Event {
                 peer,
-                result: Ok(ping::Success::Ping { rtt }),
+                result: Ok(rtt),
+                ..
             })) if peer != rendezvous_point => {
                 log::info!("Ping to {} is {}ms", peer, rtt.as_millis())
             }

diff --git a/examples/rendezvous/src/bin/rzv-register.rs b/examples/rendezvous/src/bin/rzv-register.rs
@@ -104,7 +104,8 @@ async fn main() {
             }
             SwarmEvent::Behaviour(MyBehaviourEvent::Ping(ping::Event {
                 peer,
-                result: Ok(ping::Success::Ping { rtt }),
+                result: Ok(rtt),
+                ..
             })) if peer != rendezvous_point => {
                 log::info!("Ping to {} is {}ms", peer, rtt.as_millis())
             }

diff --git a/interop-tests/src/bin/ping.rs b/interop-tests/src/bin/ping.rs
@@ -137,8 +137,8 @@ async fn main() -> Result<()> {
 
         let rtt = loop {
             if let Some(SwarmEvent::Behaviour(BehaviourEvent::Ping(ping::Event {
-                peer: _,
-                result: Ok(ping::Success::Ping { rtt }),
+                result: Ok(rtt),
+                ..
             }))) = swarm.next().await
             {
                 log::info!("Ping successful: {rtt:?}");

diff --git a/misc/metrics/CHANGELOG.md b/misc/metrics/CHANGELOG.md
@@ -26,9 +26,13 @@
   Note that you can use the `_count` metric of the `Histogram` as a replacement for the `Counter`.
   See [PR 3927].
 
+- Remove the `pong_received` counter because it is no longer exposed by `libp2p-ping`.
+  See [PR 3947].
+
 [PR 3715]: https://github.com/libp2p/rust-libp2p/pull/3715
 [PR 3927]: https://github.com/libp2p/rust-libp2p/pull/3927
 [PR 3325]: https://github.com/libp2p/rust-libp2p/pull/3325
+[PR 3947]: https://github.com/libp2p/rust-libp2p/pull/3947
 
 ## 0.12.0
 

diff --git a/misc/metrics/src/ping.rs b/misc/metrics/src/ping.rs
@@ -55,7 +55,6 @@ enum Failure {
 pub(crate) struct Metrics {
     rtt: Histogram,
     failure: Family<FailureLabels, Counter>,
-    pong_received: Counter,
 }
 
 impl Metrics {
@@ -77,28 +76,14 @@ impl Metrics {
             failure.clone(),
         );
 
-        let pong_received = Counter::default();
-        sub_registry.register(
-            "pong_received",
-            "Number of 'pong's received",
-            pong_received.clone(),
-        );
-
-        Self {
-            rtt,
-            failure,
-            pong_received,
-        }
+        Self { rtt, failure }
     }
 }
 
 impl super::Recorder<libp2p_ping::Event> for Metrics {
     fn record(&self, event: &libp2p_ping::Event) {
         match &event.result {
-            Ok(libp2p_ping::Success::Pong) => {
-                self.pong_received.inc();
-            }
-            Ok(libp2p_ping::Success::Ping { rtt }) => {
+            Ok(rtt) => {
                 self.rtt.observe(rtt.as_secs_f64());
             }
             Err(failure) => {

diff --git a/protocols/ping/CHANGELOG.md b/protocols/ping/CHANGELOG.md
@@ -2,10 +2,17 @@
 
 - Raise MSRV to 1.65.
   See [PR 3715].
+
 - Remove deprecated items. See [PR 3702].
 
+- Don't close connections on ping failures.
+  To restore the previous behaviour, users should call `Swarm::close_connection` upon receiving a `ping::Event` with a `ping::Failure`.
+  This also removes the `max_failures` config option.
+  See [PR 3947].
+
 [PR 3715]: https://github.com/libp2p/rust-libp2p/pull/3715
 [PR 3702]: https://github.com/libp2p/rust-libp2p/pull/3702
+[PR 3947]: https://github.com/libp2p/rust-libp2p/pull/3947
 
 ## 0.42.0