From 705ed0566fd8d79236590a07d2e96162f1875395 Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Tue, 7 Jul 2026 17:57:59 +0200 Subject: [PATCH] Use rt.TryRLock in RelayStates for RelayTrack --- shared/relay/client/manager.go | 11 +++++- .../relay/client/manager_relaystates_test.go | 36 +++++-------------- 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/shared/relay/client/manager.go b/shared/relay/client/manager.go index e1515401e..77c55400d 100644 --- a/shared/relay/client/manager.go +++ b/shared/relay/client/manager.go @@ -294,8 +294,17 @@ func (m *Manager) RelayStates() []RelayConnState { // Only connected foreign relays carry state; a failed connect is evicted // immediately (openConnVia), so there is no error state to surface. + // + // Query each track without blocking: openConnVia holds a track's write-lock + // for the whole of relayClient.Connect() (the network dial). A blocking + // RLock here would stall the status path (GetFullStatus -> GetRelayStates) + // for the full dial timeout. A track mid-Connect has no relayClient set yet + // and would be skipped. TryRLock + skip preserves the result while keeping + // status responsive. for _, rt := range tracks { - rt.RLock() + if !rt.TryRLock() { + continue + } rc := rt.relayClient rt.RUnlock() if rc != nil { diff --git a/shared/relay/client/manager_relaystates_test.go b/shared/relay/client/manager_relaystates_test.go index 23b0cab85..453b1fc11 100644 --- a/shared/relay/client/manager_relaystates_test.go +++ b/shared/relay/client/manager_relaystates_test.go @@ -8,30 +8,15 @@ import ( // TestRelayStates_DoesNotBlockWhileForeignRelayConnecting is a regression test for // status calls hanging behind an in-progress relay dial. // -// openConnVia establishes a new foreign relay like this: -// -// rt = NewRelayTrack() -// rt.Lock() // track write-lock -// m.relayClients[serverAddress] = rt -// m.relayClientsMutex.Unlock() -// ... -// err := relayClient.Connect(m.ctx) // network dial, held UNDER rt.Lock() -// ... -// rt.Unlock() // released only after Connect returns/times out -// -// So while a relay is being dialed, its RelayTrack write-lock is held for the whole +// While a relay is being dialed, its RelayTrack write-lock is held for the whole // dial (up to serverResponseTimeout per transport attempt, times the transport -// fallback chain, times however many relays are being dialed at once). +// fallback chain, times however many relays are being dialed at once) in openConnVia. // -// RelayStates() — reached from the daemon status path via -// peer.Status.GetFullStatus() -> GetRelayStates() -> Manager.RelayStates() — takes -// rt.RLock() on every tracked relay. A reader lock blocks while a writer holds the -// lock, so a single foreign relay mid-Connect stalls RelayStates(), and therefore -// `netbird status -d`, for the full dial timeout. #6547 moved this off the shared -// map lock but the per-track RLock still blocks the status path. -// -// This test recreates the exact in-progress-dial state (track present in the map -// with its write-lock held) and asserts RelayStates() does not wait on it. +// RelayStates() is reached from the daemon status path via +// peer.Status.GetFullStatus() -> GetRelayStates() -> Manager.RelayStates(). +// It takes rt.RLock() on every tracked relay. A reader lock blocks while a +// writer holds the lock, so a single foreign relay mid-Connect in openConnVia +// stalls RelayStates(), and therefore `netbird status -d` hangs for the full dial timeout. func TestRelayStates_DoesNotBlockWhileForeignRelayConnecting(t *testing.T) { m := &Manager{ relayClients: make(map[string]*RelayTrack), @@ -42,8 +27,6 @@ func TestRelayStates_DoesNotBlockWhileForeignRelayConnecting(t *testing.T) { rt := NewRelayTrack() rt.Lock() m.relayClients["relay.example.com:443"] = rt - // Release at the end so a (buggy) blocked RelayStates goroutine can unwind - // instead of leaking past the test. t.Cleanup(rt.Unlock) done := make(chan []RelayConnState, 1) @@ -53,10 +36,7 @@ func TestRelayStates_DoesNotBlockWhileForeignRelayConnecting(t *testing.T) { select { case <-done: - // RelayStates returned without waiting for the in-progress dial. Good. case <-time.After(2 * time.Second): - t.Fatal("RelayStates() blocked on a relay track whose Connect() is in progress " + - "(rt.Lock held across the dial in openConnVia); `netbird status -d` hangs for " + - "the relay dial timeout") + t.Fatal("RelayStates() blocked on a relay track whose Connect() is in progress") } }