Bug? should we always remove inactive peers?

[josecelano]

I was reviewing this function:

/// 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
    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);
    }
}

If I'm not wrong, inactive peers are not removed unless remove_peerless_torrents is enabled.

If fact, the function comment does not match the code, remove_inactive_peers function is only called when remove_peerless_torrents is enabled.

Should not we always remove inactive peers whether we want to remove peerless torrents or not?

remove_peerless_torrents option depends on removing inactive peers. I think there is no way to remove a peer from a torrent but with the remove_inactive_peers function.

I guess the function should be:

/// Remove inactive peers and (optionally) peerless torrents
///
/// # Context: Tracker
pub fn cleanup_torrents(&self) {
    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);
    }
}

On the other hand, the name for the config option remove_peerless_torrents is a little bit misleading. The torrent is not removed even if it does not have peers but the stats are been persisted and that torrent has been downloaded at least once. It's an exception.

fn is_good(&self, policy: &TrackerPolicy) -> bool {
    if policy.persistent_torrent_completed_stat && self.downloaded > 0 {
        return true;
    }

    if policy.remove_peerless_torrents && self.swarm.is_empty() {
        return false;
    }

    true
}

cc @da2ce7