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 e90c0f5
Showing 1 changed file with 18 additions and 3 deletions.
21 changes: 18 additions & 3 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,14 @@ where

// remove topic from the peer_topics mapping
subscribed_topics.remove(topic_hash);
self.topic_peers.retain(|th, peers| {
if th == topic_hash {

This comment has been minimized.

Copy link
@mariocynicys

mariocynicys Dec 17, 2024

since we lock on a single topic hash, why not select it first from the hashmap? self.topic_peers[topic_hash].remove(source)

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 17, 2024

Author Member

We also drop the empty ones.

This comment has been minimized.

Copy link
@mariocynicys

mariocynicys Dec 17, 2024

yeah i see it kind of serves as a periodic frequent clean up in a way, but i think the one from disconnection hook is enough for this clean up.

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 17, 2024

Author Member
peers.remove(propagation_source);
}

!peers.is_empty()
});

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

if let Some(m) = self.metrics.as_mut() {
let peer_list = self
.topic_peers
.entry(topic_hash.clone())

This comment has been minimized.

Copy link
@mariocynicys

mariocynicys Dec 17, 2024

i guess u did this cuz the retain call above forced us to drop the peer_list (hard to reason with no LSP xD). we here shouldn't use the entry interface nor insert any value though?: self.topic_peers.get(&topic_hash).unwrap_or_default()?

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 17, 2024

Author Member

This was done due to borrowing rules. It could be done in somewhat better way, but I am not sure if we ever enable metrics tbh.

This comment has been minimized.

Copy link
@mariocynicys

mariocynicys Dec 17, 2024

that's what i thought.
self.topic_peers.get(&topic_hash).unwrap_or_default().len(); should do it then.
also note that this kind of defies the memory leak fix since it adds unnecessary memory (albeit it gets removed on disconnection hook since it cleans up the whole hashmap).

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 17, 2024

Author Member
.or_insert_with(Default::default);

This comment has been minimized.

Copy link
@laruh

laruh Dec 17, 2024

Member

Do we really need to update the metrics for a new topic with no peers?
It might be unnecessary to set metrics for such empty topics, as they don't provide useful information

This comment has been minimized.

Copy link
@onur-ozkan

onur-ozkan Dec 17, 2024

Author Member
m.set_topic_peers(topic_hash, peer_list.len());
}
}
Expand Down Expand Up @@ -3337,6 +3347,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 e90c0f5

Please sign in to comment.