Merge #1003: Fix bug: the tracker should always remove inactive peers

3fbab31 refactor: [#1002] rename is_good fn to meets_retaining_policy (Jose Celano)
1766587 feat: [#1002] remove inactive peers always (Jose Celano)

Pull request description:

  **Fix bug:** the tracker should always remove inactive peers even when the config option `core.tracker_policy.remove_peerless_torrents` is disabled.

  **Refactor:** rename the `is_good` function to `meets_retaining_policy` to be more specific.

ACKs for top commit:
  josecelano:
    ACK 3fbab31

Tree-SHA512: 765124c224e5328c3820417e7b1d53bcac4171bb5b7afb26c9c6bcf26c9792166502dde8381b22396924531bfe5f2b213f6be58ac20548fee3c872231861649e

packages/torrent-repository/src/entry/mod.rs

@@ -22,7 +22,7 @@ pub trait Entry {
     fn get_swarm_metadata(&self) -> SwarmMetadata;
 
     /// Returns True if Still a Valid Entry according to the Tracker Policy
-    fn is_good(&self, policy: &TrackerPolicy) -> bool;
+    fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool;
 
     /// Returns True if the Peers is Empty
     fn peers_is_empty(&self) -> bool;
@@ -53,7 +53,7 @@ pub trait Entry {
 #[allow(clippy::module_name_repetitions)]
 pub trait EntrySync {
     fn get_swarm_metadata(&self) -> SwarmMetadata;
-    fn is_good(&self, policy: &TrackerPolicy) -> bool;
+    fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool;
     fn peers_is_empty(&self) -> bool;
     fn get_peers_len(&self) -> usize;
     fn get_peers(&self, limit: Option<usize>) -> Vec<Arc<peer::Peer>>;
@@ -65,7 +65,7 @@ pub trait EntrySync {
 #[allow(clippy::module_name_repetitions)]
 pub trait EntryAsync {
     fn get_swarm_metadata(&self) -> impl std::future::Future<Output = SwarmMetadata> + Send;
-    fn check_good(self, policy: &TrackerPolicy) -> impl std::future::Future<Output = bool> + Send;
+    fn meets_retaining_policy(self, policy: &TrackerPolicy) -> impl std::future::Future<Output = bool> + Send;
     fn peers_is_empty(&self) -> impl std::future::Future<Output = bool> + Send;
     fn get_peers_len(&self) -> impl std::future::Future<Output = usize> + Send;
     fn get_peers(&self, limit: Option<usize>) -> impl std::future::Future<Output = Vec<Arc<peer::Peer>>> + Send;

packages/torrent-repository/src/entry/mutex_parking_lot.rs

@@ -13,8 +13,8 @@ impl EntrySync for EntryMutexParkingLot {
         self.lock().get_swarm_metadata()
     }
 
-    fn is_good(&self, policy: &TrackerPolicy) -> bool {
-        self.lock().is_good(policy)
+    fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool {
+        self.lock().meets_retaining_policy(policy)
     }
 
     fn peers_is_empty(&self) -> bool {

packages/torrent-repository/src/entry/mutex_std.rs

@@ -13,8 +13,8 @@ impl EntrySync for EntryMutexStd {
         self.lock().expect("it should get a lock").get_swarm_metadata()
     }
 
-    fn is_good(&self, policy: &TrackerPolicy) -> bool {
-        self.lock().expect("it should get a lock").is_good(policy)
+    fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool {
+        self.lock().expect("it should get a lock").meets_retaining_policy(policy)
     }
 
     fn peers_is_empty(&self) -> bool {

packages/torrent-repository/src/entry/mutex_tokio.rs

@@ -13,8 +13,8 @@ impl EntryAsync for EntryMutexTokio {
         self.lock().await.get_swarm_metadata()
     }
 
-    async fn check_good(self, policy: &TrackerPolicy) -> bool {
-        self.lock().await.is_good(policy)
+    async fn meets_retaining_policy(self, policy: &TrackerPolicy) -> bool {
+        self.lock().await.meets_retaining_policy(policy)
     }
 
     async fn peers_is_empty(&self) -> bool {

packages/torrent-repository/src/entry/rw_lock_parking_lot.rs

@@ -13,8 +13,8 @@ impl EntrySync for EntryRwLockParkingLot {
         self.read().get_swarm_metadata()
     }
 
-    fn is_good(&self, policy: &TrackerPolicy) -> bool {
-        self.read().is_good(policy)
+    fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool {
+        self.read().meets_retaining_policy(policy)
     }
 
     fn peers_is_empty(&self) -> bool {

packages/torrent-repository/src/entry/single.rs

@@ -22,7 +22,7 @@ impl Entry for EntrySingle {
         }
     }
 
-    fn is_good(&self, policy: &TrackerPolicy) -> bool {
+    fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool {
         if policy.persistent_torrent_completed_stat && self.downloaded > 0 {
             return true;
         }

packages/torrent-repository/src/repository/dash_map_mutex_std.rs

@@ -103,6 +103,6 @@ where
     }
 
     fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
-        self.torrents.retain(|_, entry| entry.is_good(policy));
+        self.torrents.retain(|_, entry| entry.meets_retaining_policy(policy));
     }
 }

packages/torrent-repository/src/repository/rw_lock_std.rs

@@ -126,6 +126,6 @@ where
     fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         let mut db = self.get_torrents_mut();
 
-        db.retain(|_, e| e.is_good(policy));
+        db.retain(|_, e| e.meets_retaining_policy(policy));
     }
 }

packages/torrent-repository/src/repository/rw_lock_std_mutex_std.rs

@@ -124,6 +124,6 @@ where
     fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         let mut db = self.get_torrents_mut();
 
-        db.retain(|_, e| e.lock().expect("it should lock entry").is_good(policy));
+        db.retain(|_, e| e.lock().expect("it should lock entry").meets_retaining_policy(policy));
     }
 }

packages/torrent-repository/src/repository/rw_lock_std_mutex_tokio.rs

@@ -143,8 +143,8 @@ where
             handles = zip(db.keys().copied(), db.values().cloned())
                 .map(|(infohash, torrent)| {
                     torrent
-                        .check_good(policy)
-                        .map(move |good| if good { None } else { Some(infohash) })
+                        .meets_retaining_policy(policy)
+                        .map(move |should_be_retained| if should_be_retained { None } else { Some(infohash) })
                         .boxed()
                 })
                 .collect::<Vec<_>>();

packages/torrent-repository/src/repository/rw_lock_tokio.rs

@@ -130,6 +130,6 @@ where
     async fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         let mut db = self.get_torrents_mut().await;
 
-        db.retain(|_, e| e.is_good(policy));
+        db.retain(|_, e| e.meets_retaining_policy(policy));
     }
 }

packages/torrent-repository/src/repository/rw_lock_tokio_mutex_std.rs

@@ -124,6 +124,6 @@ where
     async fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         let mut db = self.get_torrents_mut().await;
 
-        db.retain(|_, e| e.lock().expect("it should lock entry").is_good(policy));
+        db.retain(|_, e| e.lock().expect("it should lock entry").meets_retaining_policy(policy));
     }
 }

packages/torrent-repository/src/repository/rw_lock_tokio_mutex_tokio.rs

@@ -130,7 +130,7 @@ where
         let mut not_good = Vec::<InfoHash>::default();
 
         for (&infohash, torrent) in db.iter() {
-            if !torrent.clone().check_good(policy).await {
+            if !torrent.clone().meets_retaining_policy(policy).await {
                 not_good.push(infohash);
             }
         }

packages/torrent-repository/src/repository/skip_map_mutex_std.rs

@@ -100,7 +100,7 @@ where
 
     fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         for entry in &self.torrents {
-            if entry.value().is_good(policy) {
+            if entry.value().meets_retaining_policy(policy) {
                 continue;
             }
 
@@ -191,7 +191,7 @@ where
 
     fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         for entry in &self.torrents {
-            if entry.value().is_good(policy) {
+            if entry.value().meets_retaining_policy(policy) {
                 continue;
             }
 
@@ -282,7 +282,7 @@ where
 
     fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
         for entry in &self.torrents {
-            if entry.value().is_good(policy) {
+            if entry.value().meets_retaining_policy(policy) {
                 continue;
             }
 

packages/torrent-repository/tests/common/torrent.rs

@@ -29,13 +29,13 @@ impl Torrent {
         }
     }
 
-    pub(crate) async fn is_good(&self, policy: &TrackerPolicy) -> bool {
+    pub(crate) async fn meets_retaining_policy(&self, policy: &TrackerPolicy) -> bool {
         match self {
-            Torrent::Single(entry) => entry.is_good(policy),
-            Torrent::MutexStd(entry) => entry.is_good(policy),
-            Torrent::MutexTokio(entry) => entry.clone().check_good(policy).await,
-            Torrent::MutexParkingLot(entry) => entry.is_good(policy),
-            Torrent::RwLockParkingLot(entry) => entry.is_good(policy),
+            Torrent::Single(entry) => entry.meets_retaining_policy(policy),
+            Torrent::MutexStd(entry) => entry.meets_retaining_policy(policy),
+            Torrent::MutexTokio(entry) => entry.clone().meets_retaining_policy(policy).await,
+            Torrent::MutexParkingLot(entry) => entry.meets_retaining_policy(policy),
+            Torrent::RwLockParkingLot(entry) => entry.meets_retaining_policy(policy),
         }
     }
 

packages/torrent-repository/tests/entry/mod.rs

@@ -126,7 +126,7 @@ async fn it_should_be_empty_by_default(
 #[case::downloaded(&Makes::Downloaded)]
 #[case::three(&Makes::Three)]
 #[tokio::test]
-async fn it_should_check_if_entry_is_good(
+async fn it_should_check_if_entry_should_be_retained_based_on_the_tracker_policy(
     #[values(single(), mutex_std(), mutex_tokio(), mutex_parking_lot(), rw_lock_parking_lot())] mut torrent: Torrent,
     #[case] makes: &Makes,
     #[values(policy_none(), policy_persist(), policy_remove(), policy_remove_persist())] policy: TrackerPolicy,
@@ -141,19 +141,19 @@ async fn it_should_check_if_entry_is_good(
         (true, true) => match (has_peers, has_downloads) {
             // no peers, but has downloads
             // peers, with or without downloads
-            (false, true) | (true, true | false) => assert!(torrent.is_good(&policy).await),
+            (false, true) | (true, true | false) => assert!(torrent.meets_retaining_policy(&policy).await),
             // no peers and no downloads
-            (false, false) => assert!(!torrent.is_good(&policy).await),
+            (false, false) => assert!(!torrent.meets_retaining_policy(&policy).await),
         },
         // remove torrents without peers and drop completed download stats
         (true, false) => match (has_peers, has_downloads) {
             // peers, with or without downloads
-            (true, true | false) => assert!(torrent.is_good(&policy).await),
+            (true, true | false) => assert!(torrent.meets_retaining_policy(&policy).await),
             // no peers and with or without downloads
-            (false, true | false) => assert!(!torrent.is_good(&policy).await),
+            (false, true | false) => assert!(!torrent.meets_retaining_policy(&policy).await),
         },
         // keep torrents without peers, but keep or drop completed download stats
-        (false, true | false) => assert!(torrent.is_good(&policy).await),
+        (false, true | false) => assert!(torrent.meets_retaining_policy(&policy).await),
     }
 }
 

packages/torrent-repository/tests/repository/mod.rs

@@ -634,6 +634,6 @@ async fn it_should_remove_peerless_torrents(
     let torrents = repo.get_paginated(None).await;
 
     for (_, entry) in torrents {
-        assert!(entry.is_good(&policy));
+        assert!(entry.meets_retaining_policy(&policy));
     }
 }

src/core/mod.rs

@@ -778,18 +778,17 @@ impl Tracker {
         self.torrents.get_metrics()
     }
 
-    /// Remove inactive peers and (optionally) peerless torrents
+    /// Remove inactive peers and (optionally) peerless torrents.
     ///
     /// # Context: Tracker
     pub fn cleanup_torrents(&self) {
-        // If we don't need to remove torrents we will use the faster iter
+        let current_cutoff = CurrentClock::now_sub(&Duration::from_secs(u64::from(self.config.tracker_policy.max_peer_timeout)))
+            .unwrap_or_default();
+
+        self.torrents.remove_inactive_peers(current_cutoff);
+
         if self.config.tracker_policy.remove_peerless_torrents {
             self.torrents.remove_peerless_torrents(&self.config.tracker_policy);
-        } else {
-            let current_cutoff =
-                CurrentClock::now_sub(&Duration::from_secs(u64::from(self.config.tracker_policy.max_peer_timeout)))
-                    .unwrap_or_default();
-            self.torrents.remove_inactive_peers(current_cutoff);
         }
     }