Skip to content

Add m_pPendingTurnRequest and fix flaky tests - #468

Open
Ibn-Butt-Toota wants to merge 1 commit into
ValveSoftware:masterfrom
Ibn-Butt-Toota:butt/fix-flaky-tests-nov-26
Open

Ibn-Butt-Toota wants to merge 1 commit into
ValveSoftware:masterfrom
Ibn-Butt-Toota:butt/fix-flaky-tests-nov-26

Conversation

@Ibn-Butt-Toota

Copy link
Copy Markdown

I noticed the flaky test in the CI (it failed in the workflow for main) and found 2 bugs.

The full-cone NAT test with 10% packet loss actually reveals both of them. That being said, because of the packet loss, the test needs 100 ticks instead of 40 ticks since 2 seconds wasn't enough to handle the traffic when there's 10% loss. So the 100 ticks is 5 seconds now.

The first bug appears in 2 places, but is basically the same thing: a pending nomination on a pair blocks nominating a better pair.

With packet loss, lets say we have a direct pair check get dropped, and then another check on the direct pair get dropped. Then have we have a relay pair check Succeed, which means it'll get nominated (and a USE-CANDIDATE request is sent). A third direct pair check (that actually goes through) has its check Succeed, but since the relay's confirmation is still in flight, plus a pending nomination (in this case, the relay's) blocks new nominations, the direct pair gets skipped and the relay confirmation would arrive and result in the relay getting selected.

On top of that, a pair isn't checked again after it Succeeds. The only upgrade path is when a pair Succeeds after a selection exists. In this case, the direct pair Succeeded before the selection, so the upgrade ends up never happening.

The controlled side had a similar guard in OnPacketReceived where a USE-CANDIDATE for a pair that didn't Succeed was ignored while a nominated check on any other pair was in flight. That would cause asymmetric runs where the server moved to direct but the client stayed on relay.

The fix in both places was to make the pending nomination only block pairs that aren't better than itself via BIsPreferredRouteOver.

The second bug was from the TURN allocation waiting behind STUN discovery. The original implementation only had one slot for each interface (m_pPendingSTUNRequest) for each request. Then, the Think_DiscoverRelayCandidate would skip the interface when the slot was busy, causing TURN Allocate to not start until the bind is finished. If you have an unlucky run where 4 STUN bind requests in a row are dropped, then the STUN discovery would still be retransmitting at the end of the test, which means the Allocate is only queued. At this point, the counters are printed, with allocate_send being 0.

I decided to add a second slot (m_pPendingTURNRequest) so that way we can have both the STUN bind and TURN Allocate go out at the same time, which conveniently saves one RTT in the normal case. The change to 100 tickets also is enough time for the full retransmit schedule of 0, 0.4, 1.0, 1.9, and 3.25 plus a retry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant