Conversation
Shared::receiver() inserts an mpsc::Sender into the senders map for every established connection (inbound and outbound), but on_connection_closed only cleaned the connections map. ConnectionIds are unique, so every closed connection permanently leaked one entry — unbounded memory growth proportional to connection churn. Observed in production on a relay/dispatch hub serving ~3k-7k residential peers: ~480 bytes retained per connection close at 6-14 closes/sec, ~10-22 MB/h RSS growth per instance that never plateaus. The stale entries are functionally invisible (Shared::sender() only selects candidates via the connections map), so this is a pure leak.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Shared::on_connection_closedremoves the connection fromself.connectionsbut never removes the matching entry fromself.senders. Every connection that is ever established leaks onempsc::Sender<(PeerId, Stream)>entry, keyed byConnectionId, for the lifetime of theBehaviour.The leak is functionally invisible -
sender()only consults theconnectionsmap, so a stalesendersentry is never read - which is why it survived this long. It is purely unbounded memory growth proportional to total connections ever accepted, not concurrent connections.This patch adds the missing
self.senders.remove(&conn);alongside the existingself.connections.remove(&conn);.Production observation
We run a hub on
libp2p-streamserving 3k-7k residential peers with high churn (mobile/residential NAT, short-lived connections). Each leaked entry costs roughly 480 bytes (ConnectionIdkey +mpsc::Sender+ hashmap slot overhead), which showed up as a steady 10-22 MB/h RSS growth per instance that scaled with connection churn rate rather than concurrent connection count - the signature of a per-connection-close leak rather than a per-live-connection cost. RSS flattens with this patch applied.Change
Two files:
protocols/stream/src/shared.rs- prunesendersinon_connection_closed, plus a test module covering it.protocols/stream/CHANGELOG.md- entry under0.5.0-alpha.Tests
Two unit tests added:
connection_close_prunes_senders- assertssendersis empty afteron_connection_closedfor a connection that had a registered sender.reconnect_churn_does_not_accumulate_senders- drives repeated connect/close cycles and assertssendersdoes not grow across them (the regression that actually bit us).Notes
The branch is based on a slightly older
masterand is behind by unrelated commits; the diff is a 2-file patch that applies cleanly to currentmaster. Happy to rebase or reword the changelog entry as preferred.