[client] Fix/v4 stun routing #4430
Conversation
Generate the extra candidate locally instead of sending it over the network. The new candidate is based on the original logic but does not inherit the original candidate ID to prevent congestion.
There was a problem hiding this comment.
Pull Request Overview
Updates STUN routing logic to deduplicate package sending and improve candidate handling by using candidate IDs for message routing. The PR also moves UDP multiplexer code from the bind package to a dedicated udpmux package.
- Implements candidate ID-based STUN message routing to avoid duplication
- Refactors UDP multiplexer code into a dedicated
udpmuxpackage - Updates ICE library dependency to support enhanced STUN routing
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| go.mod | Updates ICE library dependency version |
| client/internal/peer/worker_ice.go | Removes duplicate STUN candidate logic and adds candidate ID routing |
| client/iface/udpmux/*.go | New package containing refactored UDP multiplexer implementation |
| Multiple device files | Updates imports from bind to udpmux package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // overwrite the original candidate ID with the new one to avoid candidate duplication | ||
| if e.Key == ice.ExtensionKeyCandidateID { | ||
| e.Value = candidate.ID() |
There was a problem hiding this comment.
The code is modifying the original candidate's extension value instead of creating a new extension for the cloned candidate. This could affect the original candidate's ID. Consider creating a new extension with the new candidate ID rather than modifying the existing one.
| // overwrite the original candidate ID with the new one to avoid candidate duplication | |
| if e.Key == ice.ExtensionKeyCandidateID { | |
| e.Value = candidate.ID() | |
| newExt := ice.Extension{ | |
| Key: ice.ExtensionKeyCandidateID, | |
| Value: candidate.ID(), | |
| } | |
| if err := ec.AddExtension(newExt); err != nil { | |
| return nil, err | |
| } | |
| continue |
Copilot uses AI. Check for mistakes.
|
| go func() { | ||
| <-c.CloseChannel() | ||
| m.RemoveConnByUfrag(ufrag) | ||
| }() | ||
|
|
||
| m.candidateConnMap[candidateID] = c |
There was a problem hiding this comment.
would having a dedicated mutex to m.candidateConnMapmake any difference in a good way?
There was a problem hiding this comment.
I think we should protect this variable in the same way we protect connsIPv4 and connsIPv6. If I add a new lock, I risk causing a deadlock. This code is simpler.
Describe your changes
Deduplicate STUN package sending.
Originally, because every peer shared the same UDP address, the library could not distinguish which STUN message was associated with which candidate. As a result, the Pion library responded from all candidates for every STUN message.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__