Avoid triggered checks from ICE-lite agents#921
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #921 +/- ##
==========================================
+ Coverage 88.44% 88.50% +0.05%
==========================================
Files 45 45
Lines 5886 5898 +12
==========================================
+ Hits 5206 5220 +14
+ Misses 469 468 -1
+ Partials 211 210 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9936440 to
9acc6a5
Compare
|
@zshang-oai There is an already a PR to solve this issue #909 I just need to test it more, We also need to be careful because we don't handle ice-lite controlling mode yet. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts ICE-lite behavior so that lite agents (which act only as STUN servers) do not originate triggered connectivity checks when handling inbound Binding Requests—particularly for accepted nominations—removing an unnecessary RTT before pair selection.
Changes:
- Refactored
controlledSelector.HandleBindingRequestto share logic via an internal helper and gate triggered checks behind apingCandidateflag. - Added a dedicated ICE-lite
HandleBindingRequestpath that responds without sending triggered checks, while still allowing nomination handling to select the pair immediately. - Added a regression test ensuring an accepted ICE-lite nomination does not result in an outbound triggered check.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
selection.go |
Introduces a shared controlled binding-request handler with an option to suppress triggered checks; adds ICE-lite binding-request behavior that avoids outbound checks. |
selection_test.go |
Adds a regression test verifying ICE-lite accepted nominations select the pair without sending triggered checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -424,6 +424,15 @@ func (s *controlledSelector) HandleSuccessResponse(m *stun.Message, local, remot | |||
| } | |||
|
|
|||
| func (s *controlledSelector) HandleBindingRequest(message *stun.Message, local, remote Candidate) { //nolint:cyclop | |||
|
@JoTurk Oh did not realize that, sorry and thank you |
Summary
HandleBindingRequestpath that responds without originating triggered checksUSE-CANDIDATE/ nomination requests as succeeded before reusing the controlled nomination handlingWhy
ICE-lite agents act only as STUN servers. Reusing the full controlled-agent request path caused the lite side to originate a triggered connectivity check, adding an unnecessary RTT before the pair became selected. For accepted nomination requests, RFC 8445 section 7.3.2 lets the lite side treat the pair as valid/nominated from the inbound request path instead.
Validation
go test ./... -run 'TestLiteSelector_NoTriggeredCheckForAcceptedNomination|TestControlledSelector_TriggeredCheckDuringChecking|TestControlledSelector_NoTriggeredCheckAfterConnected' -count=1go test ./...; it timed out inTestMulticastDNSOnlyConnectionin this local environment after about 117s