-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remeasure round trip time #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... this won't run if:
- Keep alives are disabled.
- We're actively receiving data (we defer keepalives when that happens).
periodically re-measure round trip time
9ca702b
to
4cc3cce
Compare
2022-08-26 conversation: we're wondering if this is the right fix or whether transports expose a RTT. Next step is for @marten-seemann to review again and figure out next steps. We will do this for this 0.23 go-libp2p release. |
I made a few changes to this PR:
|
@@ -335,7 +335,25 @@ func (s *Session) measureRTT() { | |||
if err != nil { | |||
return | |||
} | |||
atomic.StoreInt64(&s.rtt, rtt.Nanoseconds()) | |||
if !atomic.CompareAndSwapInt64(&s.rtt, 0, rtt.Nanoseconds()) { | |||
prev := atomic.LoadInt64(&s.rtt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this is no longer atomic, is that okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is. Worst thing that can happen is that we read the RTT again in getRTT
. I agree that it's ugly.
* feat(session): remeasure round trip time periodically re-measure round trip time * feat(session): measure rtt during keep-alive * refactor(session): repeat measure rtt via timer * fix(session): fix flaky test * simplify RTT timer by using a time.Ticker * calculate a smoothed RTT Co-authored-by: Marten Seemann <[email protected]>
Goals
Keep round trip time up to date. This just updates the measured round trip time any time the keep alive timeout is called.