From 7b82050fd4ada0f0609e9012fb08cbd41cbe89c9 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 2 Jan 2024 14:33:52 +0000 Subject: [PATCH] refactor: [#262] split global consts for limits `crate::shared::bit_torrent::common::MAX_SCRAPE_TORRENTS` `MAX_SCRAPE_TORRENTS is the limit only for the number of torrents in a `scrape`request. `crate::core::TORRENT_PEERS_LIMIT` `TORRENT_PEERS_LIMIT` is not the limit for the number of peers in an announce request (UDP and HTTP tracker). Besides, the endpoint to get the torrent details in the API does not limit the number of peers for the torrent. So the API returns all peers in the tracker. This could lead to performance issues and we migth need to paginate results, but the API should either return all peers and paginate them in a new enpdoint. --- src/core/mod.rs | 21 +++++--- src/core/torrent/mod.rs | 79 +++++++++++++++++++------------ src/core/torrent/repository.rs | 10 ++-- src/servers/http/mod.rs | 4 +- src/servers/udp/handlers.rs | 12 ++--- src/shared/bit_torrent/common.rs | 1 - tests/servers/http/v1/contract.rs | 8 ++-- 7 files changed, 80 insertions(+), 55 deletions(-) diff --git a/src/core/mod.rs b/src/core/mod.rs index caac5b1ea..01dd295a2 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -458,6 +458,9 @@ use crate::core::databases::Database; use crate::core::torrent::{SwarmMetadata, SwarmStats}; use crate::shared::bit_torrent::info_hash::InfoHash; +/// The maximum number of returned peers for a torrent. +pub const TORRENT_PEERS_LIMIT: usize = 74; + /// The domain layer tracker service. /// /// Its main responsibility is to handle the `announce` and `scrape` requests. @@ -623,7 +626,7 @@ impl Tracker { let swarm_stats = self.update_torrent_with_peer_and_get_stats(info_hash, peer).await; - let peers = self.get_peers_for_peer(info_hash, peer).await; + let peers = self.get_torrent_peers_for_peer(info_hash, peer).await; AnnounceData { peers, @@ -692,24 +695,28 @@ impl Tracker { Ok(()) } - async fn get_peers_for_peer(&self, info_hash: &InfoHash, peer: &Peer) -> Vec { + async fn get_torrent_peers_for_peer(&self, info_hash: &InfoHash, peer: &Peer) -> Vec { let read_lock = self.torrents.get_torrents().await; match read_lock.get(info_hash) { None => vec![], - Some(entry) => entry.get_peers_for_peer(peer).into_iter().copied().collect(), + Some(entry) => entry + .get_peers_for_peer(peer, TORRENT_PEERS_LIMIT) + .into_iter() + .copied() + .collect(), } } /// # Context: Tracker /// /// Get all torrent peers for a given torrent - pub async fn get_all_torrent_peers(&self, info_hash: &InfoHash) -> Vec { + pub async fn get_torrent_peers(&self, info_hash: &InfoHash) -> Vec { let read_lock = self.torrents.get_torrents().await; match read_lock.get(info_hash) { None => vec![], - Some(entry) => entry.get_all_peers().into_iter().copied().collect(), + Some(entry) => entry.get_peers(TORRENT_PEERS_LIMIT).into_iter().copied().collect(), } } @@ -1253,7 +1260,7 @@ mod tests { tracker.update_torrent_with_peer_and_get_stats(&info_hash, &peer).await; - let peers = tracker.get_all_torrent_peers(&info_hash).await; + let peers = tracker.get_torrent_peers(&info_hash).await; assert_eq!(peers, vec![peer]); } @@ -1267,7 +1274,7 @@ mod tests { tracker.update_torrent_with_peer_and_get_stats(&info_hash, &peer).await; - let peers = tracker.get_peers_for_peer(&info_hash, &peer).await; + let peers = tracker.get_torrent_peers_for_peer(&info_hash, &peer).await; assert_eq!(peers, vec![]); } diff --git a/src/core/torrent/mod.rs b/src/core/torrent/mod.rs index a49e218a9..5edbf0aea 100644 --- a/src/core/torrent/mod.rs +++ b/src/core/torrent/mod.rs @@ -36,7 +36,6 @@ use aquatic_udp_protocol::AnnounceEvent; use serde::{Deserialize, Serialize}; use super::peer::{self, Peer}; -use crate::shared::bit_torrent::common::MAX_SCRAPE_TORRENTS; use crate::shared::clock::{Current, TimeNow}; /// A data structure containing all the information about a torrent in the tracker. @@ -100,7 +99,7 @@ impl Entry { /// /// The number of peers that have complete downloading is synchronously updated when peers are updated. /// That's the total torrent downloads counter. - pub fn update_peer(&mut self, peer: &peer::Peer) -> bool { + pub fn insert_or_update_peer(&mut self, peer: &peer::Peer) -> bool { let mut did_torrent_stats_change: bool = false; match peer.event { @@ -123,26 +122,44 @@ impl Entry { did_torrent_stats_change } - /// Get all swarm peers, limiting the result to the maximum number of scrape - /// torrents. + /// Get all swarm peers. #[must_use] pub fn get_all_peers(&self) -> Vec<&peer::Peer> { - self.peers.values().take(MAX_SCRAPE_TORRENTS as usize).collect() + self.peers.values().collect() + } + + /// Get swarm peers, limiting the result. + #[must_use] + pub fn get_peers(&self, limit: usize) -> Vec<&peer::Peer> { + self.peers.values().take(limit).collect() + } + + /// It returns the list of peers for a given peer client. + /// + /// It filters out the input peer, typically because we want to return this + /// list of peers to that client peer. + #[must_use] + pub fn get_all_peers_for_peer(&self, client: &Peer) -> Vec<&peer::Peer> { + self.peers + .values() + // Take peers which are not the client peer + .filter(|peer| peer.peer_addr != client.peer_addr) + .collect() } /// It returns the list of peers for a given peer client, limiting the - /// result to the maximum number of scrape torrents. + /// result. /// /// It filters out the input peer, typically because we want to return this /// list of peers to that client peer. #[must_use] - pub fn get_peers_for_peer(&self, client: &Peer) -> Vec<&peer::Peer> { + pub fn get_peers_for_peer(&self, client: &Peer, limit: usize) -> Vec<&peer::Peer> { self.peers .values() // Take peers which are not the client peer .filter(|peer| peer.peer_addr != client.peer_addr) // Limit the number of peers on the result - .take(MAX_SCRAPE_TORRENTS as usize) + .take(limit) .collect() } @@ -197,6 +214,8 @@ mod tests { use crate::core::torrent::Entry; use crate::shared::clock::{Current, DurationSinceUnixEpoch, Stopped, StoppedTime, Time, Working}; + use crate::core::TORRENT_PEERS_LIMIT; + struct TorrentPeerBuilder { peer: peer::Peer, } @@ -275,7 +294,7 @@ mod tests { let mut torrent_entry = Entry::new(); let torrent_peer = TorrentPeerBuilder::default().into(); - torrent_entry.update_peer(&torrent_peer); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add the peer assert_eq!(*torrent_entry.get_all_peers()[0], torrent_peer); assert_eq!(torrent_entry.get_all_peers().len(), 1); @@ -286,7 +305,7 @@ mod tests { let mut torrent_entry = Entry::new(); let torrent_peer = TorrentPeerBuilder::default().into(); - torrent_entry.update_peer(&torrent_peer); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add the peer assert_eq!(torrent_entry.get_all_peers(), vec![&torrent_peer]); } @@ -295,10 +314,10 @@ mod tests { fn a_peer_can_be_updated_in_a_torrent_entry() { let mut torrent_entry = Entry::new(); let mut torrent_peer = TorrentPeerBuilder::default().into(); - torrent_entry.update_peer(&torrent_peer); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add the peer torrent_peer.event = AnnounceEvent::Completed; // Update the peer - torrent_entry.update_peer(&torrent_peer); // Update the peer in the torrent entry + torrent_entry.insert_or_update_peer(&torrent_peer); // Update the peer in the torrent entry assert_eq!(torrent_entry.get_all_peers()[0].event, AnnounceEvent::Completed); } @@ -307,10 +326,10 @@ mod tests { fn a_peer_should_be_removed_from_a_torrent_entry_when_the_peer_announces_it_has_stopped() { let mut torrent_entry = Entry::new(); let mut torrent_peer = TorrentPeerBuilder::default().into(); - torrent_entry.update_peer(&torrent_peer); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add the peer torrent_peer.event = AnnounceEvent::Stopped; // Update the peer - torrent_entry.update_peer(&torrent_peer); // Update the peer in the torrent entry + torrent_entry.insert_or_update_peer(&torrent_peer); // Update the peer in the torrent entry assert_eq!(torrent_entry.get_all_peers().len(), 0); } @@ -320,10 +339,10 @@ mod tests { let mut torrent_entry = Entry::new(); let mut torrent_peer = TorrentPeerBuilder::default().into(); - torrent_entry.update_peer(&torrent_peer); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add the peer torrent_peer.event = AnnounceEvent::Completed; // Update the peer - let stats_have_changed = torrent_entry.update_peer(&torrent_peer); // Update the peer in the torrent entry + let stats_have_changed = torrent_entry.insert_or_update_peer(&torrent_peer); // Update the peer in the torrent entry assert!(stats_have_changed); } @@ -335,7 +354,7 @@ mod tests { let torrent_peer_announcing_complete_event = TorrentPeerBuilder::default().with_event_completed().into(); // Add a peer that did not exist before in the entry - let torrent_stats_have_not_changed = !torrent_entry.update_peer(&torrent_peer_announcing_complete_event); + let torrent_stats_have_not_changed = !torrent_entry.insert_or_update_peer(&torrent_peer_announcing_complete_event); assert!(torrent_stats_have_not_changed); } @@ -346,10 +365,10 @@ mod tests { let mut torrent_entry = Entry::new(); let peer_socket_address = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(127, 0, 0, 1)), 8080); let torrent_peer = TorrentPeerBuilder::default().with_peer_address(peer_socket_address).into(); - torrent_entry.update_peer(&torrent_peer); // Add peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add peer // Get peers excluding the one we have just added - let peers = torrent_entry.get_peers_for_peer(&torrent_peer); + let peers = torrent_entry.get_all_peers_for_peer(&torrent_peer); assert_eq!(peers.len(), 0); } @@ -364,16 +383,16 @@ mod tests { let torrent_peer_1 = TorrentPeerBuilder::default() .with_peer_address(SocketAddr::new(peer_ip, 8080)) .into(); - torrent_entry.update_peer(&torrent_peer_1); + torrent_entry.insert_or_update_peer(&torrent_peer_1); // Add peer 2 let torrent_peer_2 = TorrentPeerBuilder::default() .with_peer_address(SocketAddr::new(peer_ip, 8081)) .into(); - torrent_entry.update_peer(&torrent_peer_2); + torrent_entry.insert_or_update_peer(&torrent_peer_2); // Get peers for peer 1 - let peers = torrent_entry.get_peers_for_peer(&torrent_peer_1); + let peers = torrent_entry.get_all_peers_for_peer(&torrent_peer_1); // The peer 2 using the same IP but different port should be included assert_eq!(peers[0].peer_addr.ip(), Ipv4Addr::new(127, 0, 0, 1)); @@ -397,10 +416,10 @@ mod tests { let torrent_peer = TorrentPeerBuilder::default() .with_peer_id(peer_id_from_i32(peer_number)) .into(); - torrent_entry.update_peer(&torrent_peer); + torrent_entry.insert_or_update_peer(&torrent_peer); } - let peers = torrent_entry.get_all_peers(); + let peers = torrent_entry.get_peers(TORRENT_PEERS_LIMIT); assert_eq!(peers.len(), 74); } @@ -410,7 +429,7 @@ mod tests { let mut torrent_entry = Entry::new(); let torrent_seeder = a_torrent_seeder(); - torrent_entry.update_peer(&torrent_seeder); // Add seeder + torrent_entry.insert_or_update_peer(&torrent_seeder); // Add seeder assert_eq!(torrent_entry.get_stats().0, 1); } @@ -420,7 +439,7 @@ mod tests { let mut torrent_entry = Entry::new(); let torrent_leecher = a_torrent_leecher(); - torrent_entry.update_peer(&torrent_leecher); // Add leecher + torrent_entry.insert_or_update_peer(&torrent_leecher); // Add leecher assert_eq!(torrent_entry.get_stats().2, 1); } @@ -430,11 +449,11 @@ mod tests { ) { let mut torrent_entry = Entry::new(); let mut torrent_peer = TorrentPeerBuilder::default().into(); - torrent_entry.update_peer(&torrent_peer); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Add the peer // Announce "Completed" torrent download event. torrent_peer.event = AnnounceEvent::Completed; - torrent_entry.update_peer(&torrent_peer); // Update the peer + torrent_entry.insert_or_update_peer(&torrent_peer); // Update the peer let number_of_previously_known_peers_with_completed_torrent = torrent_entry.get_stats().1; @@ -448,7 +467,7 @@ mod tests { // Announce "Completed" torrent download event. // It's the first event announced from this peer. - torrent_entry.update_peer(&torrent_peer_announcing_complete_event); // Add the peer + torrent_entry.insert_or_update_peer(&torrent_peer_announcing_complete_event); // Add the peer let number_of_peers_with_completed_torrent = torrent_entry.get_stats().1; @@ -468,7 +487,7 @@ mod tests { let inactive_peer = TorrentPeerBuilder::default() .updated_at(timeout_seconds_before_now.sub(Duration::from_secs(1))) .into(); - torrent_entry.update_peer(&inactive_peer); // Add the peer + torrent_entry.insert_or_update_peer(&inactive_peer); // Add the peer torrent_entry.remove_inactive_peers(timeout); diff --git a/src/core/torrent/repository.rs b/src/core/torrent/repository.rs index 62df9b510..ac3d03054 100644 --- a/src/core/torrent/repository.rs +++ b/src/core/torrent/repository.rs @@ -69,7 +69,7 @@ impl Repository for Sync { let (stats, stats_updated) = { let mut torrent_entry_lock = torrent_entry.lock().unwrap(); - let stats_updated = torrent_entry_lock.update_peer(peer); + let stats_updated = torrent_entry_lock.insert_or_update_peer(peer); let stats = torrent_entry_lock.get_stats(); (stats, stats_updated) @@ -126,7 +126,7 @@ impl Repository for SyncSingle { std::collections::btree_map::Entry::Occupied(entry) => entry.into_mut(), }; - let stats_updated = torrent_entry.update_peer(peer); + let stats_updated = torrent_entry.insert_or_update_peer(peer); let stats = torrent_entry.get_stats(); ( @@ -168,7 +168,7 @@ impl TRepositoryAsync for RepositoryAsync { let (stats, stats_updated) = { let mut torrent_entry_lock = torrent_entry.lock().await; - let stats_updated = torrent_entry_lock.update_peer(peer); + let stats_updated = torrent_entry_lock.insert_or_update_peer(peer); let stats = torrent_entry_lock.get_stats(); (stats, stats_updated) @@ -226,7 +226,7 @@ impl TRepositoryAsync for AsyncSync { let (stats, stats_updated) = { let mut torrent_entry_lock = torrent_entry.lock().unwrap(); - let stats_updated = torrent_entry_lock.update_peer(peer); + let stats_updated = torrent_entry_lock.insert_or_update_peer(peer); let stats = torrent_entry_lock.get_stats(); (stats, stats_updated) @@ -273,7 +273,7 @@ impl TRepositoryAsync for RepositoryAsyncSingle { let (stats, stats_updated) = { let mut torrents_lock = self.torrents.write().await; let torrent_entry = torrents_lock.entry(*info_hash).or_insert(Entry::new()); - let stats_updated = torrent_entry.update_peer(peer); + let stats_updated = torrent_entry.insert_or_update_peer(peer); let stats = torrent_entry.get_stats(); (stats, stats_updated) diff --git a/src/servers/http/mod.rs b/src/servers/http/mod.rs index 10666d8a5..b2d232fc6 100644 --- a/src/servers/http/mod.rs +++ b/src/servers/http/mod.rs @@ -71,7 +71,7 @@ //! is behind a reverse proxy. //! //! > **NOTICE**: the maximum number of peers that the tracker can return is -//! `74`. Defined with a hardcoded const [`MAX_SCRAPE_TORRENTS`](crate::shared::bit_torrent::common::MAX_SCRAPE_TORRENTS). +//! `74`. Defined with a hardcoded const [`TORRENT_PEERS_LIMIT`](crate::core::TORRENT_PEERS_LIMIT). //! Refer to [issue 262](https://github.com/torrust/torrust-tracker/issues/262) //! for more information about this limitation. //! @@ -237,7 +237,7 @@ //! In order to scrape multiple torrents at the same time you can pass multiple //! `info_hash` parameters: `info_hash=%81%00%0...00%00%00&info_hash=%82%00%0...00%00%00` //! -//! > **NOTICE**: the maximum number of torrent you can scrape at the same time +//! > **NOTICE**: the maximum number of torrents you can scrape at the same time //! is `74`. Defined with a hardcoded const [`MAX_SCRAPE_TORRENTS`](crate::shared::bit_torrent::common::MAX_SCRAPE_TORRENTS). //! //! **Sample response** diff --git a/src/servers/udp/handlers.rs b/src/servers/udp/handlers.rs index 39a077466..a1461a457 100644 --- a/src/servers/udp/handlers.rs +++ b/src/servers/udp/handlers.rs @@ -598,7 +598,7 @@ mod tests { handle_announce(remote_addr, &request, &tracker).await.unwrap(); - let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await; + let peers = tracker.get_torrent_peers(&info_hash.0.into()).await; let expected_peer = TorrentPeerBuilder::default() .with_peer_id(peer::Id(peer_id.0)) @@ -659,7 +659,7 @@ mod tests { handle_announce(remote_addr, &request, &tracker).await.unwrap(); - let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await; + let peers = tracker.get_torrent_peers(&info_hash.0.into()).await; assert_eq!(peers[0].peer_addr, SocketAddr::new(IpAddr::V4(remote_client_ip), client_port)); } @@ -763,7 +763,7 @@ mod tests { handle_announce(remote_addr, &request, &tracker).await.unwrap(); - let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await; + let peers = tracker.get_torrent_peers(&info_hash.0.into()).await; let external_ip_in_tracker_configuration = tracker.config.external_ip.clone().unwrap().parse::().unwrap(); @@ -820,7 +820,7 @@ mod tests { handle_announce(remote_addr, &request, &tracker).await.unwrap(); - let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await; + let peers = tracker.get_torrent_peers(&info_hash.0.into()).await; let expected_peer = TorrentPeerBuilder::default() .with_peer_id(peer::Id(peer_id.0)) @@ -884,7 +884,7 @@ mod tests { handle_announce(remote_addr, &request, &tracker).await.unwrap(); - let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await; + let peers = tracker.get_torrent_peers(&info_hash.0.into()).await; // When using IPv6 the tracker converts the remote client ip into a IPv4 address assert_eq!(peers[0].peer_addr, SocketAddr::new(IpAddr::V6(remote_client_ip), client_port)); @@ -1001,7 +1001,7 @@ mod tests { handle_announce(remote_addr, &request, &tracker).await.unwrap(); - let peers = tracker.get_all_torrent_peers(&info_hash.0.into()).await; + let peers = tracker.get_torrent_peers(&info_hash.0.into()).await; let _external_ip_in_tracker_configuration = tracker.config.external_ip.clone().unwrap().parse::().unwrap(); diff --git a/src/shared/bit_torrent/common.rs b/src/shared/bit_torrent/common.rs index 0ce345a3e..9bf9dfd3c 100644 --- a/src/shared/bit_torrent/common.rs +++ b/src/shared/bit_torrent/common.rs @@ -5,7 +5,6 @@ use aquatic_udp_protocol::{AnnounceEvent, NumberOfBytes}; use serde::{Deserialize, Serialize}; /// The maximum number of torrents that can be returned in an `scrape` response. -/// It's also the maximum number of peers returned in an `announce` response. /// /// The [BEP 15. UDP Tracker Protocol for `BitTorrent`](https://www.bittorrent.org/beps/bep_0015.html) /// defines this limit: diff --git a/tests/servers/http/v1/contract.rs b/tests/servers/http/v1/contract.rs index d7f4d50cc..9a6aa2454 100644 --- a/tests/servers/http/v1/contract.rs +++ b/tests/servers/http/v1/contract.rs @@ -740,7 +740,7 @@ mod for_all_config_modes { client.announce(&announce_query).await; - let peers = test_env.tracker.get_all_torrent_peers(&info_hash).await; + let peers = test_env.tracker.get_torrent_peers(&info_hash).await; let peer_addr = peers[0].peer_addr; assert_eq!(peer_addr.ip(), client_ip); @@ -776,7 +776,7 @@ mod for_all_config_modes { client.announce(&announce_query).await; - let peers = test_env.tracker.get_all_torrent_peers(&info_hash).await; + let peers = test_env.tracker.get_torrent_peers(&info_hash).await; let peer_addr = peers[0].peer_addr; assert_eq!(peer_addr.ip(), test_env.tracker.config.get_ext_ip().unwrap()); @@ -812,7 +812,7 @@ mod for_all_config_modes { client.announce(&announce_query).await; - let peers = test_env.tracker.get_all_torrent_peers(&info_hash).await; + let peers = test_env.tracker.get_torrent_peers(&info_hash).await; let peer_addr = peers[0].peer_addr; assert_eq!(peer_addr.ip(), test_env.tracker.config.get_ext_ip().unwrap()); @@ -846,7 +846,7 @@ mod for_all_config_modes { ) .await; - let peers = test_env.tracker.get_all_torrent_peers(&info_hash).await; + let peers = test_env.tracker.get_torrent_peers(&info_hash).await; let peer_addr = peers[0].peer_addr; assert_eq!(peer_addr.ip(), IpAddr::from_str("150.172.238.178").unwrap());