Skip to content

Commit

Permalink
fix mem-leak on topic peers
Browse files Browse the repository at this point in the history
Signed-off-by: onur-ozkan <[email protected]>
  • Loading branch information
onur-ozkan committed Dec 17, 2024
1 parent 6fc061b commit 15b30da
Showing 1 changed file with 15 additions and 5 deletions.
20 changes: 15 additions & 5 deletions protocols/gossipsub/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@

use std::{
cmp::{max, Ordering},
collections::HashSet,
collections::{hash_map::Entry, VecDeque},
collections::{BTreeSet, HashMap},
collections::{hash_map::Entry, BTreeSet, HashMap, HashSet, VecDeque},
fmt,
net::IpAddr,
task::{Context, Poll},
Expand Down Expand Up @@ -2070,6 +2068,10 @@ where

// remove topic from the peer_topics mapping
subscribed_topics.remove(topic_hash);
if let Some(peers) = self.topic_peers.get_mut(topic_hash) {
peers.remove(propagation_source);

This comment has been minimized.

Copy link
@mariocynicys

mariocynicys Dec 17, 2024

i meant we still should remove the empty key if the propagation source was the last one, but just for this key topic_hash and not looping over all the topics with each unsubscribtion.

also looking at this now, looks like we already removed the propagatino_source from the topic_peers[topic_hash] a couple of lines above (L2061) (note that this peer_list variable is an entry reference to topic_peers[topoic_hash].

so we just need to remove the topic_peers[topic_hash] all together if no more peers are listening (emmpty vector).

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 18, 2024

Author Member

topic_peers[topic_hash] is not panic safe.

Ok, I am tired of this - Gonna make a complete review before making any further changes.

}

unsubscribed_peers.push((*propagation_source, topic_hash.clone()));
// generate an unsubscribe event to be polled
application_event.push(ToSwarm::GenerateEvent(Event::Unsubscribed {
Expand All @@ -2079,8 +2081,11 @@ where
}
}

if let Some(m) = self.metrics.as_mut() {
m.set_topic_peers(topic_hash, peer_list.len());
if let (Some(m), Some(len)) = (
self.metrics.as_mut(),
self.topic_peers.get(topic_hash).map(|set| set.len()),

This comment has been minimized.

Copy link
@mariocynicys

mariocynicys Dec 17, 2024

i would keep it as it was tbh, i.e. not prevent the set_topic_peers call if topic_peers is empty.
whoever is depending on the data from the metrics might be also interested in setting the topic peers count to zero when nobody is listening anymore. given that the thing is called metrics though, it's not that critical, so i will leave this up to your preference.

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 18, 2024

Author Member

I wouldn't be interested on metrics of the topic that no longer exists on the node. But sure, I want to fix the memory problem as quick as possible, I don't want to argue over this non-sense part.

) {
m.set_topic_peers(topic_hash, len);
}
}

Expand Down Expand Up @@ -3337,6 +3342,11 @@ where
// support the protocol.
self.peer_topics.remove(&peer_id);

self.topic_peers.retain(|_, peers| {
peers.remove(&peer_id);
!peers.is_empty()
});

// If metrics are enabled, register the disconnection of a peer based on its protocol.
if let Some(metrics) = self.metrics.as_mut() {
let peer_kind = &self
Expand Down

0 comments on commit 15b30da

Please sign in to comment.