From 0a032119031e350040bff8faa3325a8273787fa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 25 Aug 2026 15:09:00 +0200 Subject: [PATCH] [client] Name the netevents fields after their types Review feedback on #7254: the fields kept their netstate-era names after the type changed to *netevents.Manager or a NetworkWatcher interface. Rename the Manager-typed fields to netMgr/NetMgr and the interface-typed guard fields to netWatcher, document that the Manager write methods panic on a nil receiver while the read methods stay nil-safe, and fix the waiteBeforeRetry typo. The relay client keeps its netEvents fields: those hold the NetEvents interface, so the name already matches the type. --- client/internal/connect.go | 20 ++++++++++---------- client/internal/engine.go | 14 +++++++------- client/internal/peer/conn.go | 6 +++--- client/internal/peer/guard/guard.go | 20 ++++++++++---------- client/netevents/netevents.go | 11 +++++++++-- shared/management/client/grpc.go | 14 +++++++------- shared/relay/client/guard.go | 22 +++++++++++----------- shared/signal/client/grpc.go | 14 +++++++------- 8 files changed, 64 insertions(+), 57 deletions(-) diff --git a/client/internal/connect.go b/client/internal/connect.go index 161c76079..ca50f912f 100644 --- a/client/internal/connect.go +++ b/client/internal/connect.go @@ -72,9 +72,9 @@ type ConnectClient struct { persistSyncResponse bool - // netEvents gates every reconnection loop on OS-reported network + // netMgr gates every reconnection loop on OS-reported network // availability and sweeps connections on network change. - netEvents *netevents.Manager + netMgr *netevents.Manager } // ConnectClientOption configures optional ConnectClient behavior. @@ -82,7 +82,7 @@ type ConnectClientOption func(*ConnectClient) // WithNetEvents injects the OS network event handling. func WithNetEvents(events *netevents.Manager) ConnectClientOption { - return func(c *ConnectClient) { c.netEvents = events } + return func(c *ConnectClient) { c.netMgr = events } } func NewConnectClient( @@ -293,7 +293,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan } // suspend connection attempts while the OS reports no usable network - if waited, err := c.netEvents.Wait(c.ctx); err != nil { + if waited, err := c.netMgr.Wait(c.ctx); err != nil { return nil } else if waited { backOff.Reset() @@ -311,7 +311,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan log.Debugf("connecting to the Management service %s", c.config.ManagementURL.Host) mgmClient, err := mgm.NewClient(engineCtx, c.config.ManagementURL.Host, myPrivateKey, mgmTlsEnabled, - mgm.WithNetEvents(c.netEvents)) + mgm.WithNetEvents(c.netMgr)) if err != nil { // On daemon shutdown / Down() the parent context is cancelled // and the dial fails with "context canceled". Wrapping that @@ -386,7 +386,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan }() // with the global Netbird config in hand connect (just a connection, no stream yet) Signal - signalClient, err := connectToSignal(engineCtx, loginResp.GetNetbirdConfig(), myPrivateKey, c.netEvents) + signalClient, err := connectToSignal(engineCtx, loginResp.GetNetbirdConfig(), myPrivateKey, c.netMgr) if err != nil { log.Error(err) return wrapErr(err) @@ -423,7 +423,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan } relayManager := relayClient.NewManager(engineCtx, relayURLs, myPrivateKey.PublicKey().String(), engineConfig.MTU, - relayClient.WithNetEvents(c.netEvents)) + relayClient.WithNetEvents(c.netMgr)) c.statusRecorder.SetRelayMgr(relayManager) if len(relayURLs) > 0 { if token != nil { @@ -451,7 +451,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan UpdateManager: c.updateManager, ClientMetrics: c.clientMetrics, MetricsCtx: c.ctx, - NetState: c.netEvents, + NetMgr: c.netMgr, }, mobileDependency) engine.SetSyncResponsePersistence(c.persistSyncResponse) c.engine = engine @@ -711,7 +711,7 @@ func selectMTU(localMTU uint16, peerMTU int32) uint16 { } // connectToSignal creates Signal Service client and established a connection -func connectToSignal(ctx context.Context, wtConfig *mgmProto.NetbirdConfig, ourPrivateKey wgtypes.Key, netEvents *netevents.Manager) (*signal.GrpcClient, error) { +func connectToSignal(ctx context.Context, wtConfig *mgmProto.NetbirdConfig, ourPrivateKey wgtypes.Key, netMgr *netevents.Manager) (*signal.GrpcClient, error) { var sigTLSEnabled bool if wtConfig.Signal.Protocol == mgmProto.HostConfig_HTTPS { sigTLSEnabled = true @@ -720,7 +720,7 @@ func connectToSignal(ctx context.Context, wtConfig *mgmProto.NetbirdConfig, ourP } signalClient, err := signal.NewClient(ctx, wtConfig.Signal.Uri, ourPrivateKey, sigTLSEnabled, - signal.WithNetEvents(netEvents)) + signal.WithNetEvents(netMgr)) if err != nil { log.Errorf("error while connecting to the Signal Exchange Service %s: %s", wtConfig.Signal.Uri, err) return nil, gstatus.Errorf(codes.FailedPrecondition, "failed connecting to Signal Service : %s", err) diff --git a/client/internal/engine.go b/client/internal/engine.go index d83868ab6..fac5224c8 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -182,9 +182,9 @@ type EngineServices struct { UpdateManager *updater.Manager ClientMetrics *metrics.ClientMetrics MetricsCtx context.Context - // NetState gates the reconnection loops on OS-reported network + // NetMgr gates the reconnection loops on OS-reported network // availability; nil disables gating. - NetState *netevents.Manager + NetMgr *netevents.Manager } // Engine is a mechanism responsible for reacting on Signal and Management stream events and managing connections to the remote peers. @@ -208,9 +208,9 @@ type Engine struct { config *EngineConfig mobileDep MobileDependency - // netState gates the peer reconnection guards on OS-reported network + // netMgr gates the peer reconnection guards on OS-reported network // availability; nil disables gating. - netState *netevents.Manager + netMgr *netevents.Manager // STUNs is a list of STUN servers used by ICE STUNs []*stun.URI @@ -345,7 +345,7 @@ func NewEngine( syncMsgMux: &sync.Mutex{}, config: config, mobileDep: mobileDep, - netState: services.NetState, + netMgr: services.NetMgr, STUNs: []*stun.URI{}, TURNs: []*stun.URI{}, networkSerial: 0, @@ -1902,8 +1902,8 @@ func (e *Engine) createPeerConn(pubKey string, allowedIPs []netip.Prefix, agentV Addr: e.getRosenpassAddr(), PermissiveMode: e.config.RosenpassPermissive, }, - ICEConfig: e.createICEConfig(), - NetworkState: e.netState, + ICEConfig: e.createICEConfig(), + NetMgr: e.netMgr, } serviceDependencies := peer.ServiceDependencies{ diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index 7e0b4559e..83089606f 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -95,9 +95,9 @@ type ConnConfig struct { // ICEConfig ICE protocol configuration ICEConfig icemaker.Config - // NetworkState gates the reconnection guard on OS-reported network + // NetMgr gates the reconnection guard on OS-reported network // availability; nil disables gating. - NetworkState *netevents.Manager + NetMgr *netevents.Manager } type Conn struct { @@ -259,7 +259,7 @@ func (conn *Conn) open(engineCtx context.Context, firstPacket []byte) error { conn.handshaker.AddICEListener(conn.workerICE.OnNewOffer) } - conn.guard = guard.NewGuard(conn.Log, conn.isConnectedOnAllWay, conn.config.Timeout, conn.srWatcher, conn.config.NetworkState) + conn.guard = guard.NewGuard(conn.Log, conn.isConnectedOnAllWay, conn.config.Timeout, conn.srWatcher, conn.config.NetMgr) conn.wg.Add(1) go func() { diff --git a/client/internal/peer/guard/guard.go b/client/internal/peer/guard/guard.go index e2ad32a73..73bab2a89 100644 --- a/client/internal/peer/guard/guard.go +++ b/client/internal/peer/guard/guard.go @@ -41,22 +41,22 @@ type Guard struct { isConnectedOnAllWay connStatusFunc timeout time.Duration srWatcher *SRWatcher - // netState gates reconnect attempts on OS-reported network availability; + // netWatcher gates reconnect attempts on OS-reported network availability; // nil disables gating. - netState NetworkWatcher + netWatcher NetworkWatcher relayedConnDisconnected chan struct{} iCEConnDisconnected chan struct{} } -// NewGuard creates a reconnection guard for a peer connection. A nil netState +// NewGuard creates a reconnection guard for a peer connection. A nil netWatcher // disables network availability gating. -func NewGuard(log *log.Entry, isConnectedFn connStatusFunc, timeout time.Duration, srWatcher *SRWatcher, netState NetworkWatcher) *Guard { +func NewGuard(log *log.Entry, isConnectedFn connStatusFunc, timeout time.Duration, srWatcher *SRWatcher, netWatcher NetworkWatcher) *Guard { return &Guard{ log: log, isConnectedOnAllWay: isConnectedFn, timeout: timeout, srWatcher: srWatcher, - netState: netState, + netWatcher: netWatcher, relayedConnDisconnected: make(chan struct{}, 1), iCEConnDisconnected: make(chan struct{}, 1), } @@ -109,8 +109,8 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) { defer iceState.reset() var netChanged <-chan struct{} - if g.netState != nil { - netChanged = g.netState.Changed() + if g.netWatcher != nil { + netChanged = g.netWatcher.Changed() } for { @@ -118,7 +118,7 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) { case <-tickerChannel: // skip attempts while the OS reports no usable network; the // netChanged case below resumes the loop once it returns - if g.netState != nil && !g.netState.IsOnline() { + if g.netWatcher != nil && !g.netWatcher.IsOnline() { continue } switch g.isConnectedOnAllWay() { @@ -159,8 +159,8 @@ func (g *Guard) reconnectLoopWithRetry(ctx context.Context, callback func()) { case <-netChanged: // Re-arm for the next transition before acting on this one. - netChanged = g.netState.Changed() - if !g.netState.IsOnline() { + netChanged = g.netWatcher.Changed() + if !g.netWatcher.IsOnline() { continue } // Ticks skipped while offline drove the backoff towards its diff --git a/client/netevents/netevents.go b/client/netevents/netevents.go index 635539264..474cbfa22 100644 --- a/client/netevents/netevents.go +++ b/client/netevents/netevents.go @@ -23,8 +23,9 @@ type Recorder interface { // Manager ties the network availability state, the connection sweeper and the // status recorder together; it outlives engine restarts. A nil *Manager is -// the valid no-events value: the read methods report always-online and never -// sweep. +// the valid no-events value for consumers: the read methods report +// always-online and never sweep. Only the event sources hold a real Manager, +// so the write methods do not tolerate a nil receiver. type Manager struct { // mu serializes availability transitions: the IsOnline check and the // state update must be atomic, or a racing offline flip can skip the sweep @@ -52,6 +53,9 @@ func NewManager(recorder Recorder) *Manager { // redial while offline, so the stale sockets would otherwise stay silently // "connected" until their own timeouts and the client would keep reporting // Connected with no network at all. +// +// Panics on a nil receiver: only the mobile bindings that own a Manager +// report availability. func (m *Manager) SetNetworkAvailable(available bool) { m.mu.Lock() defer m.mu.Unlock() @@ -67,6 +71,9 @@ func (m *Manager) SetNetworkAvailable(available bool) { // stale after the OS switched networks and schedules a sweep that cuts // whatever has not redialed on the new network by then. The engine and the // TUN device stay untouched. +// +// Panics on a nil receiver: only the mobile bindings that own a Manager +// report network changes. func (m *Manager) NotifyNetworkChange() { m.sweeper.MarkNetworkChange() log.Infof("network change: connections marked stale") diff --git a/shared/management/client/grpc.go b/shared/management/client/grpc.go index 0bd912929..82ad305a9 100644 --- a/shared/management/client/grpc.go +++ b/shared/management/client/grpc.go @@ -63,9 +63,9 @@ type GrpcClient struct { connStateCallbackLock sync.RWMutex serverURL string - // netEvents gates the stream retry loop on OS-reported network + // netMgr gates the stream retry loop on OS-reported network // availability and sweeps the transport on network change. - netEvents *netevents.Manager + netMgr *netevents.Manager // syncStreamErr holds the last Sync stream error, or nil while the stream // is established and healthy. GetServerKey succeeds even when the peer @@ -121,7 +121,7 @@ type Option func(*GrpcClient) // WithNetEvents injects the OS network event handling. func WithNetEvents(events *netevents.Manager) Option { - return func(c *GrpcClient) { c.netEvents = events } + return func(c *GrpcClient) { c.netMgr = events } } // NewClient creates a new client to Management service @@ -142,7 +142,7 @@ func NewClient(ctx context.Context, addr string, ourPrivateKey wgtypes.Key, tlsE extraOpts = append(extraOpts, grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(maxSize))) log.Infof("management gRPC max receive message size set to %d bytes", maxSize) } - extraOpts = append(extraOpts, nbgrpc.WithSweeper(c.netEvents)) + extraOpts = append(extraOpts, nbgrpc.WithSweeper(c.netMgr)) var conn *grpc.ClientConn operation := func() error { @@ -223,12 +223,12 @@ func (c *GrpcClient) withMgmtStream( ctx context.Context, handler func(ctx context.Context, serverPubKey wgtypes.Key, backOff backoff.BackOff) error, ) error { - backOff := c.netEvents.QuickRetryBackoff(ctx, defaultBackoff(ctx)) + backOff := c.netMgr.QuickRetryBackoff(ctx, defaultBackoff(ctx)) operation := func() error { // suspend reconnect attempts while the OS reports no usable network. // Wait only errors on a cancelled context, which means shutdown, so // stop the loop without reporting a failure. - if waited, err := c.netEvents.Wait(ctx); err != nil { + if waited, err := c.netMgr.Wait(ctx); err != nil { log.Debugf("management connection context has been canceled while offline, this usually indicates shutdown") return nil //nolint:nilerr // a cancelled context means shutdown, not a retryable failure } else if waited { @@ -264,7 +264,7 @@ func (c *GrpcClient) withMgmtStream( return handler(ctx, *serverPubKey, backOff) } - err := nbgrpc.Retry(ctx, operation, backOff, c.netEvents) + err := nbgrpc.Retry(ctx, operation, backOff, c.netMgr) if err != nil { log.Warnf("exiting the Management service connection retry loop due to the unrecoverable error: %s", err) } diff --git a/shared/relay/client/guard.go b/shared/relay/client/guard.go index c83939d6c..c0294b82d 100644 --- a/shared/relay/client/guard.go +++ b/shared/relay/client/guard.go @@ -40,8 +40,8 @@ type Guard struct { // attempts. maxBackoffInterval time.Duration - // netState gates reconnect attempts on OS-reported network availability. - netState NetworkWatcher + // netWatcher gates reconnect attempts on OS-reported network availability. + netWatcher NetworkWatcher // lastErr is the error from the most recent failed reconnect attempt, // surfaced as the home relay status while disconnected. @@ -50,7 +50,7 @@ type Guard struct { // NewGuard creates a new guard for the relay client. A non-positive // maxBackoffInterval falls back to defaultMaxBackoffInterval. -func NewGuard(sp *ServerPicker, maxBackoffInterval time.Duration, netState NetworkWatcher) *Guard { +func NewGuard(sp *ServerPicker, maxBackoffInterval time.Duration, netWatcher NetworkWatcher) *Guard { if maxBackoffInterval <= 0 { maxBackoffInterval = defaultMaxBackoffInterval } @@ -59,7 +59,7 @@ func NewGuard(sp *ServerPicker, maxBackoffInterval time.Duration, netState Netwo OnReconnected: make(chan struct{}, 1), serverPicker: sp, maxBackoffInterval: maxBackoffInterval, - netState: netState, + netWatcher: netWatcher, } return g } @@ -100,8 +100,8 @@ func (g *Guard) StartReconnectTrys(ctx context.Context, relayClient *Client) { select { case <-ticker.C: // suspend reconnect attempts while the OS reports no usable network - if g.netState != nil { - if waited, err := g.netState.Wait(ctx); err != nil { + if g.netWatcher != nil { + if waited, err := g.netWatcher.Wait(ctx); err != nil { return } else if waited { ticker.Stop() @@ -134,16 +134,16 @@ func (g *Guard) tryToQuickReconnect(parentCtx context.Context, rc *Client) bool return false } - if g.netState != nil { - if ok := g.netState.WaitSettled(parentCtx, quickReconnectBudget, verdictSettleWindow); !ok { + if g.netWatcher != nil { + if ok := g.netWatcher.WaitSettled(parentCtx, quickReconnectBudget, verdictSettleWindow); !ok { return false } // Still offline after the budget: leave the retry to the ticker. - if !g.netState.IsOnline() { + if !g.netWatcher.IsOnline() { return false } } else { - if cancelled := waiteBeforeRetry(parentCtx); !cancelled { + if cancelled := waitBeforeRetry(parentCtx); !cancelled { return false } } @@ -210,7 +210,7 @@ func (g *Guard) exponentTicker(ctx context.Context) *backoff.Ticker { return backoff.NewTicker(bo) } -func waiteBeforeRetry(ctx context.Context) bool { +func waitBeforeRetry(ctx context.Context) bool { timer := time.NewTimer(quickReconnectBudget) defer timer.Stop() diff --git a/shared/signal/client/grpc.go b/shared/signal/client/grpc.go index f9bc7c737..37e64b89a 100644 --- a/shared/signal/client/grpc.go +++ b/shared/signal/client/grpc.go @@ -66,9 +66,9 @@ type GrpcClient struct { connStateCallback ConnStateNotifier connStateCallbackLock sync.RWMutex - // netEvents gates the Receive retry loop on OS-reported network + // netMgr gates the Receive retry loop on OS-reported network // availability and sweeps the transport on network change. - netEvents *netevents.Manager + netMgr *netevents.Manager onReconnectedListenerFn func() @@ -98,7 +98,7 @@ type Option func(*GrpcClient) // WithNetEvents injects the OS network event handling. func WithNetEvents(events *netevents.Manager) Option { - return func(c *GrpcClient) { c.netEvents = events } + return func(c *GrpcClient) { c.netMgr = events } } // NewClient creates a new Signal client @@ -116,7 +116,7 @@ func NewClient(ctx context.Context, addr string, key wgtypes.Key, tlsEnabled boo } var extraOpts []grpc.DialOption - extraOpts = append(extraOpts, nbgrpc.WithSweeper(c.netEvents)) + extraOpts = append(extraOpts, nbgrpc.WithSweeper(c.netMgr)) var conn *grpc.ClientConn operation := func() error { @@ -186,13 +186,13 @@ func defaultBackoff(ctx context.Context) backoff.BackOff { // The connection retry logic will try to reconnect for 30 min and if wasn't successful will propagate the error to the function caller. func (c *GrpcClient) Receive(ctx context.Context, msgHandler func(msg *proto.Message) error) error { - backOff := c.netEvents.QuickRetryBackoff(ctx, defaultBackoff(ctx)) + backOff := c.netMgr.QuickRetryBackoff(ctx, defaultBackoff(ctx)) operation := func() error { // suspend reconnect attempts while the OS reports no usable network. // Wait only errors on a cancelled context, which means shutdown, so // stop the loop without reporting a failure. - if waited, err := c.netEvents.Wait(ctx); err != nil { + if waited, err := c.netMgr.Wait(ctx); err != nil { log.Debugf("signal connection context has been canceled while offline, this usually indicates shutdown") return nil } else if waited { @@ -272,7 +272,7 @@ func (c *GrpcClient) Receive(ctx context.Context, msgHandler func(msg *proto.Mes return nil } - err := nbgrpc.Retry(ctx, operation, backOff, c.netEvents) + err := nbgrpc.Retry(ctx, operation, backOff, c.netMgr) if err != nil { log.Errorf("exiting the Signal service connection retry loop due to the unrecoverable error: %v", err) return err