fix(connection-limits): correct connection tracking #4250
Conversation
AgeManning
changed the title
Co-authored-by: Darius Clark <[email protected]>
|
Thank you @AgeManning. Reviewing right now. Will prioritize a patch release. |
|
//CC @shamil-gadelshin as I think you have described seeing this bug in the last open maintainers call. |
misc/connection-limits/src/lib.rs
Outdated
| self.pending_inbound_connections.remove(&connection_id); | ||
| self.pending_outbound_connections.remove(&connection_id); |
There was a problem hiding this comment.
@AgeManning are these two lines needed? A FromSwarm::ConnectionClosed should always be preceded by a handle_established_{outbound,inbound}_connection. In those methods we reduce the self.pending_*_connections counters.
There was a problem hiding this comment.
Removed with 8167c71. Please still review @AgeManning.
There was a problem hiding this comment.
I agree this is fine to remove. Just being overly cautious
Let me know in case you feel strongly about keeping these.
|
I pushed a couple of commits. Hope you don't mind @AgeManning. Any objections to the additional changes? If not, I will merge here and cut a patch release. Again, thank you for the work @AgeManning and @divagant-martian. |
mxinden
changed the title
There was a problem hiding this comment.
Ty for the follow up changes @mxinden. To speed this up since it's late for @AgeManning I'm taking the lead on saying this looks good
mergify
bot
dismissed
mxinden’s stale review
Approvals have been dismissed because the PR was updated after the send-it label was applied.
…orrect_connection_limit
mergify
bot
dismissed
mxinden’s stale review
Approvals have been dismissed because the PR was updated after the send-it label was applied.
|
@mxinden just to bring your attention to the fact that mergify was not very fond of the merge commit |
:/ Thanks! I think the root of the issue is, that only I have push access to https://github.com/sigp/, not all rust-libp2p maintainers (including mergify). |
|
Nice. Yeah I was sleeping, sorry. Looks like diva had us covered. |
The connection limit behaviour was not taking into account connection errors. Also, it was using the behaviour events to indicate established connections which is not always going to be the case because other behaviours can deny the connections (thanks @divagant-martian). Closes #4249 Pull-Request: #4250.
Description
The connection limit behaviour was not taking into account connection errors. Also, it was using the behaviour events to indicate established connections which is not always going to be the case because other behaviours can deny the connections (thanks @divagant-martian).
Closes #4249
Notes & open questions
Change checklist