From b053231e9a102f465ad80ba232dbebfd68a478e0 Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Tue, 7 Jul 2026 18:14:38 +0200 Subject: [PATCH] WIP --- shared/relay/client/manager.go | 71 +++++++++++++++++------ shared/relay/client/manager_dial_test.go | 72 ++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 17 deletions(-) create mode 100644 shared/relay/client/manager_dial_test.go diff --git a/shared/relay/client/manager.go b/shared/relay/client/manager.go index 3a7117d51..d1a995f34 100644 --- a/shared/relay/client/manager.go +++ b/shared/relay/client/manager.go @@ -30,11 +30,17 @@ type RelayTrack struct { relayClient *Client err error created time.Time + // ready is closed once the dial started by openConnVia finishes, at which + // point exactly one of relayClient/err is set. Callers that find an existing + // track wait on this channel instead of the track lock, so the network dial + // is never performed while holding rt.Lock(). See openConnVia. + ready chan struct{} } func NewRelayTrack() *RelayTrack { return &RelayTrack{ created: time.Now(), + ready: make(chan struct{}), } } @@ -335,34 +341,27 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string // check if already has a connection to the desired relay server m.relayClientsMutex.RLock() rt, ok := m.relayClients[serverAddress] - if ok { - rt.RLock() - m.relayClientsMutex.RUnlock() - defer rt.RUnlock() - if rt.err != nil { - return nil, rt.err - } - return rt.relayClient.OpenConn(ctx, peerKey) - } m.relayClientsMutex.RUnlock() + if ok { + return m.openConnOnTrack(ctx, rt, peerKey) + } // if not, establish a new connection but check it again (because changed the lock type) before starting the // connection m.relayClientsMutex.Lock() rt, ok = m.relayClients[serverAddress] if ok { - rt.RLock() m.relayClientsMutex.Unlock() - defer rt.RUnlock() - if rt.err != nil { - return nil, rt.err - } - return rt.relayClient.OpenConn(ctx, peerKey) + return m.openConnOnTrack(ctx, rt, peerKey) } - // create a new relay client and store it in the relayClients map + // Create the track, publish it, and release the map lock BEFORE dialing. The + // dial must not run while holding any track lock: RelayStates() and the + // cleanup loop take the track lock, and blocking them for the whole dial + // timeout is what stalls `netbird status -d`. Concurrent callers find this + // track in the map and wait on rt.ready (see openConnOnTrack), so only this + // goroutine performs the dial and the others reuse its result. rt = NewRelayTrack() - rt.Lock() m.relayClients[serverAddress] = rt m.relayClientsMutex.Unlock() @@ -370,8 +369,10 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string relayClient.SetTransportFallback(m.transportFallback) err := relayClient.Connect(m.ctx) if err != nil { + rt.Lock() rt.err = err rt.Unlock() + close(rt.ready) m.relayClientsMutex.Lock() delete(m.relayClients, serverAddress) m.relayClientsMutex.Unlock() @@ -379,8 +380,10 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string } // if connection closed then delete the relay client from the list relayClient.SetOnDisconnectListener(m.onServerDisconnected) + rt.Lock() rt.relayClient = relayClient rt.Unlock() + close(rt.ready) conn, err := relayClient.OpenConn(ctx, peerKey) if err != nil { @@ -389,6 +392,31 @@ func (m *Manager) openConnVia(ctx context.Context, serverAddress, peerKey string return conn, nil } +// openConnOnTrack opens a peer connection through an existing relay track, +// waiting for the dial started by another openConnVia call (if still running) +// to finish. It waits on rt.ready rather than the track lock, so it neither +// holds nor contends the track lock across the dial; the RLock it takes +// afterwards only guards the brief relayClient read + OpenConn, matching the +// previous behaviour of protecting the client against a concurrent cleanup +// close. +func (m *Manager) openConnOnTrack(ctx context.Context, rt *RelayTrack, peerKey string) (net.Conn, error) { + select { + case <-rt.ready: + case <-ctx.Done(): + return nil, ctx.Err() + } + + rt.RLock() + defer rt.RUnlock() + if rt.err != nil { + return nil, rt.err + } + if rt.relayClient == nil { + return nil, ErrRelayClientNotConnected + } + return rt.relayClient.OpenConn(ctx, peerKey) +} + func (m *Manager) onServerConnected() { m.listenerLock.Lock() defer m.listenerLock.Unlock() @@ -485,6 +513,15 @@ func (m *Manager) cleanUpUnusedRelays() { continue } + // The dial started by openConnVia is still in progress: the track is + // published before Connect() completes and no longer runs under rt.Lock, + // so relayClient is not set yet. Nothing to clean up, and it must not be + // evicted out from under the in-flight dial. + if rt.relayClient == nil { + rt.Unlock() + continue + } + if time.Since(rt.created) <= m.keepUnusedServerTime { rt.Unlock() continue diff --git a/shared/relay/client/manager_dial_test.go b/shared/relay/client/manager_dial_test.go new file mode 100644 index 000000000..8e34db56d --- /dev/null +++ b/shared/relay/client/manager_dial_test.go @@ -0,0 +1,72 @@ +package client + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// TestManager_InProgressDialIsSafeForReadersAndCleanup covers the new state that +// option 1 introduces: openConnVia now publishes a relay track in the map and +// releases the map lock BEFORE dialing, so a track can legitimately exist with +// relayClient == nil and no track lock held while its Connect() is in flight. +// +// The status path (RelayStates) and the cleanup loop both touch every track, so +// both must tolerate that mid-dial state — neither deref the nil relayClient nor +// evict a track whose dial is still running. +func TestManager_InProgressDialIsSafeForReadersAndCleanup(t *testing.T) { + m := &Manager{ + relayClients: make(map[string]*RelayTrack), + // 0 so the created-time grace does not mask the mid-dial (nil relayClient) + // guard in cleanUpUnusedRelays. + keepUnusedServerTime: 0, + } + + const addr = "relay.example.com:443" + rt := NewRelayTrack() // ready open, relayClient nil, unlocked == dial in progress + m.relayClients[addr] = rt + + // A status call must not block or panic on a relay still being dialed; it has + // no state to report yet. + require.Empty(t, m.RelayStates(), "a relay still being dialed has no state to report") + + // Cleanup must not deref the nil relayClient, and must not evict an in-flight dial. + m.cleanUpUnusedRelays() + m.relayClientsMutex.RLock() + _, stillTracked := m.relayClients[addr] + m.relayClientsMutex.RUnlock() + require.True(t, stillTracked, "an in-progress dial must not be cleaned up") +} + +// TestOpenConnOnTrack_ReleasesOnContextCancelDuringDial verifies the core option-1 +// property: a caller that finds a track whose dial is in progress waits on +// rt.ready, not on the track lock, and can be released by its own context. This +// is what keeps a slow relay dial from serializing behind the track lock. +func TestOpenConnOnTrack_ReleasesOnContextCancelDuringDial(t *testing.T) { + m := &Manager{relayClients: make(map[string]*RelayTrack)} + + rt := NewRelayTrack() // ready deliberately left open == dial in progress + + ctx, cancel := context.WithCancel(context.Background()) + errCh := make(chan error, 1) + go func() { + _, err := m.openConnOnTrack(ctx, rt, "peerKey") + errCh <- err + }() + + // While waiting for the dial the caller holds no track lock, so a concurrent + // reader (e.g. RelayStates) can still take it. + require.True(t, rt.TryRLock(), "waiter must not hold the track lock while the dial is in progress") + rt.RUnlock() + + // Caller gives up: openConnOnTrack must return via ctx rather than hang on the dial. + cancel() + select { + case err := <-errCh: + require.ErrorIs(t, err, context.Canceled) + case <-time.After(2 * time.Second): + t.Fatal("openConnOnTrack did not return on ctx cancellation while a dial was in progress") + } +}