Repository navigation
Add m_pPendingTurnRequest and fix flaky tests - #468
Open
Ibn-Butt-Toota wants to merge 1 commit into
Open
Ibn-Butt-Toota wants to merge 1 commit into
Ibn-Butt-Toota wants to merge 1 commit into
Conversation
This file contains hidden or 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
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.
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
directpair check get dropped, and then another check on thedirectpair get dropped. Then have we have arelaypair checkSucceed, which means it'll get nominated (and aUSE-CANDIDATErequest is sent). A thirddirectpair check (that actually goes through) has its checkSucceed, but since therelay's confirmation is still in flight, plus a pending nomination (in this case, therelay's) blocks new nominations, thedirectpair gets skipped and therelayconfirmation would arrive and result in therelaygetting selected.On top of that, a pair isn't checked again after it
Succeeds. The only upgrade path is when a pairSucceeds after a selection exists. In this case, thedirectpairSucceeded before the selection, so the upgrade ends up never happening.The controlled side had a similar guard in
OnPacketReceivedwhere aUSE-CANDIDATEfor a pair that didn'tSucceedwas 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, theThink_DiscoverRelayCandidatewould 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 theAllocateis only queued. At this point, the counters are printed, withallocate_sendbeing 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.