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 <work@onurozkan.dev>
  • Loading branch information
@onur-ozkan
onur-ozkan committed Dec 17, 2024
1 parent 6fc061b commit e16e444
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions protocols/gossipsub/src/behaviour.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3337,6 +3337,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

2 comments on commit e16e444

@mariocynicys
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you explain how this change works?

will on_connection_closed get called when a peer disconnects from a (seed) peer or that's when a peer just unsubscribes from a topic.
if it's only on disconnection, then we still have the memory leak since peers can connect indefinitely. also what if two seed peers are connected to each other? they will never delete the topic, no?

@mariocynicys
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also i think we can use the output of self.peer_topics.remove(&peer_id) to speed up the removal operation in topic_peers instead of looping over the complete map (assuming that peer_topics is a peer -> topic map and topic_peers is it's inverse map, haven't loaded this lib locally :))

Please sign in to comment.