Skip to content

Commit

Permalink
refactor: [#262] split global consts for limits
Browse files Browse the repository at this point in the history 
Current const: `crate::shared::bit_torrent::common::MAX_SCRAPE_TORRENTS`

`MAX_SCRAPE_TORRENTS` is the limit only for the number of torrents in a `scrape`request.

New const: `crate::core::TORRENT_PEERS_LIMIT`

`TORRENT_PEERS_LIMIT` is now 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 might need to paginate results, but the API should either return all peers and paginate them in a new endpoint.
  • Loading branch information
@josecelano
josecelano committed Jan 2, 2024
1 parent 2099789 commit 46e67a8
Show file tree
Hide file tree
Showing 7 changed files with 79 additions and 56 deletions.
21 changes: 14 additions & 7 deletions src/core/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -692,24 +695,28 @@ impl Tracker {
Ok(())
}

async fn get_peers_for_peer(&self, info_hash: &InfoHash, peer: &Peer) -> Vec<peer::Peer> {
async fn get_torrent_peers_for_peer(&self, info_hash: &InfoHash, peer: &Peer) -> Vec<peer::Peer> {
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<peer::Peer> {
pub async fn get_torrent_peers(&self, info_hash: &InfoHash) -> Vec<peer::Peer> {
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(),
}
}

Expand Down Expand Up @@ -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]);
}
Expand All @@ -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![]);
}
Expand Down
79 changes: 48 additions & 31 deletions src/core/torrent/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
}

Expand Down Expand Up @@ -193,8 +210,8 @@ mod tests {

use aquatic_udp_protocol::{AnnounceEvent, NumberOfBytes};

use crate::core::peer;
use crate::core::torrent::Entry;
use crate::core::{peer, TORRENT_PEERS_LIMIT};
use crate::shared::clock::{Current, DurationSinceUnixEpoch, Stopped, StoppedTime, Time, Working};

struct TorrentPeerBuilder {
Expand Down Expand Up @@ -275,7 +292,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);
Expand All @@ -286,7 +303,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]);
}
Expand All @@ -295,10 +312,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);
}
Expand All @@ -307,10 +324,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);
}
Expand All @@ -320,10 +337,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);
}
Expand All @@ -335,7 +352,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);
}
Expand All @@ -346,10 +363,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);
}
Expand All @@ -364,16 +381,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));
Expand All @@ -397,10 +414,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);
}
Expand All @@ -410,7 +427,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);
}
Expand All @@ -420,7 +437,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);
}
Expand All @@ -430,11 +447,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;

Expand All @@ -448,7 +465,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;

Expand All @@ -468,7 +485,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);

Expand Down
10 changes: 5 additions & 5 deletions src/core/torrent/repository.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Check warning on line 72 in src/core/torrent/repository.rs

View check run for this annotation

Codecov / codecov/patch

src/core/torrent/repository.rs#L72 

Added line #L72 was not covered by tests
let stats = torrent_entry_lock.get_stats();

(stats, stats_updated)
Expand Down Expand Up @@ -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);

Check warning on line 129 in src/core/torrent/repository.rs

View check run for this annotation

Codecov / codecov/patch

src/core/torrent/repository.rs#L129 

Added line #L129 was not covered by tests
let stats = torrent_entry.get_stats();

(
Expand Down Expand Up @@ -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);

Check warning on line 171 in src/core/torrent/repository.rs

View check run for this annotation

Codecov / codecov/patch

src/core/torrent/repository.rs#L171 

Added line #L171 was not covered by tests
let stats = torrent_entry_lock.get_stats();

(stats, stats_updated)
Expand Down Expand Up @@ -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);

Check warning on line 229 in src/core/torrent/repository.rs

View check run for this annotation

Codecov / codecov/patch

src/core/torrent/repository.rs#L229 

Added line #L229 was not covered by tests
let stats = torrent_entry_lock.get_stats();

(stats, stats_updated)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions src/servers/http/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//!
Expand Down Expand Up @@ -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**
Expand Down
Loading

0 comments on commit 46e67a8

Please sign in to comment.