Add round trip time measurement to candidate pair #731
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #731 +/- ##
==========================================
+ Coverage 79.11% 79.20% +0.08%
==========================================
Files 41 41
Lines 3783 3799 +16
==========================================
+ Hits 2993 3009 +16
Misses 558 558
Partials 232 232
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Thank you @boks1971 looks great! Mind adding some simple coverage to this. Just assert values are non-zero, so someone doesn't regress it. |
candidatepair.go
Outdated
| @@ -28,6 +30,12 @@ type CandidatePair struct { | |||
| state CandidatePairState | |||
| nominated bool | |||
| nominateOnBindingSuccess bool | |||
|
|
|||
| // stats | |||
| statsMu sync.RWMutex | |||
There was a problem hiding this comment.
What do you think about making these atomic (if possible?)
Having another lock on such a critical path (inbound network traffic) might eventually be targeted for optimization.
There was a problem hiding this comment.
The STUN pings are usually once every few seconds. So, I think it should be fine.
But, I will look at atomics.
There is a race with updating |
Use the round trip time measurement to populate RTT fields in CandidatePairStats. Atomic and tests
candidatepair.go
Outdated
| if prevTotalRoundTripTime != nil { | ||
| totalRoundTripTime += *prevTotalRoundTripTime | ||
| } | ||
| p.totalRoundTripTime.CompareAndSwap(prevTotalRoundTripTime, &totalRoundTripTime) |
There was a problem hiding this comment.
The following sequence will get the wrong value
- goroutine A reads value A
- Another update happens and goroutine B reads value A
- goroutine A adds to the total (value A + rtt A)
- goroutine B adds to the total (value A + rtt B) <-- this is the problematic
- Depending on who write first, the final value could be
value A + rtt Aorvalue A + rtt B. Both are wrong. The correct value isvalue A + rtt A + rtt B.
There is no strong reason to use CompareAndSwap. Store would store both values whatever order it happens in. CompareAndSwap will store only the first value.
I think I am doing to change to this to just int64 and do Duration.Nanoseconds() so that I can use atomic.AddInt64().
Will make that change now.
|
Is the race possible (since we only process one packet at a time?) The issue I was avoiding was users being able to block recv by calling APIs. |
That's true. But, to make things simpler, I just changed it to |
* Add round trip time measurement to candidate pair Use the round trip time measurement to populate RTT fields in CandidatePairStats. Atomic and tests * Use int64 nanosecnods to make atomic easier
Use the round trip time measurement to populate RTT fields in CandidatePairStats.