Debug window (not for merging)#38
Conversation
WalkthroughStarts the adapter in a background goroutine and launches a Tk9.0 debug GUI; exposes package-level PeerManager and applog.LogFilePath; caches user nicknames; filters a specific TURN URL and adjusts ICE/backoff logic; and extends Peer and PeerManager with TURN-aware manual/manual-remote reconnection, per-peer watcher scaffolding, and aggregated stats/turn exposure. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Main as main()
participant GUI as dbgwnd.CreateMainWindow()
participant BG as Adapter Goroutine
participant Adapter as adapter.Start()
participant PM as adapter.PeerManager
participant Log as applog
Main->>BG: spawn goroutine (init, NewPeerManager, adapter.Start)
Main->>GUI: CreateMainWindow() (main thread)
BG->>Adapter: adapter.Start() builds ICE servers, filters TURNs, creates PeerManager
Adapter->>PM: assign package-level PeerManager
Note right of PM: accessible via GetPeerManager(), GetTurnURLs(), GetAllPeersStats()
GUI->>PM: request stats / trigger manual reconnect
GUI->>Log: query GetLogFilePath()
GUI->>GUI: refreshUI() loop (periodic logs + stats)
alt UI-triggered manual reconnect
GUI->>PM: HandleManualReconnectRequest(playerId)
PM->>PM: scheduleManualReconnection / preparePeerForManualReconn
else Automatic watcher-driven reconnect
PM->>PM: initialConnectionWatcher -> handleReconnection(forceReconnect=false)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| } | ||
| }() | ||
|
|
||
| dbgwnd.CreateMainWindow() |
There was a problem hiding this comment.
We do not want to wrap regular faf-adapter with some UI related things, if we want to have debug window - let's introduce separate cmd or flag instead. IMHO this approach is very bad.
There was a problem hiding this comment.
Yea, cmd flag is needed for sure. I didn't add it so far as I don't want to modify FAF client for testing
| func GetLogFilePath () string { | ||
| return LogFilePath | ||
| } No newline at end of file |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
adapter/adapter.go(3 hunks)applog/applog.go(3 hunks)cmd/faf-adapter/main.go(1 hunks)dbgwnd/dbgwnd.go(1 hunks)go.mod(2 hunks)webrtc/peer_manager.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
cmd/faf-adapter/main.go (4)
launcher/info.go (2)
NewInfoFromFlags(22-63)Info(8-20)util/common.go (1)
WrapAppContextCancelExitMessage(19-27)adapter/adapter.go (1)
New(33-47)dbgwnd/dbgwnd.go (1)
CreateMainWindow(57-134)
webrtc/peer_manager.go (1)
adapter/adapter.go (1)
PeerManager(19-19)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(45-58)
dbgwnd/dbgwnd.go (2)
applog/applog.go (1)
GetLogFilePath(375-377)adapter/adapter.go (1)
GetPeerManager(181-183)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapter/adapter.go (1)
65-76: Fix broken exponential backoff logic in ICE-Breaker retry loopThe backoff condition at line 74 will never be true. Since
backoffstarts attime.Second(positive), comparing it to0*time.Secondprevents the exponential backoff from ever activating, leaving retry delays stuck at 1 second indefinitely.Replace the condition with a ceiling check:
case <-time.After(backoff): - if backoff < 0*time.Second { + const maxBackoff = 30 * time.Second + if backoff < maxBackoff { backoff *= 2 + if backoff > maxBackoff { + backoff = maxBackoff + } }
♻️ Duplicate comments (2)
webrtc/peer_manager.go (1)
266-279: GuardGetAllPeersStatsagainst nil peers / nil PeerConnections (still panics)Peers that are just being created (or have been closed) can leave
peerorpeer.connectionnil;GetAllPeersStatsstill dereferencesp.connectionunconditionally, so the debug window will panic as soon as it asks for stats on such a peer.You can safely skip those entries:
func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string) { p.peersMu.Lock() defer p.peersMu.Unlock() allPeersStats := make(map[string]webrtc.StatsReport) connectionStates := make(map[string]string) - for id,p := range(p.peers) { - allPeersStats[fmt.Sprintf("%d", id)] = p.connection.GetStats() - connectionStates[fmt.Sprintf("%d", id)] = p.connection.ConnectionState().String() - } + for id, peer := range p.peers { + if peer == nil || peer.connection == nil { + continue + } + + peerID := fmt.Sprintf("%d", id) + allPeersStats[peerID] = peer.connection.GetStats() + connectionStates[peerID] = peer.connection.ConnectionState().String() + } return allPeersStats, connectionStates }dbgwnd/dbgwnd.go (1)
319-395: Don’t stop parsing logs on short/blank lines; usecontinueinstead ofbreakBoth branches of
formatLogLinesdo:if len(st) < 5 { break }After
strings.Split(fileContent, "\n"), the last entry is usually an empty string, so thisbreakstops processing permanently—new log lines appended to the file will never be added torawLogs/formattedLogs.Change these to
continueso short/malformed lines are skipped but later entries are still handled:- if len(st) < 5 { - break - } + if len(st) < 5 { + continue + } @@ - if len(st) < 5 { - break - } + if len(st) < 5 { + continue + }
🧹 Nitpick comments (5)
webrtc/peer.go (1)
23-50: Peer creation timestamp looks correct; consider using time.Time for clarityStoring
creationTimeSecondsastime.Now().Unix()at construction is fine for the 120s gating logic; if this becomes long‑lived, using atime.Timeandtime.Sincewould make the code more idiomatic and self‑documenting, but that’s purely optional.Also applies to: 125-138
webrtc/peer_manager.go (3)
46-60: Default TURN relay is now always forced; launcher setting is ignored
NewPeerManagerhard‑codesforceTurnRelay: trueand addsdefaultTurnServerfrom the initialturnServerslice, which meanslauncherInfo.ForceTurnRelayno longer has any effect.If this is only for the debug build that ships with the window, that’s fine; otherwise consider threading the launcher flag through, e.g.:
- forceTurnRelay: true, + forceTurnRelay: launcherInfo.ForceTurnRelay, defaultTurnServer: turnServer,Also applies to: 72-85
297-320: Age‑based reconnection gating works; could be simplified withtime.SinceUsing
time.Now().Unix() - peer.creationTimeSecondsand a hard 120s threshold correctly avoids fighting with the startup watcher, but it’s a bit manual. An equivalent, slightly clearer variant would be:peerAge := time.Since(time.Unix(peer.creationTimeSeconds, 0)) if peerAge > 2*time.Minute { // schedule reconnection } else { // log skip }Not a correctness issue, just a readability tweak.
402-405: Commented‑outforceTurnRelayfallback is dead codeThe commented block switching
peer.forceTurnRelayoff onPeerConnectionStateFailedis now dead code. If this behavior is no longer desired, consider deleting it; if it’s a planned fallback, adding a briefTODOcomment with the intended condition would make the intent clearer.dbgwnd/dbgwnd.go (1)
264-317: Log autoscroll logic is fine; consider guarding against unexpectedYviewformats
textYposis computed viastrings.Split(logView.Yview(), " ")[1]without length checks; if tk9 ever changesYview’s format, this will panic. Optional, but you could defensively handle missing parts:parts := strings.Split(logView.Yview(), " ") if len(parts) == 2 { textYpos, _ := strconv.ParseFloat(parts[1], 64) if textYpos > 0.99 || logViewLinesCount < 30 { logView.Yviewmoveto(1) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
adapter/adapter.go(4 hunks)dbgwnd/dbgwnd.go(1 hunks)webrtc/peer.go(2 hunks)webrtc/peer_manager.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(46-60)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(19-19)applog/applog.go (2)
Info(79-93)Debug(110-123)launcher/info.go (1)
Info(8-20)
🔇 Additional comments (3)
webrtc/peer_manager.go (1)
120-129: Reconnection orchestration viahandleReconnection/reconnectPeer/ UI hook looks coherentThe new flow—queue‑driven reconnection via
runReconnectionManagement, explicithandleReconnectionwithcalledFromUI/forceReconnectflags, and the direct UI entrypoint inHandleReconnectRequestFromUI—is logically consistent and reuses the sameConnectOncepath. Skipping automatic reconnection when a peer is alreadyConnectedorConnectingalso makes sense.Also applies to: 131-173, 367-381, 638-643
dbgwnd/dbgwnd.go (2)
96-104: The closure pattern is safe and correct for Go 1.25.0 — no fix neededThe review incorrectly applies pre-Go 1.22 closure semantics. Go 1.22+ (which includes the project's Go 1.25.0) changed loop variable capture behavior so that each iteration gets its own
ivariable. The code as written will correctly callreconnectBtnClickedwith the proper iteration index for each button.Likely an incorrect or invalid review comment.
373-395: Review comment is inapplicable to this codebase—no action neededThe review assumes a compatibility concern with
min(), but the project already specifiesgo 1.25.0in go.mod. Since Go 1.21 (released August 8, 2023) added predeclared min and max functions, and this codebase targets Go 1.25.0 (well past Go 1.21), themin()call at line 389 will compile without issue. A local helper is unnecessary when the project's declared Go version guarantees support for the built-in function.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webrtc/peer_manager.go (1)
545-575: Package-levelmanualReconnectionMessageintroduces shared mutable state.
manualReconnectionMessageis a package-level variable populated lazily on first candidate gathering. This creates issues:
- It's shared across all peers/games
- The
RecipientIDpointer captured during initialization will be stale for subsequent peers- No synchronization for concurrent access
Consider making this a field on
PeerManagerand constructing fresh messages per peer rather than reusing a global template.
♻️ Duplicate comments (5)
webrtc/peer_manager.go (2)
328-333: Guard against nilpeer.connection— previous review not addressed.This code still dereferences
p.connectionwithout checking for nil. Peers that are still starting up haveconnection == nil, which will cause a panic whenGetStats()orConnectionState()is called.for id, p := range(p.peers) { + if p == nil || p.connection == nil { + continue + } idAsString := fmt.Sprintf("%d", id) allPeersStats[idAsString] = p.connection.GetStats() connectionStates[idAsString] = p.connection.ConnectionState().String() turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} }
675-682: Race condition onp.peersaccess — previous review not addressed.
p.peers[playerId]is read without holdingpeersMu, andpeer.connection.ConnectionState()is called without nil checks. Both issues were flagged in a previous review.for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) - - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } + + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state pionwebrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == pionwebrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + }dbgwnd/dbgwnd.go (3)
196-212: CheckbuttonVariable()comparison uses wrong string value — previous review not addressed.Tcl/Tk stores boolean values as
"1"and"0", not"true"and"false". These comparisons will always evaluate to false.func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { + if rawLogsChkButton.Variable() == "1" { showRawLogs = true } else { showRawLogs = false } } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { + if hideDebugChkButton.Variable() == "1" { hideDebug = true } else { hideDebug = false } }
349-364: Log parsing breaks on blank lines — previous review not addressed.Using
breakwhen encountering short lines (< 5 chars) stops processing permanently. Sincestrings.Splitproduces empty strings from trailing newlines, this prevents new log lines from being displayed after the first blank line.if len(st) < 5 { - break + continue }Apply the same fix at line 362-364.
556-565:reconnectButtonsEnabledis never updated — previous review not addressed.The comparison
reconnectButtonsEnabled != numOfIdsalways succeeds becausereconnectButtonsEnabledis never assigned after initialization. This causes redundant button reconfiguration on every refresh.numOfIds := len(sortedIds) if reconnectButtonsEnabled != numOfIds { for i,b := range(reconnectButtons){ if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds }
🧹 Nitpick comments (5)
webrtc/peer.go (2)
154-165: DuplicateIsDisabled()check is redundant.Lines 155-157 and 160-162 perform the identical check back-to-back with no intervening logic that could change the state. The second check and its comment ("If peed are disconnected/died/disabled while reconnecting") don't make sense here since no reconnection attempt has occurred yet.
func (p *Peer) ConnectOnce(iceServers []webrtc.ICEServer) error { if p.IsDisabled() { return errors.New("peer is disabled") } - // If peed are disconnected/died/disabled while reconnecting, just gave up. - if p.IsDisabled() { - return errors.New("peer is disabled during reconnection") - } - return p.reconnect(iceServers) }
401-404: Downgrading send failures from Error to Debug may hide connectivity issues.Changing the log level from Error to Debug means data channel send failures won't appear in default log views. While this reduces noise, it could make debugging connection problems harder. Consider using
Warnas a middle ground, or adding a counter/metric for failed sends.webrtc/peer_manager.go (1)
21-22: Package-level mutable state creates concurrency risks.
allTurnServersUrlsandturnsNameByIdare package-level variables written insaveTurnsURLsand read inGetTurnURLs. If multiplePeerManagerinstances are created or if these are accessed concurrently, there's a data race. For a debug tool this may be acceptable, but consider making these instance fields ofPeerManager.dbgwnd/dbgwnd.go (2)
285-298: Reading entire log file on every 1-second refresh is inefficient.
os.ReadFile(logFilePath)reads the complete file contents every second. For long-running sessions, the log file can grow large, causing increased memory allocation and I/O overhead.Consider tracking file position and reading only new content:
// Track last read position var logFileOffset int64 func refreshLogs() { file, err := os.Open(logFilePath) if err != nil { return } defer file.Close() file.Seek(logFileOffset, 0) // Read new content only // Update logFileOffset after reading }
20-43: Extensive global mutable state makes the module difficult to test and reason about.The module uses 24 package-level variables for UI state. While acceptable for a debug tool, this design prevents multiple window instances, complicates testing, and creates implicit coupling between functions.
For future maintainability, consider encapsulating state in a struct:
type DebugWindow struct { logStrings []string formattedLogs []string // ... other fields }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
adapter/adapter.go(4 hunks)dbgwnd/dbgwnd.go(1 hunks)webrtc/peer.go(3 hunks)webrtc/peer_manager.go(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- adapter/adapter.go
🧰 Additional context used
🧬 Code graph analysis (3)
webrtc/peer_manager.go (2)
adapter/adapter.go (1)
PeerManager(19-19)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
dbgwnd/dbgwnd.go (2)
adapter/adapter.go (1)
GetPeerManager(184-186)applog/applog.go (1)
GetLogFilePath(375-377)
webrtc/peer.go (2)
applog/context.go (1)
FromContext(10-12)applog/applog.go (1)
Debug(110-123)
🔇 Additional comments (1)
webrtc/peer.go (1)
49-56: New peer fields for manual reconnection look reasonable.The new fields properly support the manual reconnection workflow. However, these fields (
manualReconnIsActive,remoteManualRRequest,numOfManualReconns) are accessed from multiple goroutines inpeer_manager.gowithout consistent synchronization. While some accesses usereconnectMu, others don't (e.g., reads inhandleReconnectionat lines 174, 183).Consider whether all accesses to these fields should be guarded by
reconnectMufor thread-safety, or document which goroutine owns each field.
| for i := 0; i < 15; i++ { | ||
| but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) | ||
| reconnectButtons = append(reconnectButtons, but) | ||
|
|
||
| padyFloat := 8 + float64(i)*4.5 | ||
| Grid(but, Row(1), Column(0), Sticky("NW"), Pady(fmt.Sprintf("%.1f", padyFloat)+"m 0m"),Padx("80m 0m")) | ||
| but.Configure(State("disabled")) | ||
| } |
There was a problem hiding this comment.
Closure captures loop variable by reference — all buttons will trigger with i=15.
The closure func() { reconnectBtnClicked(i) } captures i by reference. Since Go closures capture variables, not values, all 15 buttons will call reconnectBtnClicked(15) (the final value of i after the loop).
for i := 0; i < 15; i++ {
- but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) }))
+ idx := i // capture current value
+ but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) }))
reconnectButtons = append(reconnectButtons, but)🤖 Prompt for AI Agents
In dbgwnd/dbgwnd.go around lines 107 to 114 the loop closure uses the loop
variable i directly, causing all buttons to call reconnectBtnClicked(15); fix by
capturing the current loop index into a new local variable inside the loop
(e.g., idx := i) and use that variable in the Command closure (or alternatively
define the closure to accept the index as a parameter and pass i), so each
button's closure references its own copy of the index instead of the shared loop
variable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webrtc/peer_manager.go (1)
545-575: SharedmanualReconnectionMessagehas incorrectRecipientIDfor multiple peers.The global
manualReconnectionMessageis initialized once with the firstremotePeer(line 569) and reused for all subsequent manual reconnections. WhenHandleManualReconnectRequestsends this message (line 731), theRecipientIDwill be wrong for any peer other than the first one.Consider making this per-peer or constructing a fresh message with the correct
RecipientIDinHandleManualReconnectRequest:-var manualReconnectionMessage icebreaker.CandidatesMessage - func (p *PeerManager) onPeerCandidatesGathered(remotePeer uint) onPeerCandidatesGatheredCallback { return func(description *webrtc.SessionDescription, candidates []webrtc.ICECandidate) { err := p.icebreakerClient.SendEvent( icebreaker.CandidatesMessage{ // ... existing message ... }) - - // A fake candidate message that is used in manual reconnection process - // Will be fixed in future - if manualReconnectionMessage.GameID == 0 { - manualReconnectionMessage = icebreaker.CandidatesMessage{ - BaseEvent: icebreaker.BaseEvent{ - EventType: icebreaker.EventKindCandidates, - GameID: p.gameId, - SenderID: p.localUserId, - RecipientID: &remotePeer, - }, - Session: description, - Candidates: candidates, - } - manualReconnectionMessage.Candidates = make([]webrtc.ICECandidate, 0) - } - if err != nil { // ... error handling ... } } }Then in
HandleManualReconnectRequest, construct the message with the correctplayerId:func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { // ... - err := p.icebreakerClient.SendEvent(manualReconnectionMessage) + err := p.icebreakerClient.SendEvent(icebreaker.CandidatesMessage{ + BaseEvent: icebreaker.BaseEvent{ + EventType: icebreaker.EventKindCandidates, + GameID: p.gameId, + SenderID: p.localUserId, + RecipientID: &playerId, + }, + Session: nil, + Candidates: make([]webrtc.ICECandidate, 0), + }) // ... }
♻️ Duplicate comments (8)
webrtc/peer_manager.go (8)
21-22: Global TURN metadata risks cross-session contamination.Similar to the
watchersmap issue (lines 642-643), these package-level variables persist across adapter restarts or multiple game sessions. Stale TURN URLs could leak between sessions or cause unexpected behavior if the TURN configuration changes.Consider moving these into the
PeerManagerstruct as instance fields and initializing them in the constructor.
107-132: Fragile TURN URL parsing with hardcoded.comassumption.This issue was previously flagged: the function assumes TURN server URLs contain
.com(line 113), which will fail for servers using other TLDs (.org,.io,.net, country codes, etc.).Based on past review comments, consider using the
net/urlpackage to properly parse URLs instead of string slicing with hardcoded assumptions.
320-336: Guard against nilPeerConnectioninstances.This critical issue was previously flagged: peers that are still starting up leave
peer.connectionnil. Dereferencing it at lines 330-331 will panic as soon as the debug window requests stats.Based on past review comments, skip peers without an active connection before calling
GetStats()/ConnectionState():func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string, map[string][]int) { p.peersMu.Lock() defer p.peersMu.Unlock() allPeersStats := make(map[string]webrtc.StatsReport) connectionStates := make(map[string]string) turnIds := make(map[string][]int) - for id,p := range(p.peers) { + for id, peer := range p.peers { + if peer == nil || peer.connection == nil { + continue + } + idAsString := fmt.Sprintf("%d", id) - allPeersStats[idAsString] = p.connection.GetStats() - connectionStates[idAsString] = p.connection.ConnectionState().String() - turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} + allPeersStats[idAsString] = peer.connection.GetStats() + connectionStates[idAsString] = peer.connection.ConnectionState().String() + turnIds[idAsString] = []int{peer.specTurnIdLocal, peer.specTurnIdRemote} } return allPeersStats, connectionStates, turnIds }
652-718: Race condition and nil pointer dereference ininitialConnectionWatcher.This critical issue was previously flagged: at lines 678-682,
p.peers[playerId]is accessed without holdingpeersMu, causing concurrent map access. Additionally,peer.connectionmay be nil during setup or after closure but is dereferenced without checking, causing a panic.Based on past review comments, acquire the mutex before reading, check all nil conditions, and use the typed constant:
func (p *PeerManager) initialConnectionWatcher(playerId uint) { // ... setup code ... for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + } reconnectDelaySec -= 1 // ... rest of logic ... } // ... }
642-643: Globalwatchersmap risks cross-session contamination.This issue was previously flagged: the
watchersmap andwatcherMutexare package-level. If the adapter is restarted or multiple games run, stale entries persist and could prevent new watchers from being created for the same player IDs.Based on past review comments, consider making
watchersan instance field ofPeerManageror clearing it when a newPeerManageris created.
721-745:HandleManualReconnectRequestdoesn't unlock mutex on all paths.This critical issue was previously flagged: when
peer.manualReconnIsActive || peer.remoteManualRRequestis true (line 726 condition fails), the else branch at lines 737-739 logs but never unlockspeersMu.Based on past review comments, use
deferto ensure the mutex is always released:func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { peerAsString := fmt.Sprintf("%d", playerId) p.peersMu.Lock() + defer p.peersMu.Unlock() + peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive && !peer.remoteManualRRequest { peer.manualReconnIsActive = true - p.peersMu.Unlock() p.preparePeerForManualReconn(playerId) err := p.icebreakerClient.SendEvent(manualReconnectionMessage) if err != nil { applog.Error("Failed to send manualReconnectionMessage to peer " + peerAsString, zap.Error(err)) } go p.scheduleManualReconnection(playerId) } else { applog.Debug("Manual reconnection is already in process. Skipping local request. Peer: " + peerAsString) } } else { applog.Error("Can't initiate manual reconnection: No such peer " + peerAsString) - p.peersMu.Unlock() } - }Note: After this fix, you'll need to ensure
preparePeerForManualReconndoesn't also try to acquirepeersMu(it does at line 782), so you'll need to refactor to avoid deadlock (see separate comment on that function).
763-778: Mutex not released on all code paths inhandleRemoteManualReconnRequest.This critical issue was previously flagged: when
peer.manualReconnIsActiveis true (line 767 condition fails), the function returns without unlockingpeersMu.Based on past review comments, use
deferto ensure proper cleanup:func (p *PeerManager) handleRemoteManualReconnRequest(playerId uint) { p.peersMu.Lock() + defer p.peersMu.Unlock() + peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive { peer.remoteManualRRequest = true - p.peersMu.Unlock() // No need to initiate reconn process when remote request is recieved // We prepare specific turn and then wait for reconn from other side p.preparePeerForManualReconn(playerId) - } - } else { - p.peersMu.Unlock() + } } }Note: After this fix, you'll need to ensure
preparePeerForManualReconndoesn't also try to acquirepeersMu(it does at line 782), so you'll need to refactor to avoid deadlock (see separate comment on that function).
780-837:preparePeerForManualReconnacquires mutex that caller may already hold.This issue was previously flagged: this function locks
peersMuat line 782, but it's called fromHandleManualReconnectRequestandhandleRemoteManualReconnRequest. If those functions are fixed to usedefer p.peersMu.Unlock()(as suggested in separate comments), this will deadlock.Based on past review comments, you have two options:
Option 1 (recommended): Make this function assume the caller holds the lock:
-func (p *PeerManager) preparePeerForManualReconn(playerId uint) { +// preparePeerForManualReconnLocked prepares peer for manual reconnection. +// Caller must hold p.peersMu. +func (p *PeerManager) preparePeerForManualReconnLocked(playerId uint) { peerAsString := fmt.Sprintf("%d", playerId) - p.peersMu.Lock() peer, ok := p.peers[playerId] if ok { // ... existing logic ... } - p.peersMu.Unlock() }Then update callers to call the
Lockedvariant while holding the mutex.Option 2: Ensure all callers explicitly unlock before calling this function (requires NOT using
deferin the calling functions, which contradicts the other suggested fixes).
🧹 Nitpick comments (2)
webrtc/peer_manager.go (2)
85-85: HardcodedforceTurnRelayoverrides launcher configuration.This unconditionally sets
forceTurnRelaytotrue, ignoringlauncherInfo.ForceTurnRelay. While acceptable for debug/testing purposes, ensure this doesn't make it to production as it prevents users from choosing their relay policy.
256-262: "Dirty hook" using empty candidates for manual reconnection signaling.The use of an empty
Candidatesarray to signal manual reconnection is a workaround. While functional for debugging, consider adding a properManualReconnectionMessageevent type to the icebreaker protocol for cleaner semantics if this feature is promoted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webrtc/peer_manager.go(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webrtc/peer_manager.go (2)
launcher/info.go (1)
Info(8-20)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (1)
webrtc/peer_manager.go (1)
163-219: LGTM: Reconnection logic with manual TURN support.The addition of the
forceReconnectparameter and the conditional logic to handle manual reconnection with specific TURN servers vs. normal reconnection is well-structured. The approach of resetting TURN IDs for normal connections (lines 186-191) is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
webrtc/peer_manager.go (5)
107-132: Fragile TURN URL parsing assumes all URLs contain.com.This logic (line 113) will fail for servers using
.org,.io,.net, or country-code TLDs. The string slicing on lines 116-125 is also error-prone.Use
net/urlto parse URLs properly, extract the host, and derive a meaningful name from the hostname components.As per past review comments, this approach is more robust and handles edge cases correctly.
320-336: Nil pointer dereference risk inGetAllPeersStats.Lines 330-331 dereference
p.connectionwithout checking for nil. Peers that are still starting up or have been closed will have nil connections, causing a panic when the debug window requests stats.Add nil checks before accessing
connection:func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string, map[string][]int) { p.peersMu.Lock() defer p.peersMu.Unlock() allPeersStats := make(map[string]webrtc.StatsReport) connectionStates := make(map[string]string) turnIds := make(map[string][]int) - for id,p := range(p.peers) { + for id, peer := range p.peers { + if peer == nil || peer.connection == nil { + continue + } + idAsString := fmt.Sprintf("%d", id) - allPeersStats[idAsString] = p.connection.GetStats() - connectionStates[idAsString] = p.connection.ConnectionState().String() - turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} + allPeersStats[idAsString] = peer.connection.GetStats() + connectionStates[idAsString] = peer.connection.ConnectionState().String() + turnIds[idAsString] = []int{peer.specTurnIdLocal, peer.specTurnIdRemote} } return allPeersStats, connectionStates, turnIds }As per past review comments, this prevents panics when peers are in transitional states.
642-718: Race condition and nil pointer risks ininitialConnectionWatcher.Two critical issues identified by past review:
Race condition (lines 678-682):
p.peers[playerId]is accessed without holdingpeersMu, causing concurrent map access when other goroutines modify the map.Nil pointer (line 679):
peer.connection.ConnectionState()is called without checking ifpeerorpeer.connectionis nil, which will panic during startup or after closure.Acquire the mutex before reading, check for nil, release immediately:
for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + }As per past review comments, this prevents both the race and the panic.
Additionally, the global
watchersmap (lines 642-643) persists across adapter restarts, which past review flagged as a major issue. Consider moving it intoPeerManageras documented in prior feedback.
721-745: Mutex not unlocked in one code path.Line 738's else branch (when manual reconnection is already active) logs but never unlocks
peersMu, causing a deadlock. Line 742 correctly unlocks for the "no such peer" case.Use
deferto ensure the mutex is always released:func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { peerAsString := fmt.Sprintf("%d", playerId) p.peersMu.Lock() + defer p.peersMu.Unlock() + peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive && !peer.remoteManualRRequest { peer.manualReconnIsActive = true - p.peersMu.Unlock() p.preparePeerForManualReconn(playerId) err := p.icebreakerClient.SendEvent(manualReconnectionMessage) if err != nil { applog.Error("Failed to send manualReconnectionMessage to peer " + peerAsString, zap.Error(err)) } go p.scheduleManualReconnection(playerId) } else { applog.Debug("Manual reconnection is already in process. Skipping local request. Peer: " + peerAsString) } } else { applog.Error("Can't initiate manual reconnection: No such peer " + peerAsString) - p.peersMu.Unlock() } - }Note: After this fix,
preparePeerForManualReconn(line 729) will deadlock because it also tries to acquirepeersMu(line 784). See separate comment on that function.As per past review comments, using
defersimplifies the control flow and prevents missed unlocks.
782-839:preparePeerForManualReconnwill deadlock when callers holdpeersMu.This function locks
peersMu(line 784), but its callers (HandleManualReconnectRequestandhandleRemoteManualReconnRequest) may already hold the lock. If you apply the recommendeddeferunlock pattern to those functions, calling this will deadlock.Either:
- Refactor to assume caller holds lock (recommended): Rename to
preparePeerForManualReconnLocked, remove lines 784 and 838, and document that callers must holdpeersMu.- Ensure callers unlock before calling: Explicitly unlock in callers before calling this function.
Option 1 is cleaner and prevents accidental misuse.
As per past review comments, clarifying mutex ownership prevents these issues.
🧹 Nitpick comments (5)
webrtc/peer_manager.go (5)
21-22: Global TURN state may cause cross-session issues.These package-level variables persist across PeerManager instances. If the adapter restarts or multiple games run, stale TURN data could interfere.
Consider moving
allTurnServersUrlsandturnsNameByIdinto thePeerManagerstruct and initializing them inNewPeerManagerorsaveTurnsURLs.
85-85:forceTurnRelayhardcoded, ignoring launcher configuration.The value is always set to
true, bypassinglauncherInfo.ForceTurnRelay. If this is intentional for debugging, consider adding a comment. Otherwise, restore the launcher value.
256-262: Empty candidates used as manual reconnection signal (acknowledged workaround).The comment indicates this is temporary. While functional for debugging, consider adding a dedicated
ManualReconnectionMessageevent type to the icebreaker protocol for clarity and type safety.
545-575: GlobalmanualReconnectionMessageinitialized lazily on first use.Line 563 checks
GameID == 0to determine first use. This works but adds overhead on every call. Consider initializing this inNewPeerManageror a dedicated setup function.Also, as with other globals, this could cause issues if multiple
PeerManagerinstances exist.
763-780: Mutex is now unlocked on all paths (past issue addressed).Lines 769, 775, and 778 ensure
peersMuis released in all branches. This addresses the past review concern.However, using
defer p.peersMu.Unlock()after line 764 would simplify the code and prevent future mistakes:func (p *PeerManager) handleRemoteManualReconnRequest(playerId uint) { p.peersMu.Lock() + defer p.peersMu.Unlock() + peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive { peer.remoteManualRRequest = true - p.peersMu.Unlock() p.preparePeerForManualReconn(playerId) - } else { - p.peersMu.Unlock() } - } else { - p.peersMu.Unlock() } }Note:
preparePeerForManualReconnalso lockspeersMu(see separate comment), so you'll need to refactor that function to avoid deadlock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webrtc/peer_manager.go(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webrtc/peer_manager.go (4)
adapter/adapter.go (1)
PeerManager(19-19)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)launcher/info.go (1)
Info(8-20)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (4)
webrtc/peer_manager.go (4)
163-219: Manual reconnection flow separation looks functional.The logic correctly differentiates between normal reconnection (using default TURN list) and manual reconnection (using
peerSpecificTurn). The flag management and error handling appear sound.The complexity is reasonable given the dual-path requirement.
364-378: Startup reconnection skip logic is a reasonable workaround.The 2-minute threshold prevents reconnection attempts from interfering with initial connection establishment. The comment acknowledges this needs a proper fix later, which is appropriate for this debug window context.
452-477: Manual reconnection state handling prevents conflicting reconnection attempts.The logic correctly skips automatic reconnection scheduling when manual reconnection is active, avoiding race conditions between the two flows.
747-761:scheduleManualReconnectionmutex usage is correct.The function properly acquires and releases
peersMuaround the peer existence check before callinghandleReconnection.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
dbgwnd/dbgwnd.go (4)
108-116: Fix closure over loop variable so each RECONNECT button uses its own indexAll 15 buttons currently capture
iby reference, so they will all callreconnectBtnClicked(15)instead of their own index. Captureiinto a local variable inside the loop and use that in the closure.- // Reconnect buttons (15 buttons as max players count is 16) - for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + // Reconnect buttons (15 buttons as max players count is 16) + for i := 0; i < 15; i++ { + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
142-147: Checkbutton state handling is brittle;.Variable()comparison may never flip flagsThe checkbuttons bind
Variable(rawLogsChButtonVar)/Variable(hideDebugButtonVar)but the callbacks compare...Variable() == "true", which may not match the actual string values tk9.0 stores for the on/off states. That can leaveshowRawLogsandhideDebugeffectively stuck.Consider either:
- Binding real Go bools (if supported by tk9.0) and reading those, or
- Using explicit on/off strings and comparing against those, e.g.:
-rawLogsChkButton = logFrame.Checkbutton(Txt("Show unformatted logs"), Variable(rawLogsChButtonVar), -Onvalue(true), Offvalue(false), Command(func() { rawLogsChkButtonPressed() })) +rawLogsChkButton = logFrame.Checkbutton( + Txt("Show unformatted logs"), + Variable("rawLogs"), // tk variable name or similar + Onvalue("true"), + Offvalue("false"), + Command(func() { rawLogsChkButtonPressed() }), +) func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { - showRawLogs = true - } else { - showRawLogs = false - } + showRawLogs = (rawLogsChkButton.Variable() == "true") }Apply the same pattern to
hideDebugChkButtonPressed().Also applies to: 198-214
287-340: Log parsing stops at the first short/blank line; later log entries are skippedIn
formatLogLines()both branchesbreakthe loop whenlen(st) < 5. If a blank or very short line appears anywhere in the log (not just the trailing split-empty), all subsequent entries are ignored on every refresh, which can make the log view silently stale.Replace the
breakwithcontinueso short lines are skipped but later lines are still processed:- if len(st) < 5 { - break - } + if len(st) < 5 { + continue + } @@ - if len(st) < 5 { - break - } + if len(st) < 5 { + continue + }Also applies to: 342-419
561-570:reconnectButtonsEnabledis never updated, so the check is redundant
reconnectButtonsEnabledis always0, so wheneverlen(sortedIds) > 0this block runs on everyrefreshConnStats()call. Either update the variable after configuring the buttons, or remove the comparison.To make the optimization effective:
numOfIds := len(sortedIds) if reconnectButtonsEnabled != numOfIds { for i,b := range(reconnectButtons){ if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds }
🧹 Nitpick comments (4)
dbgwnd/dbgwnd.go (4)
226-255: Be explicit aboutListbox.Getreturn type and indexing for readability/safetyBoth
turnServerSelected()andinfoListIdSelected()assumeListbox.Get(...)returns a slice and index into[0]. If tk9.0 ever changes this API or someone unfamiliar reads the code, it’s easy to misinterpret this as string indexing.To make intent clear and safer, consider naming the element and avoiding double
len(...) > 0checks:- selectedURL := turnServersList.Get(strconv.Itoa(selected[0])) - if len(selectedURL) > 0 { - selectedTurnServer = selectedURL[0] + urls := turnServersList.Get(strconv.Itoa(selected[0])) + if len(urls) > 0 { + selectedTurnServer = urls[0] selectedTurnURL.Configure(Txt(selectedTurnServer)) }Same idea for
connIdListBox.Get(...)/selectedId[0].
287-301: Reading the whole log file every second can be expensive on large logs
refreshLogs()re-reads the entire log file (os.ReadFile) each second, then re-splits and re-formats. For long-running sessions or big logs this can become a noticeable I/O and CPU hit.Consider tailing from the last offset or using
os.FileandSeekto read only appended data; keep the full-file read as a fallback for initial load or error recovery.
378-415: Log formatting assumes a very specific JSON layout and can panic on unexpected linesThe timestamp replacement and user-id stripping assume fixed positions:
formattedLogs[i][:index-27]will panic ifindex < 27.- The
"localUserId"/"localGameId"removal logic assumes both keys are present and ordered; malformed or different log formats can break this.For a debug tool this may be acceptable, but to make it more robust you could:
- Guard
index >= 27before slicing.- Use
encoding/jsoninto a small struct/map and re-emit the fields you want instead of manual string surgery.
422-444: Avoid dummy element insortedIdsIntto simplify logic
sortedIdsInt := make([]int, 1)seeds the slice with an extra zero that you then filter out (if id != 0). This works but is confusing.Simpler and clearer:
- sortedIds = nil - sortedIdsInt := make([]int, 1) + sortedIds = nil + var sortedIdsInt []int for id := range allPeersStats { i, err := strconv.Atoi(id) if err == nil { sortedIdsInt = append(sortedIdsInt, i) }
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
webrtc/peer_manager.go (6)
787-844: Lock ordering inpreparePeerForManualReconnis fragile.This function acquires
peersMuwhile callers sometimes hold it before calling. Current callers do unlock first, but this pattern is error-prone. Consider either documenting the lock contract clearly or making it aLockedvariant that assumes the caller holds the lock.
331-336: Nil pointer dereference risk inGetAllPeersStats.
p.connectionmay be nil during peer setup or after closure. Dereferencing it without a guard will panic.- for id,p := range(p.peers) { + for id, peer := range(p.peers) { + if peer == nil || peer.connection == nil { + continue + } idAsString := fmt.Sprintf("%d", id) - allPeersStats[idAsString] = p.connection.GetStats() - connectionStates[idAsString] = p.connection.ConnectionState().String() - turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} + allPeersStats[idAsString] = peer.connection.GetStats() + connectionStates[idAsString] = peer.connection.ConnectionState().String() + turnIds[idAsString] = []int{peer.specTurnIdLocal, peer.specTurnIdRemote} }Note: The loop variable
pshadows the receiverp *PeerManager. Rename it topeerfor clarity.
108-133: Fragile TURN URL parsing assumes.comTLD.The string parsing at line 114 looks for
.com, which will fail for servers using other TLDs (.org,.io,.net, country codes). Consider usingnet/urlfor proper parsing.
645-646: Globalwatchersmap risks cross-session state leakage.The package-level
watchersmap and mutex persist across adapter restarts. Stale entries could prevent new watchers from being created for the same player IDs in a new session. Consider making this an instance field ofPeerManager.
678-685: Race condition and nil pointer risk ininitialConnectionWatcher.
p.peers[playerId]is accessed without holdingpeersMu, racing with other goroutines. Additionally,peer.connectionmay be nil and is dereferenced without checking.for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) - - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } + + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + }
724-749: Mutex not released when manual reconnection is already active.When
peer.manualReconnIsActive || peer.remoteManualRRequestis true (line 729 condition fails), the else branch at lines 741-743 logs but never unlockspeersMu, causing a deadlock. Usedeferfor cleaner control flow.func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { peerAsString := fmt.Sprintf("%d", playerId) p.peersMu.Lock() + defer p.peersMu.Unlock() + peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive && !peer.remoteManualRRequest { peer.manualReconnIsActive = true p.disableRecBtnPeers = append(p.disableRecBtnPeers, peerAsString) - p.peersMu.Unlock() - p.preparePeerForManualReconn(playerId) + // Note: preparePeerForManualReconn also acquires peersMu - refactor needed + go func() { + p.preparePeerForManualReconn(playerId) + }() err := p.icebreakerClient.SendEvent(manualReconnectionMessage) // ... rest unchanged } else { applog.Debug("Manual reconnection is already in process. Skipping local request. Peer: " + peerAsString) } } else { applog.Error("Can't initiate manual reconnection: No such peer " + peerAsString) - p.peersMu.Unlock() } - }Warning:
preparePeerForManualReconnalso acquirespeersMu(line 789), so withdefer, you'll need to refactor to avoid deadlock. Either makepreparePeerForManualReconnassume the lock is held, or call it after unlocking.dbgwnd/dbgwnd.go (5)
117-124: Closure captures loop variable — all buttons trigger withi=15.The loop variable
iis captured by reference in the closure. After the loop completes, all 15 buttons will callreconnectBtnClicked(15).for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
206-222: Checkbutton value comparisons will always fail.Tcl/Tk stores boolean values as
"1"and"0", not"true"and"false". The comparisons at lines 208 and 217 will never match, soshowRawLogsandhideDebugwill always be set tofalse.func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { + if rawLogsChkButton.Variable() == "1" { showRawLogs = true } else { showRawLogs = false } } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { + if hideDebugChkButton.Variable() == "1" { hideDebug = true } else { hideDebug = false } }
359-361: Log parsing stops on first blank line.
strings.Splityields empty strings from trailing newlines. Thebreakstatements stop processing entirely on the first short entry, preventing new log lines from reaching the UI. Replace withcontinue.if len(st) < 5 { - break + continue }Apply this fix in both branches (lines 359-361 and 372-374).
Also applies to: 372-374
548-555: Missing bounds check onturnIdsslice access.Accessing
tIds[0]andtIds[1]without verifying slice length can panic if the slice is shorter than expected.if tIds, ok := turnIds[id]; ok { - if tIds[0] != 0 { + if len(tIds) > 0 && tIds[0] != 0 { localTurnName = turnsNameById[tIds[0]] } - if tIds[1] != 0 { + if len(tIds) > 1 && tIds[1] != 0 { remoteTurnName = turnsNameById[tIds[1]] } }
581-593:reconnectButtonsEnabledis incorrectly updated inside the loop.The variable is incremented inside the loop (line 586) instead of being set to
numOfIdsafter the loop. This causes the enable/disable logic to run on every refresh and produces incorrect state tracking.func refreshButtons() { numOfIds := len(sortedIds) if reconnectButtonsEnabled != numOfIds { for i,b := range(reconnectButtons){ - reconnectButtonsEnabled += 1 if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds }
🧹 Nitpick comments (2)
dbgwnd/dbgwnd.go (2)
25-53: Extensive mutable global state reduces testability and risks unintended side effects.The file relies heavily on package-level mutable state. While acceptable for a debug tool, consider encapsulating this state in a struct if this code is ever promoted to production. This would improve testability and allow multiple instances if needed.
295-308: File reads entire log on every refresh.
os.ReadFile(logFilePath)reads the entire log file every second. For large log files, this becomes expensive. Consider usingos.Seekand reading only new content, or limiting the displayed lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(19-19)applog/applog.go (1)
Info(79-93)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
dbgwnd/dbgwnd.go (3)
adapter/adapter.go (2)
PeerManager(19-19)GetPeerManager(184-186)moho/packet.go (1)
State(28-28)applog/applog.go (1)
GetLogFilePath(375-377)
🔇 Additional comments (1)
webrtc/peer_manager.go (1)
86-86:forceTurnRelayhardcoded totrue.This forces all connections through TURN relay servers. Verify this is intentional for this debug branch, as it will prevent direct peer-to-peer connections and increase latency/server load.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
webrtc/peer_manager.go (4)
164-220: Manual reconnection flags are accessed under inconsistent/no locking
manualReconnIsActiveandremoteManualRRequestare:
- Written under
peersMuinHandleManualReconnectRequest/handleRemoteManualReconnRequest.- Written under
peer.reconnectMuin the manual branch ofhandleReconnection.- Read with no lock in
handleReconnectionandonPeerStateChanged.This inconsistent locking creates data races on these fields and on
peer.connectionstate, especially with concurrent reconnection attempts and state changes.Pick a single mutex to protect these flags (preferably
peer.reconnectMufor reconnection‑related state) and use it consistently:func (p *PeerManager) handleReconnection(playerId uint, forceReconnect bool) { @@ - if !forceReconnect && !peer.manualReconnIsActive && !peer.remoteManualRRequest{ - if peer.IsActive() || (peer.connection != nil && - peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnecting) { + peer.reconnectMu.Lock() + manualActive := peer.manualReconnIsActive + remoteReq := peer.remoteManualRRequest + conn := peer.connection + peer.reconnectMu.Unlock() + + if !forceReconnect && !manualActive && !remoteReq { + if peer.IsActive() || (conn != nil && + conn.ConnectionState() == webrtc.PeerConnectionStateConnecting) { @@ - if !peer.manualReconnIsActive && !peer.remoteManualRRequest{ + if !manualActive && !remoteReq { @@ - if err := peer.ConnectOnce(peer.peerSpecificTurn); err != nil { + if err := peer.ConnectOnce(peer.peerSpecificTurn); err != nil { @@ - peer.reconnectMu.Lock() - peer.manualReconnIsActive = false - peer.remoteManualRRequest = false - peer.reconnectMu.Unlock() + peer.reconnectMu.Lock() + peer.manualReconnIsActive = false + peer.remoteManualRRequest = false + peer.reconnectMu.Unlock()And in
onPeerStateChanged/ manual‑reconn handlers, read/update these flags only while holding the same mutex.Also applies to: 447-476, 724-787
21-23: TURN URL/name handling is fragile and can panic on short hostnames
saveTurnsURLsstill assumes.comand slices strings by fixed indices. Besides being brittle for non‑.comTLDs, the branch:preparedName = preparedName[len(preparedName)-8:]will panic if
len(preparedName) < 8(e.g.turn:aa.com), crashing the adapter. This also feeds intoturnsNameById/GetTurnURLsused by the debug UI.At minimum, guard against short strings and
.comabsence; ideally usenet/urlto parse the host:-import "strings" +import ( + "net/url" + "strings" +) @@ func (p *PeerManager) saveTurnsURLs() { - for i, _ := range(p.turnServer) { - for _,url := range(p.turnServer[i].URLs) { - _, ok := turnsNameById[i+1] - if !ok { - preparedName := " " - index := strings.Index(url, ".com") - - if index != -1 { - preparedName = url[:index] - index = strings.LastIndex(preparedName, ".") - if index != -1 { - preparedName = preparedName[index+1:] - if len(preparedName) > 6 { - preparedName = preparedName[:6] - } - } else { - preparedName = preparedName[len(preparedName) - 8:] - } - } - turnsNameById[i+1] = preparedName - } - allTurnServersUrls = append(allTurnServersUrls, url) - } - } + for i := range p.turnServer { + for _, raw := range p.turnServer[i].URLs { + if _, ok := turnsNameById[i+1]; !ok { + name := "-" + if u, err := url.Parse(raw); err == nil { + host := u.Host + if idx := strings.Index(host, ":"); idx != -1 { + host = host[:idx] + } + parts := strings.Split(host, ".") + if len(parts) >= 2 { + name = parts[len(parts)-2] // second‑level domain + } else if host != "" { + name = host + } + if len(name) > 6 { + name = name[:6] + } + } + turnsNameById[i+1] = name + } + allTurnServersUrls = append(allTurnServersUrls, raw) + } + }Also applies to: 86-92, 108-133, 349-351
321-339:GetAllPeersStatscan panic on peers without an active connectionThis method locks
peersMubut then unconditionally dereferencesp.connection(the per‑peer*webrtc.PeerConnection) for every entry. For peers that are still starting up or already closed,peer.connectioncan be nil, leading to a panic when the debug window calls this.Skip peers with nil connections before calling
GetStats/ConnectionState:func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string, map[string][]int, []string) { @@ - for id,p := range(p.peers) { - idAsString := fmt.Sprintf("%d", id) - allPeersStats[idAsString] = p.connection.GetStats() - connectionStates[idAsString] = p.connection.ConnectionState().String() - turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} - } + for id, peer := range p.peers { + if peer == nil || peer.connection == nil { + continue + } + + idAsString := fmt.Sprintf("%d", id) + allPeersStats[idAsString] = peer.connection.GetStats() + connectionStates[idAsString] = peer.connection.ConnectionState().String() + turnIds[idAsString] = []int{peer.specTurnIdLocal, peer.specTurnIdRemote} + }
645-721:initialConnectionWatcherraces onp.peersand can nil‑derefpeer.connectionInside
initialConnectionWatcher:
p.peers[playerId]is read withoutpeersMu, while other goroutines modifyp.peers, risking “concurrent map read and map write”.peer.connection.ConnectionState()is called without checkingpeer/peer.connectionfor nil, which is possible early in setup or after closure.Wrap map access in
peersMuand snapshot the connection state safely:func (p *PeerManager) initialConnectionWatcher(playerId uint) { @@ - for i := 0; i < keepAliveSec; i++ { - time.Sleep(time.Second) - - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } + for i := 0; i < keepAliveSec; i++ { + time.Sleep(time.Second) + + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var connected bool + if ok && peer != nil && peer.connection != nil && + peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnected { + connected = true + } + p.peersMu.Unlock() + + if connected { + peerConnnectedSecondsTotal++ + }adapter/adapter.go (1)
17-23: GlobalPeerManageris still racy between adapter startup and the debug UI
PeerManageris written inStart()(likely from a background goroutine) and read from the debug window viaGetPeerManager()with no synchronization. Even a single flip fromnil→non‑nil without a happens‑before edge is a data race in Go and can break under the race detector.Either avoid the package‑level variable and pass
*PeerManagerinto the UI, or guard access with a mutex/atomic.Value. For example:+import "sync" + -var PeerManager *webrtc.PeerManager +var ( + peerManagerMu sync.RWMutex + peerManager *webrtc.PeerManager +) @@ - peerManager := webrtc.NewPeerManager( + peerManager := webrtc.NewPeerManager( @@ - PeerManager = peerManager + peerManagerMu.Lock() + peerManager = peerManager + peerManagerMu.Unlock() @@ -func GetPeerManager() *webrtc.PeerManager { - return PeerManager -} +func GetPeerManager() *webrtc.PeerManager { + peerManagerMu.RLock() + defer peerManagerMu.RUnlock() + return peerManager +}Also applies to: 120-132, 211-213
dbgwnd/dbgwnd.go (5)
117-125: Reconnect button closures capture the loop index by referenceEach button’s
Command(func() { reconnectBtnClicked(i) })closes over the loop variablei. In Go, all closures created in this loop will see the final valuei == 15, so every button will trigger the samebuttonID.Capture the index in a new local per iteration:
- for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + for i := 0; i < 15; i++ { + idx := i + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
151-156: CheckbuttonVariablehandling and string comparison are likely wrongYou pass plain bools to
Variable(...)and later comparerawLogsChkButton.Variable()/hideDebugChkButton.Variable()to"true". Tk/Tk9 typically uses"1"/"0"(or a Tcl var name) under the hood, so these comparisons may never be true and raw/debug toggles won’t behave as expected.Either wire real Go bools, or at least compare to
"1":Preferred (bind Go bools):
-var rawLogsChButtonVar bool -var hideDebugButtonVar bool +var rawLogsChButtonVar bool +var hideDebugButtonVar bool @@ - rawLogsChkButton = logFrame.Checkbutton(Txt("Show unformatted logs"), Variable(rawLogsChButtonVar), + rawLogsChkButton = logFrame.Checkbutton(Txt("Show unformatted logs"), Variable(&rawLogsChButtonVar), Onvalue(true), Offvalue(false), Command(func() { rawLogsChkButtonPressed() })) @@ - hideDebugChkButton = logFrame.Checkbutton(Txt("Hide DEBUG"), Variable(hideDebugButtonVar), + hideDebugChkButton = logFrame.Checkbutton(Txt("Hide DEBUG"), Variable(&hideDebugButtonVar), Onvalue(true), Offvalue(false), Command(func() { hideDebugChkButtonPressed() })) @@ func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { - showRawLogs = true - } else { - showRawLogs = false - } + showRawLogs = rawLogsChButtonVar } @@ func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { - hideDebug = true - } else { - hideDebug = false - } + hideDebug = hideDebugButtonVar }Also applies to: 207-223
296-309: Log parsing stops permanently on the first short/blank lineBoth branches of
formatLogLinesbreak the loop whenlen(st) < 5. Because the file is split on\n, you’ll always see empty entries, and the first one stops processing forever—new log lines never get added torawLogs/formattedLogs.Change the
breaktocontinueso short/blank lines are skipped but later entries still process:func formatLogLines() { if showRawLogs { for i, st := range logStrings { @@ - if len(st) < 5 { - break - } + if len(st) < 5 { + continue + } @@ } else { for i, st := range logStrings { @@ - if len(st) < 5 { - break - } + if len(st) < 5 { + continue + }Also applies to: 351-365, 367-379
549-556: Guard against shortturnIdsslices when resolving TURN names
turnIds[id]is indexed at[0]and[1]without checking the slice length. IfGetAllPeersStats()ever returns a shorter slice for a peer (bug, partial data, or future change), this will panic and bring down the debug UI.Defensively check lengths before indexing:
if tIds, ok := turnIds[id]; ok { - if tIds[0] != 0 { - localTurnName = turnsNameById[tIds[0]] - } - if tIds[1] != 0 { - remoteTurnName = turnsNameById[tIds[1]] - } + if len(tIds) > 0 && tIds[0] != 0 { + localTurnName = turnsNameById[tIds[0]] + } + if len(tIds) > 1 && tIds[1] != 0 { + remoteTurnName = turnsNameById[tIds[1]] + } }
589-601: Reconnect button state tracking is incorrect;idsToDisablecleared too earlyTwo issues in
refreshButtons:
reconnectButtonsEnabledis incremented inside the loop instead of being set tonumOfIds, so it drifts away from the actual number of active IDs and the guard becomes meaningless.idsToDisableis set tonilinside the loop, so only the firstsortedIds[i]is ever matched; subsequent IDs inidsToDisableare ignored.Suggested fix:
func refreshButtons() { numOfIds := len(sortedIds) if reconnectButtonsEnabled != numOfIds { - for i,b := range(reconnectButtons){ - reconnectButtonsEnabled += 1 + for i, b := range reconnectButtons { if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds } @@ - if len(idsToDisable) > 0 || len(disabledBtnsTimers) > 0 { - for i,_ := range(disabledBtnsTimers) { + if len(idsToDisable) > 0 || len(disabledBtnsTimers) > 0 { + for id := range disabledBtnsTimers { - if disabledBtnsTimers[i] > 0 { - disabledBtnsTimers[i] -= 1 + if disabledBtnsTimers[id] > 0 { + disabledBtnsTimers[id] -= 1 } else { - delete(disabledBtnsTimers, i) + delete(disabledBtnsTimers, id) } } - for i,_ := range(sortedIds) { - if slices.Contains(idsToDisable, sortedIds[i]){ - disabledBtnsTimers[sortedIds[i]] = disableRecButnOnManualRec + for i := range sortedIds { + if slices.Contains(idsToDisable, sortedIds[i]) { + disabledBtnsTimers[sortedIds[i]] = disableRecButnOnManualRec } if _, ok := disabledBtnsTimers[sortedIds[i]]; ok { reconnectButtons[i].Configure((State("disabled"))) } else { reconnectButtons[i].Configure((State("normal"))) } - idsToDisable = nil } + idsToDisable = nil } }Also applies to: 611-632
🧹 Nitpick comments (1)
adapter/adapter.go (1)
68-82: Backoff condition is dead; simplify to explicit fixed delayThe comment says you want a fixed 1s reconnect delay, but
if backoff < 0*time.Second { backoff *= 2 }is never true withbackoff := time.Second, so this code is effectively dead and confusing.Either remove the unused growth logic or just use a literal in
time.After:- backoff := time.Second + backoff := time.Second @@ - case <-time.After(backoff): - // Better don't increase delay and just use fixed 1 sec value - // Might be critical in some situations when you want to restore conn as fast as possible - if backoff < 0*time.Second { - backoff *= 2 - } + case <-time.After(backoff): + // Fixed 1s delay; no exponential backoff on reconnect. }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
adapter/adapter.go(5 hunks)dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)icebreaker/events.go (2)
CandidatesMessage(50-54)BaseEvent(23-28)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(51-65)
dbgwnd/dbgwnd.go (5)
adapter/adapter.go (3)
PeerManager(20-20)GetPeerManager(211-213)GetNicknames(215-220)webrtc/peer_manager.go (1)
PeerManager(51-65)webrtc/peer.go (1)
Peer(23-56)moho/packet.go (1)
State(28-28)applog/applog.go (1)
GetLogFilePath(375-377)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webrtc/peer_manager.go (1)
548-578: GlobalmanualReconnectionMessagestill risks stale data.While
RecipientIDis now updated at line 757 before sending, the globalmanualReconnectionMessageis populated with the first peer's session and then reused. If the session description becomes stale or the first peer disconnects, subsequent manual reconnection requests may send invalid data.Consider constructing a fresh message for each request:
func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { // ... - manualReconnectionMessage.BaseEvent.RecipientID = &playerId - err := p.icebreakerClient.SendEvent(manualReconnectionMessage) + msg := icebreaker.CandidatesMessage{ + BaseEvent: icebreaker.BaseEvent{ + EventType: icebreaker.EventKindCandidates, + GameID: p.gameId, + SenderID: p.localUserId, + RecipientID: &playerId, + }, + Candidates: []webrtc.ICECandidate{}, // empty = manual reconnect signal + } + err := p.icebreakerClient.SendEvent(msg)
♻️ Duplicate comments (13)
adapter/adapter.go (2)
20-22: GlobalPeerManagerstill lacks synchronization with readers.The past review comment about the race condition between
Start()writingPeerManagerandGetPeerManager()reading it remains unaddressed. While pointer assignment is atomic at the machine level, this is still a data race under Go's memory model.Consider using
atomic.Valueor a mutex as previously suggested:+import "sync/atomic" + -var PeerManager *webrtc.PeerManager +var peerManager atomic.Value // stores *webrtc.PeerManager
211-213:GetPeerManagerreturns the global without synchronization.This accessor reads the global
PeerManagerthat is written from a different goroutine inStart(). Without synchronization, this is a data race.dbgwnd/dbgwnd.go (6)
104-111: Closure captures loop variableiby reference — all buttons will callreconnectBtnClicked(15).The closure
func() { reconnectBtnClicked(i) }capturesiby reference. After the loop completes, all 15 button callbacks will use the final value ofi(15).for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
199-215: CheckbuttonVariable()returns"1"/"0", not"true"/"false".Tcl/Tk stores boolean values as
"1"and"0". The comparisons against"true"will always fail.func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { + if rawLogsChkButton.Variable() == "1" { showRawLogs = true } else { showRawLogs = false } } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { + if hideDebugChkButton.Variable() == "1" { hideDebug = true } else { hideDebug = false } }
345-360: Usingbreakinstead ofcontinuestops log parsing on first short line.Log files often have blank lines (especially the last line after a newline). Using
breakstops all further processing when a short line is encountered, preventing new log entries from reaching the UI.if len(st) < 5 { - break + continue }Apply this fix in both the raw logs branch (line 346) and formatted logs branch (line 359).
540-547: No bounds check ontIdsslice before indexing.If
turnIds[id]returns a slice shorter than 2 elements, accessingtIds[0]ortIds[1]will panic.if tIds, ok := turnIds[id]; ok { - if tIds[0] != 0 { + if len(tIds) > 0 && tIds[0] != 0 { localTurnName = turnsNameById[tIds[0]] } - if tIds[1] != 0 { + if len(tIds) > 1 && tIds[1] != 0 { remoteTurnName = turnsNameById[tIds[1]] } }
580-602:reconnectButtonsEnabledis incremented but never reset, breaking the optimization.Line 583 checks
reconnectButtonsEnabled != numOfIds, but line 586 incrementsreconnectButtonsEnabledinside the loop without ever resetting it. After the first run, this counter will exceednumOfIdsand the logic breaks.func refreshButtons() { numOfIds := len(sortedIds) if reconnectButtonsEnabled != numOfIds { // normal buttons for i,b := range(reconnectButtons){ - reconnectButtonsEnabled += 1 if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds
621-641:idsToDisable = nilinside loop clears the slice after first iteration.Setting
idsToDisable = nilon line 640 inside the loop prevents subsequent iterations from checking againstidsToDisable.for i,_ := range(sortedIds) { if slices.Contains(idsToDisable, sortedIds[i]){ disabledBtnsTimers[sortedIds[i]] = disableRecButnOnManualRec } // ... button state logic ... - idsToDisable = nil } + idsToDisable = nil }webrtc/peer_manager.go (5)
108-133: TURN URL parsing assumes.comTLD.The hardcoded
.comcheck will fail for servers using other TLDs (.org,.io,.net, country codes, etc.), producing incorrect or empty names.Consider using
net/urlfor proper parsing or at minimum handle the missing.comcase more gracefully.
331-336:GetAllPeersStatscan panic on nilpeer.connection.Peers that are still starting up have
connection == nil. The loop dereferencesp.connectionwithout a nil check, which will panic when the debug UI requests stats.for id,p := range(p.peers) { + if p == nil || p.connection == nil { + continue + } idAsString := fmt.Sprintf("%d", id) allPeersStats[idAsString] = p.connection.GetStats() connectionStates[idAsString] = p.connection.ConnectionState().String() turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} }
645-646: Globalwatchersmap risks cross-session contamination.The
watchersmap is package-level and is not cleared when a newPeerManageris created. If the adapter restarts or multiple games run, stale entries could prevent new watchers from being created for the same player IDs.Consider making
watchersan instance field ofPeerManageror clearing it in the constructor.
678-685:initialConnectionWatcherreadsp.peerswithout holdingpeersMu.This causes a data race with goroutines that modify
p.peers(e.g.,addPeerIfMissing,removePeer). Additionally,peer.connectionmay be nil and is dereferenced without checking.for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) + p.peersMu.Lock() - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { peerConnnectedSecondsTotal += 1 - } }
827-829: Modulo logic can yield 0 and cause out-of-range index panic.When
numOfReconnsis a multiple oftotalCombinations(e.g., 8 when there are 4 combinations), the normalization producesnumOfReconns == 0. This then causesp.turnServer[numOfReconns - 1]to index at-1, panicking.Use 1-based modulo:
if numOfReconns > totalCombinations { - numOfReconns = numOfReconns - totalCombinations * (numOfReconns / totalCombinations) + // Keep numOfReconns in [1, totalCombinations] + numOfReconns = ((numOfReconns - 1) % totalCombinations) + 1 }
🧹 Nitpick comments (3)
adapter/adapter.go (2)
77-81: Dead code: backoff multiplier never executes.The condition
backoff < 0*time.Secondis alwaysfalsesincebackoffstarts attime.Second(positive) and is never decremented. Thebackoff *= 2on line 80 will never run.If the intent is to keep a fixed 1-second delay (as the comment suggests), remove the dead code:
case <-time.After(backoff): - // Better don't increase delay and just use fixed 1 sec value - // Might be critical in some situations when you want to restore conn as fast as possible - if backoff < 0*time.Second { - backoff *= 2 - } + // Using fixed 1 sec delay for fast reconnection }
155-177: Nickname storage occurs inside the loop, potentially overwriting on each iteration.The
if playerId != "" && nickname != ""check and storage happens inside thefor i, item := rangeloop. Once both values are set, subsequent iterations still execute the storage block repeatedly. Consider breaking out of the loop or moving the storage after the loop:if i == 1 { nickname = v } } - if playerId != "" && nickname != "" { - mu.Lock() - userNicknames[playerId] = nickname - mu.Unlock() - } } + if playerId != "" && nickname != "" { + mu.Lock() + userNicknames[playerId] = nickname + mu.Unlock() + } }dbgwnd/dbgwnd.go (1)
25-50: Global mutable state creates tight coupling and testing difficulties.All UI state is held in package-level variables. This makes the module hard to test and could cause issues if the debug window is ever closed and reopened.
Consider encapsulating state in a struct for cleaner lifecycle management, though this may be acceptable for a debug-only tool.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
adapter/adapter.go(5 hunks)dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(51-65)
dbgwnd/dbgwnd.go (4)
adapter/adapter.go (3)
PeerManager(20-20)GetPeerManager(211-213)GetNicknames(215-223)webrtc/peer_manager.go (1)
PeerManager(51-65)moho/packet.go (1)
State(28-28)applog/applog.go (1)
GetLogFilePath(375-377)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (4)
adapter/adapter.go (1)
215-223:GetNicknamesnow correctly returns a snapshot copy.The implementation now creates a copy of the map while holding the mutex, addressing the previous review concern about exposing the live map.
webrtc/peer_manager.go (3)
748-775: Mutex is now properly unlocked on all paths.The
HandleManualReconnectRequestfunction correctly unlockspeersMuin all branches (lines 762, 767, 772), addressing the previous review concern.
793-811: Mutex handling inhandleRemoteManualReconnRequestis now correct.All code paths properly unlock
peersMu(lines 800, 806, 809), fixing the previously flagged issue.
813-870:preparePeerForManualReconnnow manages its own mutex correctly.The function acquires and releases
peersMuinternally (lines 815, 869), which is safe since callers now release the lock before calling this function.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
webrtc/peer_manager.go (5)
321-339: Critical: Unguarded nilpeer.connectiondereference.The past review comment about guarding against nil
peer.connectionwas not addressed. At lines 333-334, the code unconditionally callsp.connection.GetStats()andp.connection.ConnectionState()without checking ifpeerorpeer.connectionis nil. Peers still starting up or closing can have nil connections, causing a panic.Apply this fix:
func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string, map[string][]int, []string) { p.peersMu.Lock() defer p.peersMu.Unlock() allPeersStats := make(map[string]webrtc.StatsReport) connectionStates := make(map[string]string) turnIds := make(map[string][]int) idsToDisable := p.disableRecBtnPeers p.disableRecBtnPeers = nil - for id,p := range(p.peers) { + for id, peer := range p.peers { + if peer == nil || peer.connection == nil { + continue + } + idAsString := fmt.Sprintf("%d", id) - allPeersStats[idAsString] = p.connection.GetStats() - connectionStates[idAsString] = p.connection.ConnectionState().String() - turnIds[idAsString] = []int{p.specTurnIdLocal, p.specTurnIdRemote} + allPeersStats[idAsString] = peer.connection.GetStats() + connectionStates[idAsString] = peer.connection.ConnectionState().String() + turnIds[idAsString] = []int{peer.specTurnIdLocal, peer.specTurnIdRemote} } return allPeersStats, connectionStates, turnIds, idsToDisable }
21-22: Global TURN state still accumulates across restarts.The past review comment about clearing
allTurnServersUrlsandturnsNameByIdat the start ofsaveTurnsURLs()was not addressed. If the adapter restarts within the same process, these package-level variables will retain old entries and append duplicates.Apply this fix at the start of
saveTurnsURLs():func (p *PeerManager) saveTurnsURLs() { + allTurnServersUrls = nil + turnsNameById = make(map[int]string) for i, _ := range(p.turnServer) {
649-725: Critical: Race condition and nil dereference ininitialConnectionWatcher.The past review comment about the race condition on
p.peersand nilpeer.connectiondereference was not addressed. At lines 685-688, the code readsp.peers[playerId]without holdingpeersMu(causing concurrent map access races with other goroutines that modifypeers), and callspeer.connection.ConnectionState()without checking for nil (which will panic during peer setup or closure).Apply this fix:
for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + }
649-650: Globalwatchersmap risks cross-session contamination.The past review comment about moving
watchersto be an instance field ofPeerManagerwas not addressed. These package-level variables persist across adapter restarts or multiple games, causing stale entries to block new watchers for the same player IDs.Consider making
watchersan instance field:type PeerManager struct { // ... existing fields ... + watchers map[uint]bool + watcherMutex sync.Mutex }Then initialize in
NewPeerManager:peerManager := PeerManager{ // ... existing initialization ... + watchers: make(map[uint]bool), }And update all references to use
p.watchersandp.watcherMutexinstead of the global variables.
213-217: Mutex inconsistency for reconnection flags persists.The past review comment about inconsistent mutex usage was marked as addressed, but the issue remains. At lines 213-216,
peer.manualReconnIsActiveandpeer.remoteManualRRequestare modified underreconnectMu, but at lines 758 and 802 inHandleManualReconnectRequestandhandleRemoteManualReconnRequest, these same fields are accessed underpeersMu. This creates a race condition.Consistently use
peersMufor these fields. Apply this fix:} else { applog.Info("Connecting to peer with specific TURN", zap.Uint("playerId", playerId)) if err := peer.ConnectOnce(peer.peerSpecificTurn); err != nil { applog.Error("Peer connection with specific TURN failed", zap.Uint("peer", playerId), zap.Error(err)) } - peer.reconnectMu.Lock() + p.peersMu.Lock() peer.manualReconnIsActive = false peer.remoteManualRRequest = false - peer.reconnectMu.Unlock() + p.peersMu.Unlock() }
🧹 Nitpick comments (5)
webrtc/peer_manager.go (5)
752-779: Approve explicit mutex unlocks, butdeferwould be safer.The explicit
Unlock()calls at lines 766, 771, and 776 correctly handle all code paths, addressing the past critical mutex deadlock issue. However, the current pattern is error-prone if future modifications add early returns.Consider using
deferfor more robust cleanup:func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { peerAsString := fmt.Sprintf("%d", playerId) p.peersMu.Lock() + defer p.peersMu.Unlock() + peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive && !peer.remoteManualRRequest { peer.manualReconnIsActive = true p.disableRecBtnPeers = append(p.disableRecBtnPeers, peerAsString) manualReconnectionMessage.BaseEvent.RecipientID = &playerId + p.peersMu.Unlock() // Explicitly unlock before calling preparePeerForManualReconn + err := p.icebreakerClient.SendEvent(manualReconnectionMessage) if err != nil { applog.Error("Failed to send manualReconnectionMessage to peer " + peerAsString, zap.Error(err)) } - p.peersMu.Unlock() p.preparePeerForManualReconn(playerId) go p.scheduleManualReconnection(playerId) - } else { - p.peersMu.Unlock() - applog.Debug("Manual reconnection is already in process. Skipping local request. Peer: " + peerAsString) } - } else { - applog.Error("Can't initiate manual reconnection: No such peer " + peerAsString) - p.peersMu.Unlock() } - }Note: With this pattern, you'd need to ensure
preparePeerForManualReconnhas its own locking since the defer will re-unlock when the function returns (which would be a double-unlock without the explicit unlock before the call).
797-815: Explicit unlocks correctly handle all paths.The explicit
Unlock()calls at lines 804, 810, and 813 correctly address all code paths, resolving the past critical mutex issue. The implementation is correct.As with
HandleManualReconnectRequest, consider usingdeferfor more maintainable code.
367-381: Time-based reconnection guard is a documented workaround.The 120-second guard before scheduling reconnection (lines 369-381) prevents interference with initial connection attempts. This is explicitly documented as a workaround alongside
initialConnectionWatcher. While functional, this indicates the underlying connection startup race should be properly resolved in the future.Consider investigating and fixing the root cause of the initial connection race so these workarounds can be removed.
256-263: Empty candidates as reconnection signal is a protocol hack.The code at lines 258-263 uses an empty
Candidatesarray as an implicit signal for manual reconnection requests. This is explicitly documented as "a dirty hook" (line 257) and should be replaced with a proper message type.Add a dedicated
EventKindManualReconnectmessage type toicebreaker/events.goand update both the sender and receiver to use explicit message types rather than overloading theCandidatesMessagesemantics.
727-750: Non-functional feature included in codebase.The
HandleManualNoRelayReconnRequestmethod is documented as not working (line 730: "Doesn't work"). Including non-functional code in the codebase increases maintenance burden and confusion.Either remove this method entirely if it's confirmed non-functional, or if it's experimental/work-in-progress, add a clear TODO comment explaining what needs to be fixed and consider returning an error to callers indicating the feature is not yet implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (4)
webrtc/peer_manager.go (4)
552-582: Baseline message correctly updated before each send.The initialization of
manualReconnectionMessageas a baseline template is correct, and line 761 inHandleManualReconnectRequestproperly updates theRecipientIDbefore each send, addressing the past issue about stale recipients.
164-220: Clear separation of normal and manual reconnection paths.The
forceReconnectparameter and the conditional logic at lines 175-181 and 184-217 cleanly separate forced/manual reconnection from normal reconnection. The TURN ID reset at lines 191-192 for normal reconnection is appropriately documented.
454-484: State change handling correctly guards against concurrent reconnection.The guard at line 455 that checks
!peer.manualReconnIsActiveprevents the automatic reconnection logic from interfering with manual reconnection flows. The logic is sound.However, this depends on the
manualReconnIsActiveflag being correctly synchronized (see the mutex inconsistency issue at lines 213-217). Ensure that issue is resolved to guarantee this guard works reliably across goroutines.
817-875: Index calculations are mathematically sound and produce no out-of-bounds indices.The verification confirms that the complex TURN server index calculations at lines 847-858 are correct. Testing across 2, 3, and 4 TURN server configurations with multiple reconnection attempts shows all leader and secondary server indices remain within valid bounds. The modulo normalization at line 832 correctly cycles through the valid range, and the mixed-server calculation logic properly maps all theoretical combinations to actual server indices. No edge cases produce invalid indices.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
adapter/adapter.go (1)
20-22: GlobalPeerManageraccess is still racy between adapter startup and the debug UI
PeerManageris assigned inStart()(Line 138) and read viaGetPeerManager()(Lines 218-220) without any synchronization. SinceStart()runs in its own goroutine while the debug UI callsGetPeerManager()concurrently, this is a Go data race even though pointer assignment is atomic, and it can yield undefined behavior.Either avoid the package-level global and inject
*webrtc.PeerManagerinto the UI, or guard the global with synchronization (e.g. anRWMutexoratomic.Value), for example:@@ -import ( +import ( @@ - "time" - "sync" + "time" + "sync" ) @@ -var PeerManager *webrtc.PeerManager +var ( + peerManagerMu sync.RWMutex + peerManager *webrtc.PeerManager +) @@ - peerManager := webrtc.NewPeerManager( + pm := webrtc.NewPeerManager( @@ - PeerManager = peerManager + peerManagerMu.Lock() + peerManager = pm + peerManagerMu.Unlock() @@ - peerManager.Start() + pm.Start() @@ func GetPeerManager() *webrtc.PeerManager { - return PeerManager + peerManagerMu.RLock() + defer peerManagerMu.RUnlock() + return peerManager }Please double-check call sites to ensure
GetPeerManager()is never used before initialization or from unexpected goroutines after applying the synchronization.Also applies to: 138-139, 218-220
🧹 Nitpick comments (4)
adapter/adapter.go (4)
77-82: Backoff condition is effectively dead code; simplify for clarity
backoffis initialized totime.Secondand never made negative, so the conditionif backoff < 0*time.Second { backoff *= 2 }will never be true. The current effect is a fixed 1s delay, which matches the comment but makes the condition misleading.You can simplify to make intent explicit, e.g.:
- backoff := time.Second + const backoff = time.Second @@ - case <-time.After(backoff): - // Better don't increase delay and just use fixed 1 sec value - // Might be critical in some situations when you want to restore conn as fast as possible - if backoff < 0*time.Second { - backoff *= 2 - } + case <-time.After(backoff): + // Fixed 1s delay to reconnect as fast as possible }Verify that no previous behavior depended on exponential backoff before this change.
87-96: Filtering a TURN server withcontinueleaves zero-value entries and can mask “no usable servers”When you
continueon a filtered server (Lines 89-95), the corresponding slot inturnServer := make([]pionwebrtc.ICEServer, len(sessionGameResponse.Servers))is left as a zero-valueICEServer. This means:
- You may pass entries with empty
URLstoNewPeerManager.- If all servers are filtered (e.g. only the hard-coded IP is present),
len(turnServer)is still non-zero even though you effectively have no usable TURN/STUN servers, and you never log that situation.Consider building
turnServerwithappendand tracking only allowed servers, and explicitly logging when the filtered list is empty, e.g.:- turnServer := make([]pionwebrtc.ICEServer, len(sessionGameResponse.Servers)) - for i, server := range sessionGameResponse.Servers { + var turnServer []pionwebrtc.ICEServer + for _, server := range sessionGameResponse.Servers { // Hardcoded until we remove FAF's cotrun from the list if len(server.Urls) > 0 { if strings.Contains(server.Urls[0], "139.162.142.250") { continue } } - turnServer[i] = pionwebrtc.ICEServer{ + ts := pionwebrtc.ICEServer{ @@ - for j, url := range server.Urls { + for j, url := range server.Urls { // for Java being Java reasons we unfortunately raped the URLs and need to convert it back. - turnServer[i].URLs[j] = strings.ReplaceAll(url, "://", ":") + ts.URLs[j] = strings.ReplaceAll(url, "://", ":") } + turnServer = append(turnServer, ts) } + + if len(turnServer) == 0 { + applog.Error("No usable stun/turn servers after filtering (all filtered or misconfigured)") + }Confirm that pion/webrtc tolerates zero-value
ICEServerentries today and that you won’t rely on their behavior if this remains unrefactored.
162-184: Tighten nickname extraction loop and minimize repeated lockingThe nickname caching logic works, but it does extra work:
- Once both
playerIdandnicknameare found,mu.Lock()/mu.Unlock()and the map write still run for every remaining arg in the loop.- The reliance on positional indices (
i == 1for nickname,i == 2for playerId) is implicit; a brief comment would help future readers.You can simplify and lock only once, after the loop:
- if cmd == "JoinGame" || cmd == "ConnectToPeer"{ + if cmd == "JoinGame" || cmd == "ConnectToPeer" { nickname := "" playerId := "" for i, item := range parsedMsg.GetArgs() { @@ case string: if i == 1 { nickname = v } } - if playerId != "" && nickname != "" { - mu.Lock() - userNicknames[playerId] = nickname - mu.Unlock() - } } + // Args layout: [cmd, nickname, playerId, ...] + if playerId != "" && nickname != "" { + mu.Lock() + userNicknames[playerId] = nickname + mu.Unlock() + } }This keeps behavior the same but avoids repeated locking and clarifies the assumed arg ordering.
Please confirm the arg positions for
JoinGame/ConnectToPeerare stable and documented somewhere, or add a reference comment if they are.
222-230:GetNicknamessnapshot approach looks correct; consider minor style tweak
GetNicknamesnow copiesuserNicknamesundermuand returns the copy, which resolves the earlier map race against concurrent writers. The logic is sound.For minor style/robustness, you could use
deferfor the unlock:func GetNicknames() map[string]string { - mu.Lock() - nnames := make(map[string]string, len(userNicknames)) + mu.Lock() + defer mu.Unlock() + + nnames := make(map[string]string, len(userNicknames)) for k, v := range userNicknames { - nnames[k] = v - } - mu.Unlock() + nnames[k] = v + } return nnames }Run with
-racewhen exercising the debug UI nickname view to confirm there are no remaining map races.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
adapter/adapter.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(51-65)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webrtc/peer_manager.go (1)
164-220: Manual reconnection flags are not consistently synchronized, leading to data races
manualReconnIsActiveandremoteManualRRequestare read and written under different or no locks:
handleReconnection(lines 164–220) reads both flags andpeer.connectionafter releasingpeersMu.onPeerStateChanged(451–487) readspeer.manualReconnIsActivewithout any lock.HandleManualReconnectRequest,HandleManualNoRelayReconnRequest,handleRemoteManualReconnRequest, andpreparePeerForManualReconnupdate these fields underpeersMuorreconnectMu.This inconsistent locking introduces races on the same fields and on
peer.connectionstate, especially under heavy reconnect churn or manual reconnect spam.
- Decide on a single mutex (preferably
peersMu) that protectsmanualReconnIsActive,remoteManualRRequest, and any associated peer TURN-selection fields.- In
handleReconnection, keeppeersMulong enough to snapshot the relevant booleans and connection state into locals, then operate on those locals after unlocking:func (p *PeerManager) handleReconnection(playerId uint, forceReconnect bool) { @@ - p.peersMu.Lock() - peer, ok := p.peers[playerId] - if !ok || peer.IsDisabled() { - p.peersMu.Unlock() - return - } - p.peersMu.Unlock() - - if !forceReconnect && !peer.manualReconnIsActive && !peer.remoteManualRRequest{ - if peer.IsActive() || (peer.connection != nil && - peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnecting) { + p.peersMu.Lock() + peer, ok := p.peers[playerId] + if !ok || peer.IsDisabled() { + p.peersMu.Unlock() + return + } + manualReconn := peer.manualReconnIsActive + remoteManual := peer.remoteManualRRequest + isActive := peer.IsActive() + conn := peer.connection + p.peersMu.Unlock() + + if !forceReconnect && !manualReconn && !remoteManual { + if isActive || (conn != nil && + conn.ConnectionState() == webrtc.PeerConnectionStateConnecting) { applog.Info("Peer already active/connecting, skipping reconnection", zap.Uint("peer", playerId)) return } }
- In
onPeerStateChangedand the manual recon handlers, always read or modify these flags while holding the same mutex, and avoid mutating them underreconnectMuif you standardize onpeersMu.This will remove races on these flags and on
peer.connectionusage.Also applies to: 451-487, 758-785, 803-821, 823-881
♻️ Duplicate comments (7)
dbgwnd/dbgwnd.go (5)
103-111: Fix closure capture so each RECONNECT button uses its own indexThe
Command(func() { reconnectBtnClicked(i) })closure capturesiby reference, so all buttons will callreconnectBtnClickedwith the final loop value. Capture the index into a new local per iteration.- for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + for i := 0; i < 15; i++ { + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
147-152: Checkbutton value handling is broken;Variable()never equals"true"Tk checkbuttons with
Onvalue(true), Offvalue(false)end up using"1"/"0"(and hereVariableis also wired incorrectly), so the comparisons against"true"never succeed and the toggles don't work.- rawLogsChkButton = logFrame.Checkbutton(Txt("Show unformatted logs"), Variable(rawLogsChButtonVar), - Onvalue(true), Offvalue(false), Command(func() { rawLogsChkButtonPressed() })) - - hideDebugChkButton = logFrame.Checkbutton(Txt("Hide DEBUG"), Variable(hideDebugButtonVar), - Onvalue(true), Offvalue(false), Command(func() { hideDebugChkButtonPressed() })) + rawLogsChkButton = logFrame.Checkbutton( + Txt("Show unformatted logs"), + Variable(&rawLogsChButtonVar), + Onvalue(true), + Offvalue(false), + Command(func() { rawLogsChkButtonPressed() }), + ) + + hideDebugChkButton = logFrame.Checkbutton( + Txt("Hide DEBUG"), + Variable(&hideDebugButtonVar), + Onvalue(true), + Offvalue(false), + Command(func() { hideDebugChkButtonPressed() }), + ) @@ func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { - showRawLogs = true - } else { - showRawLogs = false - } + showRawLogs = rawLogsChButtonVar } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { - hideDebug = true - } else { - hideDebug = false - } + hideDebug = hideDebugButtonVar }Also applies to: 199-215
336-364: Blank/short log lines stop processing future logsUsing
breakonlen(st) < 5means the first empty/short entry (e.g. final split fromstrings.Split) permanently stops log ingestion; later lines never reach the UI.- if len(st) < 5 { - break - } + if len(st) < 5 { + continue + } @@ - if len(st) < 5 { - break - } + if len(st) < 5 { + continue + }
582-645: Reconnect button state book‑keeping is wrong;idsToDisablecleared too earlyTwo issues here:
reconnectButtonsEnabledis incremented inside the loop, so the guard!= numOfIdsnever stabilizes.idsToDisable = nilinside theforloop clears the slice after the first ID, so only the first matching peer gets its timers set.func refreshButtons() { numOfIds := len(sortedIds) - if reconnectButtonsEnabled != numOfIds { - // normal buttons - for i,b := range(reconnectButtons){ - reconnectButtonsEnabled += 1 - if i < numOfIds { - b.Configure((State("normal"))) - } else { - b.Configure((State("disabled"))) - } - } + if reconnectButtonsEnabled != numOfIds { + // normal buttons + for i, b := range reconnectButtons { + if i < numOfIds { + b.Configure(State("normal")) + } else { + b.Configure(State("disabled")) + } + } + reconnectButtonsEnabled = numOfIds } @@ - if len(idsToDisable) > 0 || len(disabledBtnsTimers) > 0 { - for i,_ := range(disabledBtnsTimers) { - if disabledBtnsTimers[i] > 0 { - disabledBtnsTimers[i] -= 1 + if len(idsToDisable) > 0 || len(disabledBtnsTimers) > 0 { + for id := range disabledBtnsTimers { + if disabledBtnsTimers[id] > 0 { + disabledBtnsTimers[id] -= 1 } else { - delete(disabledBtnsTimers, i) + delete(disabledBtnsTimers, id) } } - for i,_ := range(sortedIds) { - if slices.Contains(idsToDisable, sortedIds[i]){ - disabledBtnsTimers[sortedIds[i]] = disableRecButnOnManualRec + for i := range sortedIds { + id := sortedIds[i] + if slices.Contains(idsToDisable, id) { + disabledBtnsTimers[id] = disableRecButnOnManualRec } // normal buttons - if _, ok := disabledBtnsTimers[sortedIds[i]]; ok { + if _, ok := disabledBtnsTimers[id]; ok { reconnectButtons[i].Configure((State("disabled"))) } else { reconnectButtons[i].Configure((State("normal"))) } - - idsToDisable = nil } + idsToDisable = nil } }
217-243: HandleAtoierrors instead of silently treating bad IDs as 0Both reconnect handlers ignore
strconv.Atoierrors and may end up calling reconnection APIs with player ID 0 if parsing ever fails.func reconnNoRelaytBtnClicked(buttonID int){ if len(sortedIds) > buttonID{ if pm != nil { @@ - if answer == "yes"{ - playerID, _ := strconv.Atoi(sortedIds[buttonID]) - pm.HandleManualNoRelayReconnRequest(uint(playerID)) - } + if answer == "yes"{ + playerID, err := strconv.Atoi(sortedIds[buttonID]) + if err != nil { + return + } + pm.HandleManualNoRelayReconnRequest(uint(playerID)) + } @@ func reconnectBtnClicked(buttonID int){ if len(sortedIds) > buttonID{ if pm != nil { - playerID, _ := strconv.Atoi(sortedIds[buttonID]) - pm.HandleManualReconnectRequest(uint(playerID)) + playerID, err := strconv.Atoi(sortedIds[buttonID]) + if err != nil { + return + } + pm.HandleManualReconnectRequest(uint(playerID)) } } }webrtc/peer_manager.go (2)
21-23: Harden TURN URL parsing and reset global TURN caches per PeerManager
saveTurnsURLscurrently assumes.comand does manual slicing that can panic (e.g."turn:a.com"makespreparedNameshorter than 8, sopreparedName[len(preparedName)-8:]underflows). It also never clearsallTurnServersUrls/turnsNameById, so multipleNewPeerManagerinstances accumulate stale/duplicate entries.-import ( - "context" - "faf-pioneer/applog" - "faf-pioneer/gpgnet" - "faf-pioneer/icebreaker" - "faf-pioneer/launcher" - "faf-pioneer/util" - "github.com/pion/webrtc/v4" - "go.uber.org/zap" - "net" - "sync" - "time" - "fmt" - "strconv" - "math" - "strings" -) +import ( + "context" + "faf-pioneer/applog" + "faf-pioneer/gpgnet" + "faf-pioneer/icebreaker" + "faf-pioneer/launcher" + "faf-pioneer/util" + "github.com/pion/webrtc/v4" + "go.uber.org/zap" + "net" + "net/url" + "sync" + "time" + "fmt" + "strconv" + "math" + "strings" +) @@ -func (p *PeerManager) saveTurnsURLs() { - for i, _ := range(p.turnServer) { - for _,url := range(p.turnServer[i].URLs) { - _, ok := turnsNameById[i+1] - if !ok { - preparedName := " " - index := strings.Index(url, ".com") - - if index != -1 { - preparedName = url[:index] - index = strings.LastIndex(preparedName, ".") - if index != -1 { - preparedName = preparedName[index+1:] - if len(preparedName) > 6 { - preparedName = preparedName[:6] - } - } else { - preparedName = preparedName[len(preparedName) - 8:] - } - } - turnsNameById[i+1] = preparedName - } - allTurnServersUrls = append(allTurnServersUrls, url) - } - } -} +func (p *PeerManager) saveTurnsURLs() { + // Reset global TURN state per manager instance. + allTurnServersUrls = nil + turnsNameById = make(map[int]string) + + for i := range p.turnServer { + for _, urlStr := range p.turnServer[i].URLs { + if _, ok := turnsNameById[i+1]; !ok { + name := "unknown" + if u, err := url.Parse(urlStr); err == nil && u.Host != "" { + host := strings.Split(u.Host, ":")[0] + parts := strings.Split(host, ".") + if len(parts) >= 2 { + name = parts[len(parts)-2] + } else if len(parts) == 1 { + name = parts[0] + } + if len(name) > 6 { + name = name[:6] + } + } + turnsNameById[i+1] = name + } + allTurnServersUrls = append(allTurnServersUrls, urlStr) + } + } +}Also applies to: 108-133, 353-355
653-731:initialConnectionWatchercan still nil‑derefpeer.connectionYou now guard access with
peersMu, butpeer.connectioncan still be nil for peers that are starting up or already torn down, sopeer.connection.ConnectionState()can panic.- for i := 0; i < keepAliveSec; i++ { - time.Sleep(time.Second) - - p.peersMu.Lock() - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } - p.peersMu.Unlock() + for i := 0; i < keepAliveSec; i++ { + time.Sleep(time.Second) + + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + }
🧹 Nitpick comments (3)
webrtc/peer_manager.go (3)
321-343: HoldingpeersMuacrossGetStatsmay block other operations
GetAllPeersStatsholdspeersMuwhile callingpeer.connection.GetStats()for every peer. IfGetStatsis slow or blocks internally, it will stall any other goroutine that needspeersMu(peer add/remove, reconnections).Consider snapshotting the peer pointers under the lock, then calling
GetStatsafter unlocking:func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string, map[string][]int, []string) { - p.peersMu.Lock() - defer p.peersMu.Unlock() - - allPeersStats := make(map[string]webrtc.StatsReport) - connectionStates := make(map[string]string) - turnIds := make(map[string][]int) - idsToDisable := p.disableRecBtnPeers - p.disableRecBtnPeers = nil - - for id,peer := range(p.peers) { + p.peersMu.Lock() + snapshot := make(map[uint]*Peer, len(p.peers)) + for id, peer := range p.peers { + snapshot[id] = peer + } + idsToDisable := p.disableRecBtnPeers + p.disableRecBtnPeers = nil + p.peersMu.Unlock() + + allPeersStats := make(map[string]webrtc.StatsReport) + connectionStates := make(map[string]string) + turnIds := make(map[string][]int) + + for id, peer := range snapshot { if peer == nil || peer.connection == nil{ continue } @@ return allPeersStats, connectionStates, turnIds, idsToDisable }
371-385: Reconnection skip-on-start logic is reasonable but deserves a guardrailThe “don’t reconnect within the first 120s” workaround avoids fighting initial connection attempts, but it’s hard-coded and silently skips reconnection. If this ever misfires, debugging will be painful.
- Consider making the
120seconds threshold a named constant with a brief comment, or log the exact creation time and threshold so it’s obvious in logs why reconnection was skipped.
556-587: GlobalmanualReconnectionMessageis a brittle protocol hackUsing a single package-global
manualReconnectionMessageas a template, then mutatingRecipientIDbefore each send, is fragile and non-obvious, especially if you ever support multiple concurrent manual reconnects.
- Instead of a global, build a fresh empty-candidates message per request in
HandleManualReconnectRequest(and dropmanualReconnectionMessageentirely), using the same BaseEvent fields andRecipientIDfor the target peer.- Leave
onPeerCandidatesGatheredto only send real candidates and not manage manual recon state; keep the “len(candidates)==0 → manual recon” hook only on the receiving side.This will make the manual recon protocol easier to reason about and safer to evolve.
Also applies to: 758-776, 803-815
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dbgwnd/dbgwnd.go (4)
webrtc/peer_manager.go (1)
PeerManager(51-65)adapter/adapter.go (3)
PeerManager(20-20)GetPeerManager(218-220)GetNicknames(222-230)moho/packet.go (1)
State(28-28)applog/applog.go (1)
GetLogFilePath(375-377)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (10)
adapter/adapter.go (1)
231-233: GlobalPeerManageraccess is racy.This was flagged in a past review and remains unaddressed.
PeerManageris written inStart()and read viaGetPeerManager()without synchronization.dbgwnd/dbgwnd.go (5)
109-116: Closure captures loop variable by reference — all buttons trigger withi=15.This was flagged in a past review and remains unaddressed. All 15 buttons will call
reconnectBtnClicked(15).for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) }
205-221: CheckbuttonVariable()comparison uses wrong string value.This was flagged in a past review. Tcl/Tk stores boolean as
"1"/"0", not"true"/"false". The comparisons at lines 207 and 216 will always be false.func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { + if rawLogsChkButton.Variable() == "1" { showRawLogs = true } else { showRawLogs = false } } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { + if hideDebugChkButton.Variable() == "1" { hideDebug = true } else { hideDebug = false } }
242-249: Strconv error is silently ignored.This was flagged in a past review. If
strconv.Atoifails,playerIDwill be 0, triggering unintended behavior.
251-268: Listbox selection uses only the first character of the ID.This was flagged in a past review.
selectedId[0]indexes a single byte, not the full string.func infoListIdSelected(){ selected := connIdListBox.Curselection() if len(selected) > 0 { selectedId := connIdListBox.Get( strconv.Itoa(selected[0])) if len(selectedId) > 0 { - stat, ok := allPeersStats[selectedId[0]] + stat, ok := allPeersStats[selectedId] if ok { connInfoView.Clear() - title := "Connection ID: " + selectedId[0] + " Time: " + time.Now().Format("15:04:05") + title := "Connection ID: " + selectedId + " Time: " + time.Now().Format("15:04:05")
351-366: Don't bail out of log parsing on blank lines.This was flagged in a past review. Using
breakinstead ofcontinuestops processing forever on the first blank entry.if len(st) < 5 { - break + continue }Apply the same fix at line 364-366.
webrtc/peer_manager.go (4)
21-22: Global TURN state will persist across adapter restarts.This was flagged in a past review. Consider clearing these at the start of
saveTurnsURLs().
108-133: TURN URL parsing remains fragile with hardcoded.comassumption.This was flagged in a past review. The code will fail for other TLDs (
.org,.io,.net, etc.).
653-654: Globalwatchersmap risks cross-session contamination.This was flagged in a past review. Consider making
watchersan instance field ofPeerManager.
686-695: Potential nil pointer dereference onpeer.connection.While the mutex is now acquired (addressing the race),
peer.connectioncan still be nil during peer setup. Line 691 callspeer.connection.ConnectionState()without a nil check.for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) p.peersMu.Lock() if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ + if peer.connection != nil && peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnected { peerConnnectedSecondsTotal += 1 } } p.peersMu.Unlock()
🧹 Nitpick comments (1)
adapter/adapter.go (1)
94-99: Hardcoded IP exclusion is fragile.Filtering by hardcoded IP
139.162.142.250will break if the server IP changes. Consider using a hostname or configuration-based approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
adapter/adapter.go(5 hunks)dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(51-65)
webrtc/peer_manager.go (2)
adapter/adapter.go (1)
PeerManager(20-20)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (7)
adapter/adapter.go (2)
235-243:GetNicknamescorrectly returns a snapshot copy.The previous review concern about returning the live map has been addressed—this now returns a deep copy while holding the mutex.
175-196: The code snippet referenced does not exist in the repository.The review comment references code at adapter/adapter.go lines 175-196, but adapter.go is only 175 lines long and ends with
return nil. The nickname extraction logic with the userNicknames map, mutex operations, and the loop parsing both playerId and nickname from message arguments is not present anywhere in the current codebase.This suggests the review was created for code that either has not been committed, was removed, or is located in a different file than specified.
Likely an incorrect or invalid review comment.
dbgwnd/dbgwnd.go (2)
551-560: Guard against shortturnIdsslices improved but still has edge case.The length check
len(tIds) > 1at line 552 is good, but iftIdshas exactly 1 element, the code skips both assignments. Consider checkinglen(tIds) > 0for the first element access.The fix addresses the past review concern. The current logic safely handles short slices.
653-654:idsToDisable = nilcorrectly moved outside the loop.The past review concern has been addressed—the assignment is now after the loop completes.
webrtc/peer_manager.go (3)
758-785: Mutex handling inHandleManualReconnectRequestis now correct.The previous review concerns about missing unlocks have been addressed—all code paths now properly release
peersMu.
803-821: Mutex handling inhandleRemoteManualReconnRequestis now correct.All code paths properly release
peersMu.
823-881:preparePeerForManualReconnmutex and modulo logic are now correct.The modulo normalization
((numOfReconns - 1) % totalCombinations) + 1correctly keeps values in[1, totalCombinations]. The mutex is acquired at the start and released at the end.
| manualReconnectionMessage.BaseEvent.RecipientID = &playerId | ||
| err := p.icebreakerClient.SendEvent(manualReconnectionMessage) |
There was a problem hiding this comment.
manualReconnectionMessage is mutated in place, causing race conditions.
The global manualReconnectionMessage has its RecipientID modified at line 767, then sent at line 768. If two manual reconnections happen concurrently for different peers, the RecipientID can be overwritten before SendEvent is called.
Create a local copy instead of mutating the global:
- manualReconnectionMessage.BaseEvent.RecipientID = &playerId
- err := p.icebreakerClient.SendEvent(manualReconnectionMessage)
+ msg := manualReconnectionMessage
+ msg.BaseEvent.RecipientID = &playerId
+ err := p.icebreakerClient.SendEvent(msg)🤖 Prompt for AI Agents
In webrtc/peer_manager.go around lines 767-768, the global
manualReconnectionMessage is being mutated (its BaseEvent.RecipientID set) right
before SendEvent which creates a race if multiple reconnections run
concurrently; instead, make a local copy of manualReconnectionMessage (clone the
struct and its BaseEvent) and set RecipientID on that copy (use a new local
playerId variable and pointer) and pass the copy to p.icebreakerClient.SendEvent
so the global template is never modified.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (15)
dbgwnd/dbgwnd.go (7)
109-116: Closure captures loop variable by reference — all buttons will trigger withi=15.The closure
func() { reconnectBtnClicked(i) }capturesiby reference. All 15 buttons will callreconnectBtnClicked(15).Apply this fix:
for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
205-221: CheckbuttonVariable()returns"1"/"0", not"true"/"false".Tcl/Tk stores boolean values as
"1"and"0". The comparisonsrawLogsChkButton.Variable() == "true"andhideDebugChkButton.Variable() == "true"will always fail.func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { + if rawLogsChkButton.Variable() == "1" { showRawLogs = true } else { showRawLogs = false } } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { + if hideDebugChkButton.Variable() == "1" { hideDebug = true } else { hideDebug = false } }
242-249: Strconv error is silently ignored.If
strconv.Atoifails,playerIDwill be 0, which could trigger unintended behavior.func reconnectBtnClicked(buttonID int){ if len(sortedIds) > buttonID{ if pm != nil { - playerID, _ := strconv.Atoi(sortedIds[buttonID]) + playerID, err := strconv.Atoi(sortedIds[buttonID]) + if err != nil { + return + } pm.HandleManualReconnectRequest(uint(playerID)) } } }
251-268: Listbox selection uses only the first character of the ID.
selectedId[0]indexes a single byte of the string (e.g.,"12"→'1'), causing lookups and display for wrong IDs.func infoListIdSelected(){ selected := connIdListBox.Curselection() if len(selected) > 0 { selectedId := connIdListBox.Get( strconv.Itoa(selected[0])) if len(selectedId) > 0 { - stat, ok := allPeersStats[selectedId[0]] + stat, ok := allPeersStats[selectedId] if ok { connInfoView.Clear() - title := "Connection ID: " + selectedId[0] + " Time: " + time.Now().Format("15:04:05") + title := "Connection ID: " + selectedId + " Time: " + time.Now().Format("15:04:05") connInfoView.Insert(END, title, "", "\n") connInfoView.Insert(END, godump.DumpJSONStr(stat) +" ", "", "\n") } } } }
342-419: Don't bail out of log parsing on blank lines.Every log line ends with a newline, so
strings.Splityields empty strings. Because the loopbreaks whenlen(st) < 5, processing stops permanently. Replacebreakwithcontinue.if len(st) < 5 { - break + continue }Apply this fix at both line 352 and line 365.
593-615:reconnectButtonsEnabledis incremented incorrectly.The variable is incremented on every iteration instead of being set to the final count after the loop.
func refreshButtons() { numOfIds := len(sortedIds) if reconnectButtonsEnabled != numOfIds { // normal buttons for i,b := range(reconnectButtons){ - reconnectButtonsEnabled += 1 if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds
634-655:idsToDisable = nilinside loop clears the slice prematurely.Setting
idsToDisable = nilon line 653 inside the loop clears it after the first iteration, preventing other IDs from being checked.for i,_ := range(sortedIds) { if slices.Contains(idsToDisable, sortedIds[i]){ disabledBtnsTimers[sortedIds[i]] = disableRecButnOnManualRec } // normal buttons if _, ok := disabledBtnsTimers[sortedIds[i]]; ok { reconnectButtons[i].Configure((State("disabled"))) } else { reconnectButtons[i].Configure((State("normal"))) } - - idsToDisable = nil } + idsToDisable = nil } }cmd/faf-adapter/main.go (1)
18-55: SIGINT no longer shuts the process down properly.By moving startup into a goroutine while the main goroutine blocks in
dbgwnd.CreateMainWindow(), signal handling is broken:
- Pressing Ctrl+C cancels
ctxbut the Tk main loop keeps running- Closing the window doesn't call
cancel(), so defers (applog.Shutdown) won't runos.Exit(0)at line 52 will abruptly terminate when the adapter finishes, potentially before the UI is closedConsider restructuring so that:
- The signal context is in the main goroutine
- The UI loop can be interrupted when
ctx.Done()fires- Window close triggers
cancel()for graceful shutdownadapter/adapter.go (2)
20-22: GlobalPeerManageris racy between adapter.Start and the debug UI.
PeerManageris written inStart()at line 151 and read fromGetPeerManager()at line 232 without synchronization. This is a data race in Go's memory model.Guard with a mutex or
atomic.Value:+var peerManagerMu sync.RWMutex + func GetPeerManager() *webrtc.PeerManager { - return PeerManager + peerManagerMu.RLock() + defer peerManagerMu.RUnlock() + return PeerManager }And wrap the assignment at line 151:
+ peerManagerMu.Lock() PeerManager = peerManager + peerManagerMu.Unlock()
82-86: Dead code: backoff growth condition is always false.The condition
backoff < 0*time.Secondcan never be true sincebackoffis initialized totime.Second.case <-time.After(backoff): - // Better don't increase delay and just use fixed 1 sec value - // Might be critical in some situations when you want to restore conn as fast as possible - if backoff < 0*time.Second { - backoff *= 2 - } + // Use fixed 1 sec delay for faster reconnection }webrtc/peer_manager.go (5)
21-22: Global TURN state will persist across adapter restarts.
allTurnServersUrlsandturnsNameByIdare package-level and accumulate data. If the adapter restarts, stale entries persist.Clear these at the start of
saveTurnsURLs():func (p *PeerManager) saveTurnsURLs() { + allTurnServersUrls = nil + turnsNameById = make(map[int]string) for i, _ := range(p.turnServer) {
108-133: TURN URL parsing remains fragile with hardcoded.comassumption.The code assumes
.comTLD and will fail for.org,.io,.net, country codes, etc.Consider using
net/urlfor proper URL parsing as suggested in the past review.
686-695:initialConnectionWatchercan panic on nilpeer.connection.At line 691,
peer.connection.ConnectionState()is called without checking ifpeer.connectionis nil. The connection may not be initialized yet.for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) p.peersMu.Lock() if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ + if peer.connection != nil && peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnected { peerConnnectedSecondsTotal += 1 } } p.peersMu.Unlock()
653-654: Globalwatchersmap risks cross-session contamination.The
watchersmap is package-level. If the adapter restarts, stale entries persist.Consider making
watchersan instance field ofPeerManageror clearing it when a newPeerManageris created.
767-768:manualReconnectionMessageis mutated in place, causing race conditions.The global message has its
RecipientIDmodified at line 767, then sent at line 768. Concurrent manual reconnections can overwrite the recipient.Create a local copy:
- manualReconnectionMessage.BaseEvent.RecipientID = &playerId - err := p.icebreakerClient.SendEvent(manualReconnectionMessage) + msg := manualReconnectionMessage + msg.BaseEvent.RecipientID = &playerId + err := p.icebreakerClient.SendEvent(msg)
🧹 Nitpick comments (2)
webrtc/peer.go (1)
154-165: DuplicateIsDisabled()check inConnectOnce.Lines 155-157 and 159-162 both check
IsDisabled()with the same effect. The second check with the "during reconnection" comment appears redundant since no reconnection has started yet at this point.func (p *Peer) ConnectOnce(iceServers []webrtc.ICEServer) error { if p.IsDisabled() { return errors.New("peer is disabled") } - // If peed are disconnected/died/disabled while reconnecting, just gave up. - if p.IsDisabled() { - return errors.New("peer is disabled during reconnection") - } - return p.reconnect(iceServers) }adapter/adapter.go (1)
94-99: Hardcoded IP filter for TURN server removal.The comment indicates this is temporary until FAF's coturn is removed from the list. Consider adding a TODO or tracking issue reference.
Is there a tracking issue for removing this hardcoded filter?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
adapter/adapter.go(5 hunks)cmd/faf-adapter/main.go(1 hunks)dbgwnd/dbgwnd.go(1 hunks)webrtc/peer.go(3 hunks)webrtc/peer_manager.go(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
webrtc/peer.go (2)
applog/context.go (1)
FromContext(10-12)applog/applog.go (1)
Debug(110-123)
webrtc/peer_manager.go (2)
adapter/adapter.go (1)
PeerManager(20-20)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(51-65)
dbgwnd/dbgwnd.go (4)
adapter/adapter.go (4)
PeerManager(20-20)GetPeerManager(231-233)GetRegion(245-247)GetNicknames(235-243)webrtc/peer_manager.go (1)
PeerManager(51-65)moho/packet.go (1)
State(28-28)applog/applog.go (1)
GetLogFilePath(375-377)
🔇 Additional comments (10)
dbgwnd/dbgwnd.go (1)
551-560: LGTM - turnIds slice length check added.The length check
if len(tIds) > 1properly guards against short slices before accessing indices 0 and 1.webrtc/peer.go (3)
49-56: New peer fields for manual reconnection support look good.The new fields (
creationTimeSeconds,numOfManualReconns,manualReconnIsActive,remoteManualRRequest,peerSpecificTurn,specTurnIdLocal,specTurnIdRemote) are appropriately scoped for tracking per-peer reconnection state.
143-149: Field initializations are correct.The new fields are initialized with sensible defaults:
creationTimeSecondswith current Unix time, and reconnection flags/counters to 0/false.
399-405: Log level change from Error to Debug is appropriate.Send failures on the data channel can be transient (e.g., channel closing during shutdown). Using Debug level avoids log noise.
adapter/adapter.go (1)
235-243: LGTM -GetNicknamesnow returns a snapshot copy.The function properly copies the map while holding the mutex, preventing race conditions.
webrtc/peer_manager.go (5)
86-86:forceTurnRelayis hardcoded totrue.This overrides the
sessionInfo.MapForceTurnRelayconfiguration. If this is intentional for debugging, consider adding a comment. Otherwise, this should use the session configuration.Is forcing TURN relay intentional for this debug window PR?
321-343: LGTM -GetAllPeersStatsproperly guards against nil peers/connections.The nil checks at line 332-334 prevent panics when peers are still initializing.
758-785: LGTM - Mutex handling inHandleManualReconnectRequestis correct.All code paths now properly release
peersMu(lines 772, 777, 782).
803-821: LGTM - Mutex handling inhandleRemoteManualReconnRequestis correct.All code paths now properly release
peersMu.
837-839: LGTM - Modulo normalization fix is correct.The formula
((numOfReconns - 1) % totalCombinations) + 1properly keepsnumOfReconnsin the range [1, totalCombinations], preventing the out-of-range index issue.
| func reconnNoRelaytBtnClicked(buttonID int){ | ||
| if len(sortedIds) > buttonID{ | ||
| if pm != nil { | ||
| name := "" | ||
| if n, ok := playerNames[sortedIds[buttonID]]; ok { | ||
| name = n | ||
| } | ||
| answer := MessageBox(Detail("Are you sure you want to try to connect directly? This will expose your real IP address to peer: " + | ||
| sortedIds[buttonID] + " " + name), Type("yesno")) | ||
| connInfoView.Insert(END, answer +" ", "", "\n") | ||
|
|
||
| if answer == "yes"{ | ||
| playerID, _ := strconv.Atoi(sortedIds[buttonID]) | ||
| pm.HandleManualNoRelayReconnRequest(uint(playerID)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Strconv error ignored in reconnNoRelaytBtnClicked.
Same issue as reconnectBtnClicked - if parsing fails, playerID will be 0.
func reconnNoRelaytBtnClicked(buttonID int){
if len(sortedIds) > buttonID{
if pm != nil {
// ... message box code ...
if answer == "yes"{
- playerID, _ := strconv.Atoi(sortedIds[buttonID])
+ playerID, err := strconv.Atoi(sortedIds[buttonID])
+ if err != nil {
+ return
+ }
pm.HandleManualNoRelayReconnRequest(uint(playerID))
}
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func reconnNoRelaytBtnClicked(buttonID int){ | |
| if len(sortedIds) > buttonID{ | |
| if pm != nil { | |
| name := "" | |
| if n, ok := playerNames[sortedIds[buttonID]]; ok { | |
| name = n | |
| } | |
| answer := MessageBox(Detail("Are you sure you want to try to connect directly? This will expose your real IP address to peer: " + | |
| sortedIds[buttonID] + " " + name), Type("yesno")) | |
| connInfoView.Insert(END, answer +" ", "", "\n") | |
| if answer == "yes"{ | |
| playerID, _ := strconv.Atoi(sortedIds[buttonID]) | |
| pm.HandleManualNoRelayReconnRequest(uint(playerID)) | |
| } | |
| } | |
| } | |
| } | |
| func reconnNoRelaytBtnClicked(buttonID int){ | |
| if len(sortedIds) > buttonID{ | |
| if pm != nil { | |
| name := "" | |
| if n, ok := playerNames[sortedIds[buttonID]]; ok { | |
| name = n | |
| } | |
| answer := MessageBox(Detail("Are you sure you want to try to connect directly? This will expose your real IP address to peer: " + | |
| sortedIds[buttonID] + " " + name), Type("yesno")) | |
| connInfoView.Insert(END, answer +" ", "", "\n") | |
| if answer == "yes"{ | |
| playerID, err := strconv.Atoi(sortedIds[buttonID]) | |
| if err != nil { | |
| return | |
| } | |
| pm.HandleManualNoRelayReconnRequest(uint(playerID)) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In dbgwnd/dbgwnd.go around lines 223 to 240, the call to strconv.Atoi ignores
the returned error so a malformed sortedIds[buttonID] will produce playerID 0
and proceed incorrectly; change the code to capture and check the error from
strconv.Atoi, handle the failure by logging or showing an error message and
returning early instead of calling pm.HandleManualNoRelayReconnRequest with an
invalid ID, and only convert and call the handler when strconv.Atoi succeeds.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
webrtc/peer_manager.go (8)
164-217: Manual reconnection flags are accessed under inconsistent locking
handleReconnectionreadspeer.manualReconnIsActiveandpeer.remoteManualRRequestwithout any lock, but elsewhere these fields are mutated under eitherpeersMu(e.g.HandleManualReconnectRequest,handleRemoteManualReconnRequest) orpeer.reconnectMu(lines 213-215).This reintroduces the inconsistent-locking/data-race problem previously flagged for these fields.
To avoid races and hard-to-reason-about behavior, pick a single mutex to protect
manualReconnIsActiveandremoteManualRRequest(eitherpeersMuorpeer.reconnectMu) and use it consistently for all reads and writes, including here and inonPeerStateChanged.For example, if you choose
peer.reconnectMu:func (p *PeerManager) handleReconnection(playerId uint, forceReconnect bool) { // ... find peer ... - if !forceReconnect && !peer.manualReconnIsActive && !peer.remoteManualRRequest { + peer.reconnectMu.Lock() + manualActive := peer.manualReconnIsActive + remoteReq := peer.remoteManualRRequest + peer.reconnectMu.Unlock() + + if !forceReconnect && !manualActive && !remoteReq { if peer.IsActive() || (peer.connection != nil && peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnecting) { applog.Info("Peer already active/connecting, skipping reconnection", zap.Uint("peer", playerId)) return } } - if !peer.manualReconnIsActive && !peer.remoteManualRRequest { + if !manualActive && !remoteReq { // normal path... } else { // manual TURN path, where you already hold reconnectMu later } }
561-592: GlobalmanualReconnectionMessageis a shared mutable template and introduces races
manualReconnectionMessageis initialized once from the firstonPeerCandidatesGatheredcall and then reused for all manual reconnection requests.Issues:
- It’s package-global;
HandleManualReconnectRequestlater mutatesBaseEvent.RecipientIDin place. Concurrent manual reconnections for different peers can race on that field.- The template is only initialized when
manualReconnectionMessage.GameID == 0. If a user triggers a manual reconnect before any candidates have been gathered, you’ll send a partially zero-valued event (GameID and SenderID both 0).- Using a hidden “fake candidate” message as control signaling is brittle and hard to reason about.
Given the logic in
handleIceBreakerEventonly cares thatlen(Candidates) == 0for manual reconnection, you can avoid the shared global entirely and construct a freshCandidatesMessageper manual request.See concrete suggestion in the comment on
HandleManualReconnectRequestbelow.
21-22: Global TURN URL state persists across sessions and can accumulate stale entries
allTurnServersUrlsandturnsNameByIdare package-level and never cleared, so repeated adapter runs in the same process will append duplicate URLs and retain stale TURN name mappings.Either:
- Reset these in
saveTurnsURLs(e.g.,allTurnServersUrls = nil; turnsNameById = make(map[int]string)), or- Make them fields on
PeerManagerso they are naturally scoped to one instance.
108-133: TURN URL parsing is fragile and can panic on short hostnamesThe
.com-based parsing has two concrete problems:
- If the TURN URL host doesn’t contain
.com(e.g..net,.io, country-code TLDs),preparedNameremains a single space, so UI labels will be empty/odd.- The
elsebranch usespreparedName[len(preparedName)-8:]. IfpreparedNamelength is< 8(e.g.turn.com:3478→turn), this panics with a negative slice bound.Additionally, globals
allTurnServersUrlsandturnsNameByIdare never reset, so callingsaveTurnsURLsmultiple times accumulates duplicates.Consider:
- Reinitializing globals at the start of this function.
- Replacing the ad-hoc string slicing with
net/url+ hostname parsing (as suggested in the earlier review) to support arbitrary TLDs and avoid panics.func (p *PeerManager) saveTurnsURLs() { - for i, _ := range(p.turnServer) { - for _,url := range(p.turnServer[i].URLs) { + allTurnServersUrls = nil + turnsNameById = make(map[int]string) + + for i := range p.turnServer { + for _, url := range p.turnServer[i].URLs { _, ok := turnsNameById[i+1] if !ok { - preparedName := " " - index := strings.Index(url, ".com") - // ... + // TODO: parse url with net/url, extract second-level domain, truncate to 6 chars. + preparedName := "unknown" turnsNameById[i+1] = preparedName } allTurnServersUrls = append(allTurnServersUrls, url) } } }
353-355: TURN URL accessor exposes package-level mutable state
GetTurnURLsreturns the package-levelallTurnServersUrlsandturnsNameById. In combination withsaveTurnsURLsnot clearing these, callers may see stale entries across adapter restarts in the same process.If you keep the globals, consider documenting that the data is effectively process-global and possibly duplicated. Otherwise, prefer moving these into
PeerManagerso you can guarantee they’re tied to a single game/session and easily reset.
464-493: Manual reconnection flag inonPeerStateChangedhas same locking inconsistencyHere
peer.manualReconnIsActiveis read without any synchronization, while other paths mutate it underpeersMuorpeer.reconnectMu.This is the same race/concurrency-ownership problem as in
handleReconnection. Once you decide which mutex should ownmanualReconnIsActive(andremoteManualRRequest), this check should be performed under that mutex as well, e.g.:case webrtc.PeerConnectionStateFailed, webrtc.PeerConnectionStateClosed: - if !peer.manualReconnIsActive { + peer.reconnectMu.Lock() + manualActive := peer.manualReconnIsActive + peer.reconnectMu.Unlock() + + if !manualActive { // schedule reconnection... } else { applog.Debug("Peer is already reconnecting manually. Skipping 'failed or closed' case") }
658-736:initialConnectionWatcherstill has nil-pointer risk and uses global watchers mapEven though the watcher goroutine is currently disabled, the implementation has two issues if it’s ever re-enabled:
- Inside the loop, you dereference
peer.connectionwithout nil checks underpeersMu:if peer, ok := p.peers[playerId]; ok { if peer.connection.ConnectionState().String() == "connected" { peerConnnectedSecondsTotal++ } }If
peerexists butpeer.connectionis nil (early in setup or after close), this will panic.
watchersis a package-level map keyed by player ID. It will retain state across adapter restarts/games in the same process and can block watcher creation for a reused player ID.A safer pattern:
for i := 0; i < keepAliveSec; i++ { time.Sleep(time.Second) - p.peersMu.Lock() - if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ - peerConnnectedSecondsTotal += 1 - } - } - p.peersMu.Unlock() + p.peersMu.Lock() + peer, ok := p.peers[playerId] + var state webrtc.PeerConnectionState + if ok && peer != nil && peer.connection != nil { + state = peer.connection.ConnectionState() + } + p.peersMu.Unlock() + + if state == webrtc.PeerConnectionStateConnected { + peerConnnectedSecondsTotal++ + }And consider scoping
watcherstoPeerManageror clearing it when starting a new game.
763-790: Manual reconnect request mutates global template and does I/O underpeersMuTwo concerns here:
Shared mutable
manualReconnectionMessage
HandleManualReconnectRequestsetsmanualReconnectionMessage.BaseEvent.RecipientID = &playerIdand then sends that struct. As noted earlier, this introduces a data race if two manual reconnections run concurrently for different peers and also relies onmanualReconnectionMessagehaving been initialized viaonPeerCandidatesGathered.You can remove the shared global entirely and construct the control message on demand:
func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { peerAsString := fmt.Sprintf("%d", playerId) p.peersMu.Lock() peer, ok := p.peers[playerId] if ok { if !peer.manualReconnIsActive && !peer.remoteManualRRequest { peer.manualReconnIsActive = true p.disableRecBtnPeers = append(p.disableRecBtnPeers, peerAsString)
manualReconnectionMessage.BaseEvent.RecipientID = &playerIderr := p.icebreakerClient.SendEvent(manualReconnectionMessage)
// Send an explicit "manual reconnect" candidates message with no candidates.msg := icebreaker.CandidatesMessage{BaseEvent: icebreaker.BaseEvent{EventType: icebreaker.EventKindCandidates,GameID: p.gameId,SenderID: p.localUserId,RecipientID: &playerId,},Session: nil,Candidates: nil, // empty slice → manual reconnect signal}p.peersMu.Unlock()err := p.icebreakerClient.SendEvent(msg) if err != nil { applog.Error("Failed to send manualReconnectionMessage to peer "+peerAsString, zap.Error(err)) }
p.peersMu.Unlock() p.preparePeerForManualReconn(playerId) go p.scheduleManualReconnection(playerId)You can then delete `manualReconnectionMessage` and its initialization entirely.
Holding
peersMuduring network I/OIn the current code,
SendEventis called whilepeersMuis still held. IfSendEventblocks (network hiccup, backpressure), it can stall other peer operations and reconnection scheduling. The diff above also addresses this by unlocking before the send.
🧹 Nitpick comments (6)
dbgwnd/dbgwnd.go (2)
422-424: Empty placeholder function.The
setRegion()function is defined but empty. If region setting is needed, this should be implemented before merging.Do you want me to help implement the region setting logic, or should this function be removed if not needed?
551-560: Overly conservative length check may skip valid data.The check
if len(tIds) > 1ensures both indices are safe, but skips processing entirely whenlen == 1. If the slice can have length 1 (e.g., only local TURN is set), that information won't be displayed.Consider processing available data:
if tIds, ok := turnIds[id]; ok { - if len(tIds) > 1 { - if tIds[0] != 0 { - localTurnName = turnsNameById[tIds[0]] - } - if tIds[1] != 0 { - remoteTurnName = turnsNameById[tIds[1]] - } + if len(tIds) > 0 && tIds[0] != 0 { + localTurnName = turnsNameById[tIds[0]] + } + if len(tIds) > 1 && tIds[1] != 0 { + remoteTurnName = turnsNameById[tIds[1]] } }webrtc/peer_manager.go (4)
321-343: Stats collection now guards nil PeerConnections correctly; consider minimizing lock scopeThe nil checks on
peerandpeer.connectionfix the earlier panic risk when the debug window requests stats for peers still starting up. That part looks good.One improvement: you hold
peersMuwhile callingpeer.connection.GetStats(), which is potentially non-trivial work and calls into Pion. To reduce contention onpeersMu, you can snapshot the peers and their connections under the lock, then compute stats outside:func (p *PeerManager) GetAllPeersStats() (map[string]webrtc.StatsReport, map[string]string, map[string][]int, []string) { - p.peersMu.Lock() - defer p.peersMu.Unlock() - - allPeersStats := make(map[string]webrtc.StatsReport) - connectionStates := make(map[string]string) - turnIds := make(map[string][]int) - idsToDisable := p.disableRecBtnPeers - p.disableRecBtnPeers = nil - - for id,peer := range(p.peers) { - if peer == nil || peer.connection == nil{ - continue - } - // ... - } + p.peersMu.Lock() + snapshot := make(map[uint]*Peer, len(p.peers)) + for id, peer := range p.peers { + snapshot[id] = peer + } + idsToDisable := p.disableRecBtnPeers + p.disableRecBtnPeers = nil + p.peersMu.Unlock() + + allPeersStats := make(map[string]webrtc.StatsReport) + connectionStates := make(map[string]string) + turnIds := make(map[string][]int) + + for id, peer := range snapshot { + if peer == nil || peer.connection == nil { + continue + } + // ... + } return allPeersStats, connectionStates, turnIds, idsToDisable }Not critical for the debug window, but improves robustness if stats are polled frequently.
371-387: Age-based reconnection guard looks reasonable but is brittle to system clock changesThe logic to skip reconnection for peers younger than 120 seconds avoids fighting the initial connect (and is clearly marked as a workaround). Two minor considerations:
creationTimeSecondsis presumably based ontime.Now().Unix(). If the system clock jumps backwards,tSincePeerCreationcould go negative. It’s unlikely but would cause the “skip reconnection” guard to fail open.- It might be cleaner to store a
time.Timein the peer and usetime.Since(peer.creationTime).No blocking issues here, especially given the debug/workaround nature; just something to keep in mind if this flows into production.
738-761:HandleManualNoRelayReconnRequestdepends onforceTurnRelaysemantics—please confirm behaviorThis path:
- Sets
peer.forceTurnRelay = falseunderpeersMu.- Then calls
handleReconnection(playerId, true).Whether this actually enforces a “no relay” reconnection depends entirely on how
Peer.ConnectOnceinterpretspeer.forceTurnRelay. From this file, all reconnection paths still pass a TURN server list intoConnectOnce, so it’s non-obvious whether forcing-direct actually happens.Given the inline comment “Add: Doesn’t work.”, this is clearly experimental. I’d recommend either:
- Verifying that
Peer.ConnectOnceand/or the underlying ICE config actually switches to a non-relay policy whenforceTurnRelayis false, or- Clearly marking this as a stub/unused feature so callers (e.g., debug UI) don’t rely on it yet.
792-806:scheduleManualReconnectionassumes a 2s delay is sufficientThe logic is straightforward and functionally fine for a debug feature: sleep 2 seconds, then, if the peer still exists, call
handleReconnection(which will use the manual TURN list prepared earlier).Given this is intentionally heuristic (“assume that second peer received it and even if not, it doesn’t really matter”), no changes are strictly required. Just be aware that in high-latency conditions 2 seconds might be short; you may want this configurable if the feature is promoted to production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (1)
webrtc/peer_manager.go (1)
808-826: Remote manual reconnection handler has consistent locking; behavior looks reasonable
handleRemoteManualReconnRequest:
- Guards
peers[playerId]andpeer.remoteManualRRequestunderpeersMu.- Ensures
preparePeerForManualReconnis called only after unlocking to avoid deadlocks.- Protects
disableRecBtnPeersupdates underpeersMu.Given the broader manual recon locking concerns already raised, this specific function looks correct within that model.
| } | ||
|
|
||
| func refreshLogs() { | ||
| textYpos,_ := strconv.ParseFloat(strings.Split(logView.Yview(), " ")[1], 64) |
There was a problem hiding this comment.
ParseFloat error is silently ignored.
If logView.Yview() returns an unexpected format, the split and parse will fail, leaving textYpos at its zero value (0.0). This will cause the autoscroll check at line 337 to always fail.
Consider handling the error:
- textYpos,_ := strconv.ParseFloat(strings.Split(logView.Yview(), " ")[1], 64)
+ textYpos, err := strconv.ParseFloat(strings.Split(logView.Yview(), " ")[1], 64)
+ if err != nil {
+ textYpos = 0.0 // or handle appropriately
+ }🤖 Prompt for AI Agents
In dbgwnd/dbgwnd.go around line 288, the call textYpos,_ :=
strconv.ParseFloat(strings.Split(logView.Yview(), " ")[1], 64) ignores the
ParseFloat error and assumes the split result exists; validate the split length
and capture the ParseFloat error, and handle it (e.g., log the error and set a
safe default or abort the autoscroll check) so a malformed Yview() string
doesn't leave textYpos at 0.0 and break the autoscroll logic.
| playerName := "-" | ||
| if name, ok := playerNames[id]; ok { | ||
| playerName = name | ||
| } else { | ||
| playerNames = adapter.GetNicknames() | ||
| } |
There was a problem hiding this comment.
Entire playerNames map is overwritten when a single name is missing.
At line 578, when a player name is not found in the cached playerNames map, the code fetches all nicknames and replaces the entire map. This is inefficient and loses any previously cached names.
Consider updating selectively:
playerName := "-"
if name, ok := playerNames[id]; ok {
playerName = name
} else {
- playerNames = adapter.GetNicknames()
+ allNames := adapter.GetNicknames()
+ for k, v := range allNames {
+ playerNames[k] = v
+ }
+ if name, ok := playerNames[id]; ok {
+ playerName = name
+ }
}🤖 Prompt for AI Agents
In dbgwnd/dbgwnd.go around lines 574-579, the code currently replaces the entire
playerNames map when a single id is missing; instead, fetch only the missing
nickname(s) or fetch the full nicknames and merge them into the existing
playerNames map rather than overwriting it. Change the logic so that when a name
is not found you call adapter.GetNicknames() (or adapter.GetNickname(id) if
available), iterate the returned entries to update/insert into playerNames, and
then set playerName from the now-updated map; this preserves previously cached
names and avoids losing existing entries.
| if len(oldPeer) < len(sortedIds) { | ||
| for i,_ := range(sortedIds) { | ||
| if _, ok := oldPeer[sortedIds[i]]; !ok { | ||
| disabledBtnsTimers[sortedIds[i]] = disableRecButnOnNewConnect | ||
| } | ||
| } |
There was a problem hiding this comment.
New peer detection may miss peer replacements.
The condition if len(oldPeer) < len(sortedIds) only detects when the peer count increases. If a peer disconnects and another connects in the same refresh cycle (count unchanged), the new peer won't be detected or added to oldPeer, and won't get the connection timer.
Consider detecting all new peers regardless of count:
- if len(oldPeer) < len(sortedIds) {
- for i,_ := range(sortedIds) {
- if _, ok := oldPeer[sortedIds[i]]; !ok {
- disabledBtnsTimers[sortedIds[i]] = disableRecButnOnNewConnect
- }
+ for i,_ := range(sortedIds) {
+ if _, ok := oldPeer[sortedIds[i]]; !ok {
+ disabledBtnsTimers[sortedIds[i]] = disableRecButnOnNewConnect
+ oldPeer[sortedIds[i]] = true
}
}🤖 Prompt for AI Agents
In dbgwnd/dbgwnd.go around lines 617-622, the current length check only catches
increases in peer count and misses replacements; remove the `if len(oldPeer) <
len(sortedIds)` guard and always iterate over `sortedIds`, and for each id that
is not present in `oldPeer` set `disabledBtnsTimers[id] =
disableRecButnOnNewConnect` (ensure `oldPeer` and `disabledBtnsTimers` maps are
initialized before use). This guarantees newly connected peers are detected even
when total count is unchanged.
| func (p *PeerManager) preparePeerForManualReconn(playerId uint) { | ||
| peerAsString := fmt.Sprintf("%d", playerId) | ||
| p.peersMu.Lock() | ||
| peer, ok := p.peers[playerId] | ||
|
|
||
| if ok { | ||
| peer.numOfManualReconns += 1 | ||
| peer.peerSpecificTurn = nil | ||
|
|
||
| numOfTurns := len(p.turnServer) | ||
| totalCombinations := numOfTurns * numOfTurns | ||
| isLeader := p.localUserId < playerId | ||
| numOfReconns := peer.numOfManualReconns | ||
|
|
||
| if numOfReconns > totalCombinations { | ||
| numOfReconns = ((numOfReconns - 1) % totalCombinations) + 1 | ||
| } | ||
|
|
||
| // Every manual reconnection attempt should give a different pair | ||
| // So players can find a better connection if some TURNs are lagging | ||
| // It tries pairs with same TURNs first, then mix them. 2 servers example: | ||
| // 1. Cloud<->Cloud 2. Xirsys<->Xirsys 3. Cloud<->Xirsys 4. Xirsys<->Cloud | ||
| if numOfReconns <= numOfTurns { | ||
| peer.peerSpecificTurn = append(peer.peerSpecificTurn, p.turnServer[numOfReconns - 1]) | ||
| applog.Info("Specific TURN prepared. Leader: " + strconv.Itoa(numOfReconns) + | ||
| " secondary: " + strconv.Itoa(numOfReconns) + " Peer: " + peerAsString + | ||
| " Number of manual reconnects: "+ strconv.Itoa(peer.numOfManualReconns)) | ||
| peer.specTurnIdLocal = numOfReconns | ||
| peer.specTurnIdRemote = numOfReconns | ||
| } else { | ||
| leaderServerNum := | ||
| int(math.Ceil(float64((numOfReconns - numOfTurns)) / float64((totalCombinations - numOfTurns)/numOfTurns))) | ||
| secondaryServerNum := (numOfReconns - numOfTurns) / leaderServerNum | ||
|
|
||
| for i := 1; i <= numOfTurns; i++ { | ||
| if i == leaderServerNum { | ||
| secondaryServerNum += 1 | ||
| break | ||
| } else if i == secondaryServerNum { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if isLeader { | ||
| peer.peerSpecificTurn = append(peer.peerSpecificTurn, p.turnServer[leaderServerNum - 1]) | ||
| peer.specTurnIdLocal = leaderServerNum | ||
| peer.specTurnIdRemote = secondaryServerNum | ||
| } else { | ||
| peer.peerSpecificTurn = append(peer.peerSpecificTurn, p.turnServer[secondaryServerNum - 1]) | ||
| peer.specTurnIdLocal = secondaryServerNum | ||
| peer.specTurnIdRemote = leaderServerNum | ||
| } | ||
|
|
||
| applog.Info("Specific TURN prepared. Leader:" + strconv.Itoa(leaderServerNum) + | ||
| " secondary: " + strconv.Itoa(secondaryServerNum) + " Peer: " + peerAsString) | ||
| } | ||
| } | ||
| p.peersMu.Unlock() | ||
| } No newline at end of file |
There was a problem hiding this comment.
preparePeerForManualReconn can panic when no TURN servers are configured
The combination logic over TURN servers is clever and, with the 1-based modulo fix, avoids the earlier 0 index problem. However:
- If
len(p.turnServer) == 0, then:totalCombinations := numOfTurns * numOfTurns→ 0.- The normalization
((numOfReconns - 1) % totalCombinations)divides by zero and panics. - Even before that, all later indexing into
p.turnServer[...]would be invalid.
If manual reconnect is only valid when at least one TURN server exists, it’s safer to guard explicitly:
func (p *PeerManager) preparePeerForManualReconn(playerId uint) {
peerAsString := fmt.Sprintf("%d", playerId)
p.peersMu.Lock()
peer, ok := p.peers[playerId]
if ok {
- peer.numOfManualReconns += 1
- peer.peerSpecificTurn = nil
-
- numOfTurns := len(p.turnServer)
+ numOfTurns := len(p.turnServer)
+ if numOfTurns == 0 {
+ applog.Error("Manual reconnection requested but no TURN servers are configured", zap.String("peer", peerAsString))
+ p.peersMu.Unlock()
+ return
+ }
+
+ peer.numOfManualReconns++
+ peer.peerSpecificTurn = nil
+
totalCombinations := numOfTurns * numOfTurns
// ...That makes the invariant explicit and prevents hard panics when configuration is incomplete.
🤖 Prompt for AI Agents
In webrtc/peer_manager.go around lines 828 to 886, add an explicit guard for the
case where len(p.turnServer) == 0 before any math or indexing that assumes at
least one TURN: after computing numOfTurns (and before totalCombinations or any
p.turnServer[...] access), if numOfTurns == 0 clear/set peer.peerSpecificTurn
appropriately, log or warn that manual reconnection is not possible without TURN
servers, unlock the mutex and return early; this prevents the divide-by-zero in
the ((... ) % totalCombinations) normalization and avoids invalid p.turnServer
indexing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
webrtc/peer_manager.go (6)
21-22: Global TURN URL caches are sticky across sessions and parsing is still.com‑specific
allTurnServersUrlsandturnsNameByIdare package‑level and only ever appended to;saveTurnsURLsdoesn’t clear them. If a newPeerManageris created in the same process (adapter restart, tests, multiple games), you’ll accumulate duplicates and stale TURN names, andGetTurnURLswill keep returning them. Additionally, the parsing logic assumes a.comsuffix and may generate poor names for.org,.io, ccTLDs, etc.Consider:
- Resetting both structures at the start of
saveTurnsURLs(e.g.allTurnServersUrls = nil; turnsNameById = make(map[int]string)), and- Switching to
net/url+ host parsing instead of.comsubstring matching as suggested in prior reviews.Also, be mindful that
GetTurnURLsreturns the backing slice and map; callers should treat them as read‑only to avoid accidental mutation.Also applies to: 108-133, 353-355
153-220: Manual reconnection flags use inconsistent locking, causing data races
manualReconnIsActiveandremoteManualRRequestare read and written under different synchronization regimes:
- Written under
peersMuinHandleManualReconnectRequest,HandleManualNoRelayReconnRequest, andhandleRemoteManualReconnRequest.- Written under
reconnectMuin the manual branch ofhandleReconnection.- Read with no mutex in
handleReconnection(conditions at lines 175–185) andonPeerStateChanged(line 464).In Go, this mixed/unsynchronized access constitutes a data race and can lead to stale or inconsistent decisions about whether a peer is already in a manual reconnect flow, especially with concurrent UI/debug actions and WebRTC callbacks. It also makes it hard to reason about when flags are cleared vs. when reconnection scheduling happens.
Concrete suggestions:
- Pick a single mutex to guard these fields across the codebase (likely
peersMu, since you already use it in most handlers), or- Encapsulate their access behind
Peermethods that consistently take a specific lock.
handleReconnectionin particular should either:
- Hold
peersMuwhile inspecting these flags and/or snapshot them under lock into local variables, or- Use a dedicated per‑peer lock inside
Peerfor all reconnection‑state decisions.I’d treat this as a correctness issue even for a debug‑only tool; it will be very hard to debug reconnection behaviour if the flags race.
Also applies to: 462-493, 742-790, 808-885
561-592:manualReconnectionMessageglobal template is still a source of stale IDs and races
manualReconnectionMessageis:
- Initialized once in
onPeerCandidatesGatheredthe first timeGameID == 0, and- Later mutated in place in
HandleManualReconnectRequestby settingBaseEvent.RecipientIDbefore sending.This has several problems (mirroring prior reviews):
- The first candidate sender “wins” and all later manual reconnects reuse that one message as a template, including its
Sessionand any subtle fields you may add in the future.- Mutating a global struct and then sending it concurrently for multiple peers is racy: two manual reconnects can overwrite
RecipientID(and any other fields) between construction andSendEvent.- There’s no guarantee
manualReconnectionMessagehas been initialized before the first manual reconnect request; in that case you’ll send a message with default/zero GameID.Safer approach:
- Drop the global mutable template entirely.
- When you need to send a manual reconnect signal, construct a fresh
CandidatesMessageon the spot with:
Session: nil(or whatever you actually need),Candidates: []webrtc.ICECandidate{}to preserve the “empty = manual reconnect” convention, and- A correctly set
RecipientIDfor the specificplayerId.That keeps the global state out of the equation and removes the data race.
Also applies to: 763-777
462-493:onPeerStateChangedalso reads manual reconnect flag without synchronizationIn the
Failed/Closedbranch you guard reconnection scheduling withif !peer.manualReconnIsActive { ... } else { ... }, but this read is done without any mutex, while the flag is written underpeersMu/reconnectMuelsewhere. This is the same race as inhandleReconnection, but in a WebRTC callback.Once you pick a single mutex or encapsulated accessor for
manualReconnIsActive/remoteManualRRequest, please update this site as well to avoid inconsistent decisions about whether to auto‑schedule reconnection vs. rely on manual flows.
658-659: Global watcher state andinitialConnectionWatcherfragility
watchersandwatcherMutexremain package‑level. Combined with the now‑commented‑out call site, this has a few issues (as previously noted):
- If a new
PeerManageris constructed in the same process, stale entries inwatchersmay block creation of new watchers for the same player IDs.initialConnectionWatcherdereferencespeer.connectionwithout checking for nil; while you protect access withpeersMu,peer.connectionitself may be nil early in startup or after closure, which would panic when callingConnectionState().Given this is already described as a workaround, I’d either:
- Move
watchersintoPeerManagerand reset it per game, and- Harden
initialConnectionWatcherto snapshot the peer under lock and only callConnectionState()if bothpeerandpeer.connectionare non‑nil, ideally comparing against the typed constantwebrtc.PeerConnectionStateConnectedinstead of"connected".If you intend to leave the watcher disabled long‑term, it may be worth clearly marking this as dead/debug code or removing it to avoid bit‑rot.
Also applies to: 668-736
828-885:preparePeerForManualReconnstill assumes at least one TURN server; add an explicit guardAs in prior feedback, this function assumes
len(p.turnServer) > 0:
numOfTurns := len(p.turnServer)totalCombinations := numOfTurns * numOfTurns- Later math and indexing (
(numOfReconns-1) % totalCombinations,p.turnServer[...]) will panic whennumOfTurns == 0due to divide‑by‑zero and invalid indices.Even if your current configs always provide at least one TURN, this is a brittle hidden invariant. Please add an early guard, for example:
func (p *PeerManager) preparePeerForManualReconn(playerId uint) { @@ - if ok { - peer.numOfManualReconns += 1 - peer.peerSpecificTurn = nil - - numOfTurns := len(p.turnServer) - totalCombinations := numOfTurns * numOfTurns + if ok { + numOfTurns := len(p.turnServer) + if numOfTurns == 0 { + applog.Error("Manual reconnection requested but no TURN servers are configured", + zap.String("peer", peerAsString)) + p.peersMu.Unlock() + return + } + + peer.numOfManualReconns++ + peer.peerSpecificTurn = nil + + totalCombinations := numOfTurns * numOfTurnsand then keep the rest of the combination logic unchanged. That makes the assumption explicit and prevents a hard crash if TURN is misconfigured.
🧹 Nitpick comments (7)
webrtc/peer.go (3)
49-57: New peer state fields and initialization look consistentThe added fields for creation time, manual reconnection state, per‑peer TURN IDs, and drop counters are initialized coherently and line up with how
PeerManageruses them (creation age checks, manual reconnect paths, and UI stats). If you keep evolving the manual reconnect logic, consider adding brief comments on which mutex (if any) is expected to guard each of these fields, to avoid future locking inconsistencies betweenPeerandPeerManager.Also applies to: 145-152
403-409: Double‑check demoting send failures to Debug levelChanging the log on
gameDataChannel.Sendfailures from Error to Debug will significantly reduce visibility into persistent outbound send issues. If repeated send failures meaningfully affect gameplay, you may want to:
- Keep this at Warn (or emit a periodic Warn similar to the receive‑side drop counter), while still suppressing per‑packet spam with aggregation, or
- At least ensure the debug window or other tooling surfaces these Debug logs when diagnosing connectivity problems.
Please confirm that losing Error‑level signal here is acceptable for your planned debug workflows.
425-435: Packet drop counting logic is fine; consider structured loggingThe throttled warning after every 100 dropped packets works and will prevent log spam. You might want to:
- Log
numOfDroppedPacketsandpeerIdas structured fields instead of concatenated into the message, to keep logs grep‑ and UI‑friendly.- Slight nit:
dropPacketsLastWarnis effectively a modulo‑100 counter; renaming it (or commenting) could make the intent clearer.Functionally this is OK as‑is.
webrtc/peer_manager.go (4)
36-42: Longer ICE failure and reconnection intervals – confirm behavioural trade‑offBumping
peerFailedTimeoutto 120s andpeerReconnectionIntervalto 20s will make failures take longer to be detected and reconnection attempts less frequent. That can be fine for flaky or high‑latency links, but it also means users may sit on dead connections for up to ~2 minutes.Given this PR is “debug window / not for merging”, please confirm whether these values are intended for production or just for experimentation; if the latter, they probably should be gated or reverted before merging.
51-65: Force‑enabling TURN relay and newdisableRecBtnPeersstateTwo behavioural changes worth calling out:
forceTurnRelayis now alwaystrueinNewPeerManager, which forces relay for all peers regardless of launcher/session settings. That has significant impact (no direct P2P even when possible), so if this is for debugging it should likely not ship unguarded into mainline.disableRecBtnPeersis used as a one‑shot list of peer IDs for the UI to disable manual reconnect buttons. The pattern (append while holdingpeersMu, then read‑and‑clear underpeersMuinGetAllPeersStats) looks sound.I’d recommend at least documenting that
forceTurnRelayis intentionally forced here and should be revisited before promoting this branch.Also applies to: 67-92
251-263: Empty‑candidates hook for manual reconnection is fragile by designInterpreting a
CandidatesMessagewith an emptyCandidatesslice as a manual reconnection signal works as a short‑term hack, but it conflates two distinct concepts into one event type. This can become hard to maintain once you or others add richer signalling.Since you’ve already commented this as a “dirty hook” and plan a dedicated reconnection message, I’d just reinforce that this should be a TODO and not become a long‑term protocol contract.
321-343:GetAllPeersStatsanddisableRecBtnPeersusage looks reasonable
GetAllPeersStats:
- Correctly holds
peersMuwhile iterating the map.- Skips peers with
nilor uninitializedconnection, preventing panics.- Returns connection state strings and per‑peer TURN IDs for the debug UI.
The “one‑shot” pattern for
disableRecBtnPeers(append under lock in the manual/remote manual handlers, then snapshot and clear under lock inGetAllPeersStats) is a clean way to signal the UI which buttons to disable.Only minor thought:
peer.connection.GetStats()is called while holdingpeersMu; ifGetStats()is ever slow or blocking, it will stall other operations that needpeersMu. If that becomes an issue, you could snapshot the peers into a slice under lock, then callGetStats()after releasing it.Also applies to: 64-65, 770-771, 814-815
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
adapter/adapter.go(5 hunks)webrtc/peer.go(4 hunks)webrtc/peer_manager.go(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- adapter/adapter.go
🧰 Additional context used
🧬 Code graph analysis (2)
webrtc/peer.go (2)
applog/context.go (1)
FromContext(10-12)applog/applog.go (2)
Debug(110-123)Warn(95-108)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)applog/applog.go (3)
Info(79-93)Error(125-138)Debug(110-123)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (4)
webrtc/peer_manager.go (4)
164-220: Reconnection flow logic mostly OK; watch reuse withforceReconnectThe split between “normal” reconnects and manual reconnects in
handleReconnectionmakes sense:
- For non‑manual reconnects (
!peer.manualReconnIsActive && !peer.remoteManualRRequest), you short‑circuit when the peer is already active/connecting and reset TURN IDs, then callConnectOncewith the defaultturnServerlist and schedule a retry on failure.- For manual reconnects, you call
ConnectOncewithpeer.peerSpecificTurnand clear the manual flags once done.Given the flag races noted above, just be sure that:
forceReconnectis only used to bypass the “already active/connecting” bail‑out, and does not accidentally bypass the manual gating conditions.peer.peerSpecificTurnis always populated bypreparePeerForManualReconnbefore the manual branch is hit (yourscheduleManualReconnectionsleep helps here, but it’s inherently racy if the system is under heavy load).From a logic perspective this flow looks reasonable once the locking is cleaned up.
365-386: Creation‑time gate for early reconnection is sensibleUsing
peer.creationTimeSecondsto avoid scheduling reconnection in the first 120 seconds after peer creation seems like a pragmatic workaround for slow initial connects (especially given your RU comment). The log message giving the peer age in seconds is also useful for diagnosis.Just ensure that
Peerinstances are always created viaCreatePeer(which populatescreationTimeSeconds) so you don’t end up with zero timestamps and negative/hugetSincePeerCreationvalues.Also applies to: 428-436
792-806:scheduleManualReconnectionsequencing is fine but inherently racyThe “sleep 2 seconds, then call
handleReconnectionif the peer still exists” pattern works well enough for a debug‑oriented feature and matches your comments about assuming the remote side saw the signal.Just be aware that:
- There’s no guarantee
preparePeerForManualReconnhas completed when this fires, and- If the peer disappears in between the existence check and the reconnect call, you’ll still run
handleReconnectionon a stale pointer.If this flow graduates beyond debugging, you may want a slightly more robust handshake (e.g., explicit ack or at least a snapshot of the peer and its
peerSpecificTurnunder lock).
742-761: The referenced functionHandleManualNoRelayReconnRequestdoes not exist in the current codebase at the cited lines (742-761 in a 514-line file)The review comment references code that is not present in the repository. However, the underlying concern about unsynchronized access to
peer.forceTurnRelayis valid:
forceTurnRelayis read without synchronization inPeer.reconnect()(line 155) andPeer.reconnectWebRtcPeer()(lines 216, 222)forceTurnRelayis written without synchronization inPeer.reconnectWebRtcPeer()(line 222) andPeerManager.onPeerStateChanged()(line 361)- A dedicated
reconnectMuexists on thePeerstruct but is not consistently used to guardforceTurnRelaywrites- Lines 364-366 in peer_manager.go show commented-out
reconnectMulocking, suggesting prior awareness of this issueThe data race concern is legitimate if this function or similar manual reconnection logic exists elsewhere, but the specific code snippet and line numbers do not match the current codebase.
Ru players need 30+ seconds on start to establish connection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
adapter/adapter.go (2)
73-87: Backoff growth condition is dead and contradicts the comment
backoffis initialized totime.Secondand never changed, soif backoff < 0*time.Secondis always false and the multiplication never runs, while the comment says “don’t increase delay and just use fixed 1 sec value”.Either remove the condition and multiplication entirely, or delete the unused
backoffvariable and directly usetime.Secondintime.Afterfor clarity:- backoff := time.Second + backoff := time.Second @@ - case <-time.After(backoff): - // Better don't increase delay and just use fixed 1 sec value - // Might be critical in some situations when you want to restore conn as fast as possible - if backoff < 0*time.Second { - backoff *= 2 - } + case <-time.After(backoff): + // Fixed 1s delay for faster reconnection; no exponential backoff.
20-22: GlobalPeerManagerremains racy betweenStart()and debug UI access
PeerManageris written once inStart()and read viaGetPeerManager()with no synchronization. GivenStart()runs in its own goroutine and the debug window can callGetPeerManager()concurrently, this is a data race under Go’s memory model (even though the pointer only changes from nil to non-nil).Please either avoid the package-level global or guard it with a mutex/
atomic.Value, e.g.:+import "sync" + -var PeerManager *webrtc.PeerManager +var ( + peerManagerMu sync.RWMutex + peerManager *webrtc.PeerManager +) @@ - peerManager := webrtc.NewPeerManager( + peerManager := webrtc.NewPeerManager( @@ - PeerManager = peerManager + peerManagerMu.Lock() + peerManager = peerManager + peerManagerMu.Unlock() @@ func GetPeerManager() *webrtc.PeerManager { - return PeerManager + peerManagerMu.RLock() + defer peerManagerMu.RUnlock() + return peerManager }Also applies to: 244-246
webrtc/peer_manager.go (6)
21-23: Global TURN URL cache leaks across sessions and URL parsing is brittle
allTurnServersUrlsandturnsNameByIdare package-level and only ever appended/assigned insaveTurnsURLs(). If a newPeerManageris created in the same process (e.g., multiple games or adapter restarts), old URLs and names linger and new ones are added, producing duplicates and stale data. The heuristic based onstrings.Index(url, ".com")and substring slicing is also fragile for non-.comTLDs and short hosts.Consider:
- Resetting the globals at the start of
saveTurnsURLs():func (p *PeerManager) saveTurnsURLs() { - for i, _ := range(p.turnServer) { + allTurnServersUrls = nil + turnsNameById = make(map[int]string) + + for i := range p.turnServer {
- And longer term, replacing the
.comsubstring logic withnet/urlparsing and proper host extraction (as suggested in earlier reviews) so the display names are robust across different TURN hostnames.Also applies to: 108-133, 321-343, 353-355
164-217: Manual reconnection flags use inconsistent locking and are read without synchronization
peer.manualReconnIsActiveandpeer.remoteManualRRequestare:
- Written under
peersMuinHandleManualReconnectRequestandhandleRemoteManualReconnRequest.- Written under
peer.reconnectMuin the manual branch ofhandleReconnection.- Read without any lock in
handleReconnection(both the “already active/connecting” gate and the manual/normal path split) and inonPeerStateChanged.This violates Go’s race rules and can lead to:
- Data races on these bools between reconnection goroutines and the WebRTC state-change callback.
- Inconsistent behavior where some reconnection paths think manual recon is active and others do not.
- Hard-to-reproduce bugs where automatic reconnection runs concurrently with a manual recon attempt.
A safer pattern is:
- Pick a single mutex (ideally
peersMu) as the owner for these two fields.- Only read/write
manualReconnIsActiveandremoteManualRRequestwhile holding that mutex.- Avoid using
peer.reconnectMufor them; keepreconnectMuscoped toreconnectionScheduled/ low-level reconnect sequencing.For example, in
handleReconnection’s manual branch:- } else { - applog.Info("Connecting to peer with specific TURN", zap.Uint("playerId", playerId)) - if err := peer.ConnectOnce(peer.peerSpecificTurn); err != nil { - applog.Error("Peer connection with specific TURN failed", zap.Uint("peer", playerId), zap.Error(err)) - } - peer.reconnectMu.Lock() - peer.manualReconnIsActive = false - peer.remoteManualRRequest = false - peer.reconnectMu.Unlock() - } + } else { + applog.Info("Connecting to peer with specific TURN", zap.Uint("playerId", playerId)) + if err := peer.ConnectOnce(peer.peerSpecificTurn); err != nil { + applog.Error("Peer connection with specific TURN failed", zap.Uint("peer", playerId), zap.Error(err)) + } + + p.peersMu.Lock() + if peer2, ok := p.peers[playerId]; ok && peer2 == peer { + peer.manualReconnIsActive = false + peer.remoteManualRRequest = false + } + p.peersMu.Unlock() + }And similarly, wrap the flag checks at the top of
handleReconnectionand inonPeerStateChangedwithpeersMuso all accesses are consistent.Also applies to: 456-490, 743-761, 763-806, 808-826, 828-886
561-592: GlobalmanualReconnectionMessagetemplate is racy and can target the wrong peer
manualReconnectionMessageis:
- Initialized once in
onPeerCandidatesGatheredthe first time candidates are sent (using that firstremotePeer).- Later mutated in-place in
HandleManualReconnectRequestviamanualReconnectionMessage.BaseEvent.RecipientID = &playerIdand sent.Issues:
- The template is shared across all peers;
HandleManualReconnectRequestfor peer B can still be using aSession/metadata captured when talking to peer A.RecipientIDis mutated on the shared global; concurrent manual recon requests for different peers will race and can stomp each other’s recipient beforeSendEvent.- There’s no synchronization around reads/writes of
manualReconnectionMessage.Since the receiving side only treats “Candidates with empty list” as a manual-reconnect signal, you don’t need this global at all. Build a fresh message per request:
func (p *PeerManager) HandleManualReconnectRequest(playerId uint) { @@ - if !peer.manualReconnIsActive && !peer.remoteManualRRequest { + if !peer.manualReconnIsActive && !peer.remoteManualRRequest { peer.manualReconnIsActive = true p.disableRecBtnPeers = append(p.disableRecBtnPeers, peerAsString) - - manualReconnectionMessage.BaseEvent.RecipientID = &playerId - err := p.icebreakerClient.SendEvent(manualReconnectionMessage) + + msg := icebreaker.CandidatesMessage{ + BaseEvent: icebreaker.BaseEvent{ + EventType: icebreaker.EventKindCandidates, + GameID: p.gameId, + SenderID: p.localUserId, + RecipientID: &playerId, + }, + Session: nil, + Candidates: []webrtc.ICECandidate{}, + } + err := p.icebreakerClient.SendEvent(msg)You can then remove
manualReconnectionMessageentirely unless it’s needed elsewhere.Also applies to: 763-790
321-343:GetAllPeersStatscorrectly guards against nil connectionsThe new
GetAllPeersStatsimplementation:
- Holds
peersMufor the duration.- Skips peers where
peer == nilorpeer.connection == nil.- Collects stats, connection state, and TURN IDs.
- Resets
disableRecBtnPeersafter exporting it.This addresses the earlier nil-dereference risk when some peers haven’t finished starting up.
658-736: Watcher globals andinitialConnectionWatcherremain fragile (currently unused)
watchersandwatcherMutexare package-level and never reset, andinitialConnectionWatcherstill dereferencespeer.connectionwithout checking for nil (even though it now usespeersMu). Right now the call site is commented out, so this is dead code, but if re-enabled:
- You risk cross-session leakage in
watchers(old entries persisting).- A panic if
peer.connectionis nil when the watcher runs.Consider either removing this workaround entirely for the debug build, or:
- Moving
watchersintoPeerManagerand clearing it per game/session.- Adding a
peer != nil && peer.connection != nilguard before callingConnectionState().
828-886:preparePeerForManualReconncan panic when no TURN servers are configured
preparePeerForManualReconnassumeslen(p.turnServer) > 0:
numOfTurns := len(p.turnServer)totalCombinations := numOfTurns * numOfTurns- Normalization with modulo
totalCombinations- Indexing
p.turnServer[...]If
numOfTurns == 0(e.g., misconfigured session or all URLs filtered out by the adapter’s TURN filtering), you’ll hit a divide-by-zero and/or index-out-of-range panic as soon as a manual recon is requested.Guard this explicitly:
func (p *PeerManager) preparePeerForManualReconn(playerId uint) { @@ - if ok { - peer.numOfManualReconns += 1 - peer.peerSpecificTurn = nil - - numOfTurns := len(p.turnServer) - totalCombinations := numOfTurns * numOfTurns + if ok { + numOfTurns := len(p.turnServer) + if numOfTurns == 0 { + applog.Error("Manual reconnection requested but no TURN servers are configured", + zap.String("peer", peerAsString)) + p.peersMu.Unlock() + return + } + + peer.numOfManualReconns++ + peer.peerSpecificTurn = nil + + totalCombinations := numOfTurns * numOfTurnsThis makes the invariant explicit and avoids hard panics when TURN is unavailable.
🧹 Nitpick comments (5)
webrtc/peer.go (1)
418-435: Drop-counter logic works but log formatting could be simplifiedThe new drop counters avoid log spam by warning only every ~100 dropped packets, which is sensible. You might consider switching to structured logging fields (
zap.Int("droppedTotal", ...),zap.Uint("peerId", ...)) instead of string concatenation to keep logs more queryable, but this is optional.adapter/adapter.go (3)
188-211: Nickname caching now avoids map races; minor style tweak possibleThe
userNicknamesmap is now updated undermuandGetNicknamesreturns a snapshot copy while still holding the lock, which fixes the earlier concurrent-read/write-on-map issue. You might simplifyGetNicknamesslightly withdefer mu.Unlock()for clearer control flow, but functionally this is solid.Also applies to: 248-256
92-133: TURN filtering is reasonable but consider handling “all filtered out” explicitlyThe new TURN construction correctly preserves credentials while:
- Dropping the hardcoded IP
139.162.142.250.- Optionally stripping UDP URLs for
region == "RU".- Excluding TCP 3478/5349 while keeping 80/443.
However, it’s possible to end up with
turnServ.URLsempty for some servers, or eventurnServereffectively empty after filtering. Given manual reconnection logic inwebrtc.PeerManagerassumes at least one TURN server, consider:
- Logging a warning when a server ends up with no URLs, and
- Logging (or failing) early if all URLs are filtered out so the caller can avoid manual recon flows that assume TURN availability.
24-27: Region constant andGetRegionhelper are straightforwardHard-coding
region = "Global"and exposing it viaGetRegion()is simple and self-contained. Just ensure this remains debug-only or is made configurable before any production use.Also applies to: 258-260
webrtc/peer_manager.go (1)
67-89:forceTurnRelayis now always true—verify this is intended for debug-only builds
NewPeerManagernow hard-codesforceTurnRelay: true, ignoring anylauncherInfo.ForceTurnRelaysetting. That means all peers will use TURN relay (or attempt to) even when a direct path would be preferable.If this is only for the debug window experiment, that’s fine; otherwise, you likely want to respect the launcher’s configuration or make this behavior explicitly toggleable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
adapter/adapter.go(5 hunks)applog/applog.go(3 hunks)cmd/faf-adapter/main.go(1 hunks)dbgwnd/dbgwnd.go(1 hunks)go.mod(2 hunks)webrtc/peer.go(4 hunks)webrtc/peer_manager.go(16 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/faf-adapter/main.go
- applog/applog.go
- dbgwnd/dbgwnd.go
🧰 Additional context used
🧬 Code graph analysis (3)
webrtc/peer.go (2)
applog/context.go (1)
FromContext(10-12)applog/applog.go (2)
Debug(110-123)Warn(95-108)
adapter/adapter.go (1)
webrtc/peer_manager.go (1)
PeerManager(51-65)
webrtc/peer_manager.go (3)
adapter/adapter.go (1)
PeerManager(20-20)launcher/info.go (1)
Info(8-20)icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (3)
go.mod (1)
50-78: This review comment references code that does not exist in the repository and should be retracted.The go.mod file is 38 lines total and contains only WebRTC and compression dependencies (pion/webrtc/v4, klauspost/compress, golang.org/x/net, etc.). It contains zero references to modernc.org, tk9.0, or any of the ~40 indirect dependencies mentioned in this review.
The code snippet showing modernc.org/tk9.0 and related packages at lines 50-78 is not present in the actual file, which ends at line 38. A search of the entire codebase confirms no tk9.0 or modernc.org references exist anywhere.
All concerns raised about CGo wrappers, supply chain expansion, platform-specific compatibility, and CVEs in libexpat/libfontconfig/libc are not applicable to this changeset. The review appears to have been written against a different PR, repository state, or file.
Likely an incorrect or invalid review comment.
webrtc/peer.go (2)
49-57: New peer metadata fields and initialization look consistentThe added fields for creation time, manual reconnection, TURN IDs, and packet-drop tracking are coherently defined on
Peerand correctly initialized inCreatePeer. They integrate cleanly with existing state and do not introduce obvious correctness issues in this file.Also applies to: 133-153
391-408: Send error logging downgrade to Debug is reasonableChanging failed data-channel send logging from Error to Debug reduces noise for transient issues while preserving visibility when needed. The surrounding control flow is unchanged and still exits cleanly on channel close/disable.
| go.uber.org/multierr v1.11.0 // indirect | ||
| golang.org/x/crypto v0.45.0 // indirect | ||
| golang.org/x/sys v0.38.0 // indirect | ||
| golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b // indirect |
There was a problem hiding this comment.
Remove or replace golang.org/x/exp dependency.
The golang.org/x/exp package is explicitly marked as unstable and unsuitable for production use. While this PR is currently draft/exploratory, remove this dependency before merging to production.
If the code depends on features from x/exp, migrate to stable equivalents:
- If using
exp/slices: switch to the standardslicespackage (Go 1.22+, and go.mod specifies 1.25.5). - If using other exp features: evaluate stable alternatives or inline minimal implementations.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webrtc/peer_manager.go (2)
345-351:GetAllPeerIdsreadsp.peerswithout holdingpeersMu.This method iterates over
p.peerswithout acquiring the mutex, while other methods modify the map underpeersMu. This can cause a concurrent map read/write panic.🔎 Proposed fix
func (p *PeerManager) GetAllPeerIds() []uint { + p.peersMu.Lock() + defer p.peersMu.Unlock() ids := make([]uint, 0, len(p.peers)) for id := range p.peers { ids = append(ids, id) } return ids }
357-363:removePeeraccessesp.peerswithout holdingpeersMu.This method reads and deletes from
p.peerswithout mutex protection, risking concurrent map access panics.🔎 Proposed fix
func (p *PeerManager) removePeer(playerId uint) { + p.peersMu.Lock() + defer p.peersMu.Unlock() peer := p.peers[playerId] if peer != nil { _ = peer.Close() delete(p.peers, playerId) } }
♻️ Duplicate comments (15)
dbgwnd/dbgwnd.go (9)
109-116: Closure captures loop variable by reference — all buttons will trigger withi=15.The closure
func() { reconnectBtnClicked(i) }capturesiby reference. All 15 buttons will callreconnectBtnClicked(15)after the loop completes.🔎 Proposed fix
for i := 0; i < 15; i++ { - but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(i) })) + idx := i // capture current value + but := Button(Txt("RECONNECT"), Font("Helvetica", 6, "bold"), Command(func() { reconnectBtnClicked(idx) })) reconnectButtons = append(reconnectButtons, but)
205-221: Checkbutton comparisons use wrong string value.Tcl/Tk stores canonical boolean values as
"1"and"0", not"true"and"false". WithOnvalue(true), Offvalue(false), the comparisons at lines 207 and 216 will never match.🔎 Proposed fix
func rawLogsChkButtonPressed(){ clearLogWindow = true - if rawLogsChkButton.Variable() == "true" { + if rawLogsChkButton.Variable() == "1" { showRawLogs = true } else { showRawLogs = false } } func hideDebugChkButtonPressed(){ clearLogWindow = true - if hideDebugChkButton.Variable() == "true" { + if hideDebugChkButton.Variable() == "1" { hideDebug = true } else { hideDebug = false } }
235-236:strconv.Atoierrors silently ignored.If parsing fails,
playerIDwill be 0, which may trigger unintended behavior when callingHandleManualNoRelayReconnRequest(0)orHandleManualReconnectRequest(0).🔎 Proposed fix for both functions
// In reconnNoRelaytBtnClicked: - playerID, _ := strconv.Atoi(sortedIds[buttonID]) + playerID, err := strconv.Atoi(sortedIds[buttonID]) + if err != nil { + return + } pm.HandleManualNoRelayReconnRequest(uint(playerID)) // In reconnectBtnClicked: - playerID, _ := strconv.Atoi(sortedIds[buttonID]) + playerID, err := strconv.Atoi(sortedIds[buttonID]) + if err != nil { + applog.Error("UI: failed to parse player ID", zap.Error(err)) + return + } pm.HandleManualReconnectRequest(uint(playerID))Also applies to: 246-247
259-264: Listbox selection uses only the first byte of the ID string.
selectedId[0]indexes a single byte (e.g."12"→49which is the byte for'1'). The map lookup and title will use the wrong value.🔎 Proposed fix
func infoListIdSelected(){ selected := connIdListBox.Curselection() if len(selected) > 0 { selectedId := connIdListBox.Get( strconv.Itoa(selected[0])) if len(selectedId) > 0 { - stat, ok := allPeersStats[selectedId[0]] + stat, ok := allPeersStats[selectedId] if ok { connInfoView.Clear() - title := "Connection ID: " + selectedId[0] + " Time: " + time.Now().Format("15:04:05") + title := "Connection ID: " + selectedId + " Time: " + time.Now().Format("15:04:05") connInfoView.Insert(END, title, "", "\n")
291-291:ParseFloaterror and split length not validated.If
logView.Yview()returns an unexpected format, the split may have fewer than 2 elements causing a panic, orParseFloatmay fail leavingtextYposat 0.0, breaking autoscroll logic.🔎 Proposed fix
- textYpos,_ := strconv.ParseFloat(strings.Split(logView.Yview(), " ")[1], 64) + textYpos := 1.0 // default to bottom + parts := strings.Split(logView.Yview(), " ") + if len(parts) >= 2 { + if parsed, err := strconv.ParseFloat(parts[1], 64); err == nil { + textYpos = parsed + } + }
354-356: Log parsing bails out on blank lines instead of continuing.Every log line ends with a newline, so
strings.Splityields empty strings between entries. Because the loopbreaks whenlen(st) < 5, the first blank entry stops processing forever—new log lines never reach the UI.🔎 Proposed fix
// there should be no empty lines and if there is one // then something went wrong and we skip this try if len(st) < 5 { - break + continue } rawLogCount += 1 // ... and in the formatted branch: if len(st) < 5 { - break + continue }Also applies to: 367-369
577-582: EntireplayerNamesmap is overwritten when a single name is missing.At line 581, when a player name is not found, the entire map is replaced with a fresh copy from
adapter.GetNicknames(), losing any previously cached names that might not be returned again.🔎 Proposed fix
playerName := "-" if name, ok := playerNames[id]; ok { playerName = name } else { - playerNames = adapter.GetNicknames() + allNames := adapter.GetNicknames() + for k, v := range allNames { + playerNames[k] = v + } + if name, ok := playerNames[id]; ok { + playerName = name + } }
599-608:reconnectButtonsEnabledis incremented incorrectly and never set to the intended value.The variable is incremented on every iteration (
reconnectButtonsEnabled += 1), resulting inreconnectButtonsEnabledgrowing unboundedly across refreshes instead of reflectingnumOfIds.🔎 Proposed fix
if reconnectButtonsEnabled != numOfIds { // normal buttons for i,b := range(reconnectButtons){ - reconnectButtonsEnabled += 1 if i < numOfIds { b.Configure((State("normal"))) } else { b.Configure((State("disabled"))) } } + reconnectButtonsEnabled = numOfIds
620-626: New peer detection may miss peer replacements.The condition
if len(oldPeer) < len(sortedIds)only detects when the peer count increases. If a peer disconnects and another connects in the same refresh cycle, the new peer won't be detected.🔎 Proposed fix
- if len(oldPeer) < len(sortedIds) { - for i,_ := range(sortedIds) { - if _, ok := oldPeer[sortedIds[i]]; !ok { - disabledBtnsTimers[sortedIds[i]] = disableRecButnOnNewConnect - } + for i,_ := range(sortedIds) { + if _, ok := oldPeer[sortedIds[i]]; !ok { + disabledBtnsTimers[sortedIds[i]] = disableRecButnOnNewConnect + oldPeer[sortedIds[i]] = true } }webrtc/peer_manager.go (6)
21-22: Global TURN state will persist across adapter restarts.
allTurnServersUrlsandturnsNameByIdare package-level variables that accumulate data. If the adapter restarts within the same process, stale entries persist and new URLs are appended, causing duplicates.🔎 Proposed fix
func (p *PeerManager) saveTurnsURLs() { + allTurnServersUrls = nil + turnsNameById = make(map[int]string) for i, _ := range(p.turnServer) {
108-133: TURN URL parsing is fragile with hardcoded.comassumption.The code at line 114 assumes all TURN URLs contain
.comand will produce incorrect or empty names for other TLDs (.org,.io,.net, country codes, etc.).🔎 Proposed fix using net/url
+import "net/url" func (p *PeerManager) saveTurnsURLs() { + allTurnServersUrls = nil + turnsNameById = make(map[int]string) for i, _ := range(p.turnServer) { for _,urlStr := range(p.turnServer[i].URLs) { _, ok := turnsNameById[i+1] if !ok { - preparedName := " " - index := strings.Index(url, ".com") - // ... existing fragile logic ... + preparedName := "unknown" + if u, err := url.Parse(urlStr); err == nil && u.Host != "" { + host := strings.Split(u.Host, ":")[0] + parts := strings.Split(host, ".") + if len(parts) >= 2 { + preparedName = parts[len(parts)-2] + } else if len(parts) == 1 { + preparedName = parts[0] + } + if len(preparedName) > 6 { + preparedName = preparedName[:6] + } + } turnsNameById[i+1] = preparedName }
563-563: GlobalmanualReconnectionMessageis mutated in place, causing race conditions.The global
manualReconnectionMessagehas itsRecipientIDmodified at line 774, then sent at line 775. If two manual reconnections happen concurrently for different peers, theRecipientIDcan be overwritten beforeSendEventis called.🔎 Proposed fix — create a local copy
- manualReconnectionMessage.BaseEvent.RecipientID = &playerId - err := p.icebreakerClient.SendEvent(manualReconnectionMessage) + msg := manualReconnectionMessage // shallow copy + msg.BaseEvent.RecipientID = &playerId + err := p.icebreakerClient.SendEvent(msg)Also applies to: 774-775
660-661: Globalwatchersmap risks cross-session contamination.The
watchersmap andwatcherMutexare package-level. If the adapter is restarted or multiple games run, stale entries persist and could prevent new watchers from being created for the same player IDs.Consider making
watchersan instance field ofPeerManageror clearing it when a newPeerManageris created.
696-702:initialConnectionWatchercan panic on nilpeer.connection.Inside the loop at line 698,
peer.connection.ConnectionState()is called without checking ifpeer.connectionis nil. During early setup or after closure, this will panic.🔎 Proposed fix
p.peersMu.Lock() if peer, ok := p.peers[playerId]; ok { - if peer.connection.ConnectionState().String() == "connected"{ + if peer.connection != nil && peer.connection.ConnectionState() == webrtc.PeerConnectionStateConnected { peerConnnectedSecondsTotal += 1 } } p.peersMu.Unlock()
839-845:preparePeerForManualReconncan panic when no TURN servers are configured.If
len(p.turnServer) == 0, thentotalCombinationsis 0, and the modulo operation at line 845 will cause a division-by-zero panic.🔎 Proposed fix
if ok { + numOfTurns := len(p.turnServer) + if numOfTurns == 0 { + applog.Error("Manual reconnection requested but no TURN servers are configured", zap.String("peer", peerAsString)) + p.peersMu.Unlock() + return + } + peer.numOfManualReconns += 1 peer.peerSpecificTurn = nil - - numOfTurns := len(p.turnServer) totalCombinations := numOfTurns * numOfTurns
🧹 Nitpick comments (1)
dbgwnd/dbgwnd.go (1)
25-51: Excessive package-level mutable state creates testing and maintenance challenges.The file uses ~25 package-level variables for UI state, widget references, and data caches. This makes the code difficult to test, reason about, and could cause issues if the window were ever recreated or if tests need isolation.
Consider encapsulating this state within a struct (e.g.,
DebugWindow) in future iterations.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dbgwnd/dbgwnd.go(1 hunks)webrtc/peer_manager.go(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dbgwnd/dbgwnd.go (3)
webrtc/peer_manager.go (1)
PeerManager(51-65)adapter/adapter.go (4)
PeerManager(20-20)GetPeerManager(244-246)GetRegion(258-260)GetNicknames(248-256)applog/applog.go (3)
Info(79-93)Error(125-138)GetLogFilePath(375-377)
webrtc/peer_manager.go (1)
icebreaker/events.go (3)
CandidatesMessage(50-54)BaseEvent(23-28)EventKindCandidates(14-14)
🔇 Additional comments (1)
webrtc/peer_manager.go (1)
86-86:forceTurnRelayis hardcoded totrue.The constructor now unconditionally sets
forceTurnRelay: true, ignoring any session configuration. This may be intentional for debugging, but verify this is the desired behavior for production.
| if manualReconnectionMessage.GameID == 0 { | ||
| manualReconnectionMessage = icebreaker.CandidatesMessage{ | ||
| BaseEvent: icebreaker.BaseEvent{ | ||
| EventType: icebreaker.EventKindCandidates, | ||
| GameID: p.gameId, | ||
| SenderID: p.localUserId, | ||
| RecipientID: &remotePeer, | ||
| }, | ||
| Session: description, | ||
| Candidates: candidates, | ||
| } | ||
| manualReconnectionMessage.Candidates = make([]webrtc.ICECandidate, 0) | ||
| } |
There was a problem hiding this comment.
manualReconnectionMessage template populated with first peer's data only.
The global template is populated once (when manualReconnectionMessage.GameID == 0) with the first peer's remotePeer and candidates. The Session and original Candidates fields remain from that first peer. While Candidates is cleared to empty, Session may be stale or incorrect for subsequent manual reconnections.
Consider constructing a fresh message each time in HandleManualReconnectRequest rather than using a global template.
🤖 Prompt for AI Agents
In webrtc/peer_manager.go around lines 581-593, the code reuses a single global
manualReconnectionMessage template initialized only when GameID==0, causing
stale Session (and potentially other fields) to be sent to subsequent peers;
instead, create a new icebreaker.CandidatesMessage inside
HandleManualReconnectRequest for each remotePeer: populate BaseEvent with the
current p.gameId, p.localUserId, and the current remotePeer, set Session to the
current description and Candidates to the current candidates (or an empty slice
if needed), and stop mutating/depending on the shared manualReconnectionMessage
variable so each manual reconnect builds and sends a fresh, per-peer message.
2 participants
The code is bad but it works :) Tested on test server but I'll use it in GW to make sure all run smooth. There is no
Reconnectbutton yet but I'm planning to add it later. If this ever goes to prod, it will require a serious polishing by someGOpro :)https://pkg.go.dev/modernc.org/tk9.0
Pretty lightweight package (final exe is ~22MB) and it doesn't require C compiler.
Final version:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.