diff --git a/shared/relay/client/manager.go b/shared/relay/client/manager.go index d65ebb1ce..2f2839d94 100644 --- a/shared/relay/client/manager.go +++ b/shared/relay/client/manager.go @@ -30,11 +30,16 @@ type RelayTrack struct { relayClient *Client err error created time.Time + // ready is closed once the dial started by openConnVia finishes (relayClient + // or err is set). Callers reusing a track wait on this instead of the track + // lock, so the dial never runs under rt.Lock. + ready chan struct{} } func NewRelayTrack() *RelayTrack { return &RelayTrack{ created: time.Now(), + ready: make(chan struct{}), } } @@ -294,17 +299,8 @@ 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 { - if !rt.TryRLock() { - continue - } + rt.RLock() rc := rt.relayClient rt.RUnlock() if rc != nil { @@ -335,34 +331,24 @@ 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 + // Publish the track and release the map lock BEFORE dialing, so the dial does + // not run under rt.Lock (which would block RelayStates and the cleanup loop + // for the full dial). Concurrent callers find this track and wait on rt.ready. rt = NewRelayTrack() - rt.Lock() m.relayClients[serverAddress] = rt m.relayClientsMutex.Unlock() @@ -370,8 +356,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,14 +367,34 @@ 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 { - return nil, err + return relayClient.OpenConn(ctx, peerKey) +} + +// openConnOnTrack opens a peer connection through an existing relay track, +// waiting for the dial started by another openConnVia call to finish. It waits +// on rt.ready rather than the track lock, so it neither holds nor contends the +// track lock across the dial. +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() } - return conn, nil + + 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() { @@ -477,10 +485,7 @@ func (m *Manager) cleanUpUnusedRelays() { defer m.relayClientsMutex.Unlock() for addr, rt := range m.relayClients { - if !rt.TryRLock() { - // Skip while in-progress relay dial. - continue - } + rt.Lock() // if the connection failed to the server the relay client will be nil // but the instance will be kept in the relayClients until the next locking if rt.err != nil { @@ -488,6 +493,13 @@ func (m *Manager) cleanUpUnusedRelays() { continue } + // dial still in progress (openConnVia publishes the track before Connect + // completes and no longer holds rt.Lock during it), nothing to clean up. + if rt.relayClient == nil { + rt.Unlock() + continue + } + if time.Since(rt.created) <= m.keepUnusedServerTime { rt.Unlock() continue