Fix network memory leaks #687
Conversation
There was a problem hiding this comment.
I would like to have a look again on this, but don't have many comments. Tried to think of how we could probably implement in a different way (seems to me that we do similar stuff in many places) but can't think of something right now. In the meanwhile could you please sync with @arun-koshy as he is looking to extra the worker addresses from committee? It seems that you touch some similar stuff.
| authority | ||
| .workers | ||
| .values() | ||
| .map(|address| &address.worker_to_worker), | ||
| ) | ||
| .chain( | ||
| authority | ||
| .workers | ||
| .values() | ||
| .map(|address| &address.primary_to_worker), |
There was a problem hiding this comment.
I see a possible conflict with @arun-koshy PR here #670 . You might want to coordinate on this?
| pub fn cleanup<'a, I>(&mut self, to_remove: I) | ||
| where | ||
| I: IntoIterator<Item = &'a Multiaddr>, | ||
| { | ||
| for address in to_remove { | ||
| self.clients.remove(address); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you please add tests for this behaviour?
There was a problem hiding this comment.
@huitseeker Do you think this function could be in a nice trait? It is used by 3 out of 4 of our network structs
There was a problem hiding this comment.
@asonnino The natural place to add it is the BaseNetwork trait.
There was a problem hiding this comment.
But not all networks can have this functions. To be precise, they all need it except WorkerToPrimary that only has a single client.
There was a problem hiding this comment.
Same comment on get_all_network_addresses, otherwise LGTM. Please link to #566, which you're closing.
| pub fn cleanup<'a, I>(&mut self, to_remove: I) | ||
| where | ||
| I: IntoIterator<Item = &'a Multiaddr>, | ||
| { | ||
| for address in to_remove { | ||
| self.clients.remove(address); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@asonnino The natural place to add it is the BaseNetwork trait.
config/src/lib.rs
Outdated
| /// Return the network addresses that are present in the current committee but that are absent | ||
| /// from the new committee (provided as argument). | ||
| pub fn network_diff<'a>(&'a self, new_committee: &'a Self) -> HashSet<&Multiaddr> { |
There was a problem hiding this comment.
Nit: new and old are concepts for the caller of this method. The typical way to present things in such a method is self and other (for the argument).
|
@asonnino You'll want to rebase as we have fixed your |
Facility to cleanup network
Facility to cleanup network
Facility to cleanup network
Facility to cleanup network
Fix memory leaks in the network stack when updating the committee (or changing epoch)