Optimize ICE guard reconnection logic and remove unnecessary wait function (#4323)

client/internal/peer/conn.go

@@ -24,8 +24,8 @@ import (
 	"github.com/netbirdio/netbird/client/internal/peer/id"
 	"github.com/netbirdio/netbird/client/internal/peer/worker"
 	"github.com/netbirdio/netbird/client/internal/stdnet"
-	relayClient "github.com/netbirdio/netbird/shared/relay/client"
 	"github.com/netbirdio/netbird/route"
+	relayClient "github.com/netbirdio/netbird/shared/relay/client"
 	semaphoregroup "github.com/netbirdio/netbird/util/semaphore-group"
 )
 
@@ -200,19 +200,11 @@ func (conn *Conn) Open(engineCtx context.Context) error {
 	conn.wg.Add(1)
 	go func() {
 		defer conn.wg.Done()
+
 		conn.waitInitialRandomSleepTime(conn.ctx)
 		conn.semaphore.Done(conn.ctx)
 
-		conn.dumpState.SendOffer()
-		if err := conn.handshaker.sendOffer(); err != nil {
-			conn.Log.Errorf("failed to send initial offer: %v", err)
-		}
-
-		conn.wg.Add(1)
-		go func() {
-			conn.guard.Start(conn.ctx, conn.onGuardEvent)
-			conn.wg.Done()
-		}()
+		conn.guard.Start(conn.ctx, conn.onGuardEvent)
 	}()
 	conn.opened = true
 	return nil
@@ -671,7 +663,7 @@ func (conn *Conn) isConnectedOnAllWay() (connected bool) {
 		}
 	}()
 
-	if conn.statusICE.Get() == worker.StatusDisconnected {
+	if conn.statusICE.Get() == worker.StatusDisconnected && !conn.workerICE.InProgress() {
 		return false
 	}
 

client/internal/peer/guard/guard.go

@@ -19,7 +19,6 @@ type isConnectedFunc func() bool
 // - Relayed connection disconnected
 // - ICE candidate changes
 type Guard struct {
-	Reconnect               chan struct{}
 	log                     *log.Entry
 	isConnectedOnAllWay     isConnectedFunc
 	timeout                 time.Duration
@@ -30,7 +29,6 @@ type Guard struct {
 
 func NewGuard(log *log.Entry, isConnectedFn isConnectedFunc, timeout time.Duration, srWatcher *SRWatcher) *Guard {
 	return &Guard{
-		Reconnect:               make(chan struct{}, 1),
 		log:                     log,
 		isConnectedOnAllWay:     isConnectedFn,
 		timeout:                 timeout,
@@ -41,6 +39,7 @@ func NewGuard(log *log.Entry, isConnectedFn isConnectedFunc, timeout time.Durati
 }
 
 func (g *Guard) Start(ctx context.Context, eventCallback func()) {
+	g.log.Infof("starting guard for reconnection with MaxInterval: %s", g.timeout)
 	g.reconnectLoopWithRetry(ctx, eventCallback)
 }
 
@@ -61,17 +60,14 @@ func (g *Guard) SetICEConnDisconnected() {
 // reconnectLoopWithRetry periodically check the connection status.
 // Try to send offer while the P2P is not established or while the Relay is not connected if is it supported
 func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) {
-	waitForInitialConnectionTry(ctx)
-
 	srReconnectedChan := g.srWatcher.NewListener()
 	defer g.srWatcher.RemoveListener(srReconnectedChan)
 
-	ticker := g.prepareExponentTicker(ctx)
+	ticker := g.initialTicker(ctx)
 	defer ticker.Stop()
 
 	tickerChannel := ticker.C
 
-	g.log.Infof("start reconnect loop...")
 	for {
 		select {
 		case t := <-tickerChannel:
@@ -85,7 +81,6 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) {
 			if !g.isConnectedOnAllWay() {
 				callback()
 			}
-
 		case <-g.relayedConnDisconnected:
 			g.log.Debugf("Relay connection changed, reset reconnection ticker")
 			ticker.Stop()
@@ -111,6 +106,20 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) {
 	}
 }
 
+// initialTicker give chance to the peer to establish the initial connection.
+func (g *Guard) initialTicker(ctx context.Context) *backoff.Ticker {
+	bo := backoff.WithContext(&backoff.ExponentialBackOff{
+		InitialInterval:     3 * time.Second,
+		RandomizationFactor: 0.1,
+		Multiplier:          2,
+		MaxInterval:         g.timeout,
+		Stop:                backoff.Stop,
+		Clock:               backoff.SystemClock,
+	}, ctx)
+
+	return backoff.NewTicker(bo)
+}
+
 func (g *Guard) prepareExponentTicker(ctx context.Context) *backoff.Ticker {
 	bo := backoff.WithContext(&backoff.ExponentialBackOff{
 		InitialInterval:     800 * time.Millisecond,
@@ -126,13 +135,3 @@ func (g *Guard) prepareExponentTicker(ctx context.Context) *backoff.Ticker {
 
 	return ticker
 }
-
-// Give chance to the peer to establish the initial connection.
-// With it, we can decrease to send necessary offer
-func waitForInitialConnectionTry(ctx context.Context) {
-	select {
-	case <-ctx.Done():
-		return
-	case <-time.After(3 * time.Second):
-	}
-}

client/internal/peer/worker_ice.go

@@ -172,6 +172,13 @@ func (w *WorkerICE) GetLocalUserCredentials() (frag string, pwd string) {
 	return w.localUfrag, w.localPwd
 }
 
+func (w *WorkerICE) InProgress() bool {
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+
+	return w.agentConnecting
+}
+
 func (w *WorkerICE) Close() {
 	w.muxAgent.Lock()
 	defer w.muxAgent.Unlock()