fix(peer): reconnection ping-pongs #2841
Merged
Conversation
This file contains 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
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2841 +/- ##
==========================================
- Coverage 63.26% 63.20% -0.07%
==========================================
Files 211 211
Lines 42214 42219 +5
==========================================
- Hits 26708 26684 -24
- Misses 15506 15535 +29
☔ View full report in Codecov by Sentry. |
The root of all evil is state. Especially mutable and shared. Seems to me like peer communication is using a connection where both sides think they are in control of the connection. So with higher latencies it is possible to get into a sort of a reconnect ping-pong, when one sides reconnects, starts re-sending all messages, and during that time the other side reconnects, and starts sending messages themselves... just to get new connection from the other side... Fix fedimint#2800
douglaz
approved these changes
Jul 25, 2023
|
Interesting! I was aware of that problem but expected exponential backoff+jitter to fix this after a few iterations, apparently not or some variables were chosen poorly :/ Anyway, your solution is quite elegant. |
|
Fantastic! |
Right. But I think we've optimized these at some point to improve test times or something, which becomes a problem on longer latency links. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The root of all evil is state. Especially mutable and shared. Seems to me like peer communication is using a connection where both sides think they are in control. So with higher latencies it is possible to get into a sort of a reconnect ping-pong, when one sides reconnects, starts re-sending all messages, and during that time the other side reconnects, and starts sending messages themselves... just to get new connection from the other side...
Typically the way I'd write is to have two connections (for each side) and the sending side being responsible for (re-)connecting. A bit wasteful, but makes the code easier.
Here, to avoid refactoring too much, I just make the peer with a lower
PeerIdresponsible for reconnections.Fix #2800