fix(publicip): rework run loop and fix restarts

- Clearing IP data on VPN disconnection clears file
- More efficient partial updates
- Fix loop exit
- Validate settings before updating

cmd/gluetun/main.go

@@ -377,7 +377,10 @@ func _main(ctx context.Context, buildInfo models.BuildInformation,
 	portForwardLogger := logger.New(log.SetComponent("port forwarding"))
 	portForwardLooper := portforward.NewLoop(allSettings.VPN.Provider.PortForwarding,
 		routingConf, httpClient, firewallConf, portForwardLogger, puid, pgid)
-	portForwardRunError, _ := portForwardLooper.Start(context.Background())
+	portForwardRunError, err := portForwardLooper.Start(ctx)
+	if err != nil {
+		return fmt.Errorf("starting port forwarding loop: %w", err)
+	}
 
 	unboundLogger := logger.New(log.SetComponent("dns"))
 	unboundLooper := dns.NewLoop(dnsConf, allSettings.DNS, httpClient,
@@ -397,15 +400,10 @@ func _main(ctx context.Context, buildInfo models.BuildInformation,
 	publicIPLooper := publicip.NewLoop(ipFetcher,
 		logger.New(log.SetComponent("ip getter")),
 		allSettings.PublicIP, puid, pgid)
-	pubIPHandler, pubIPCtx, pubIPDone := goshutdown.NewGoRoutineHandler(
-		"public IP", goroutine.OptionTimeout(defaultShutdownTimeout))
-	go publicIPLooper.Run(pubIPCtx, pubIPDone)
-	otherGroupHandler.Add(pubIPHandler)
-
-	pubIPTickerHandler, pubIPTickerCtx, pubIPTickerDone := goshutdown.NewGoRoutineHandler(
-		"public IP", goroutine.OptionTimeout(defaultShutdownTimeout))
-	go publicIPLooper.RunRestartTicker(pubIPTickerCtx, pubIPTickerDone)
-	tickersGroupHandler.Add(pubIPTickerHandler)
+	publicIPRunError, err := publicIPLooper.Start(ctx)
+	if err != nil {
+		return fmt.Errorf("starting public ip loop: %w", err)
+	}
 
 	updaterLogger := logger.New(log.SetComponent("updater"))
 
@@ -487,12 +485,22 @@ func _main(ctx context.Context, buildInfo models.BuildInformation,
 
 	select {
 	case <-ctx.Done():
-		err = portForwardLooper.Stop()
-		if err != nil {
-			logger.Error("stopping port forward loop: " + err.Error())
+		stoppers := []interface {
+			String() string
+			Stop() error
+		}{
+			portForwardLooper, publicIPLooper,
+		}
+		for _, stopper := range stoppers {
+			err := stopper.Stop()
+			if err != nil {
+				logger.Error(fmt.Sprintf("stopping %s: %s", stopper, err))
+			}
 		}
 	case err := <-portForwardRunError:
 		logger.Errorf("port forwarding loop crashed: %s", err)
+	case err := <-publicIPRunError:
+		logger.Errorf("public IP loop crashed: %s", err)
 	}
 
 	return orderHandler.Shutdown(context.Background())

internal/configuration/settings/publicip.go

@@ -23,6 +23,20 @@ type PublicIP struct {
 	IPFilepath *string
 }
 
+// UpdateWith deep copies the receiving settings, overrides the copy with
+// fields set in the partialUpdate argument, validates the new settings
+// and returns them if they are valid, or returns an error otherwise.
+// In all cases, the receiving settings are unmodified.
+func (p PublicIP) UpdateWith(partialUpdate PublicIP) (updatedSettings PublicIP, err error) {
+	updatedSettings = p.copy()
+	updatedSettings.overrideWith(partialUpdate)
+	err = updatedSettings.validate()
+	if err != nil {
+		return updatedSettings, fmt.Errorf("validating updated settings: %w", err)
+	}
+	return updatedSettings, nil
+}
+
 func (p PublicIP) validate() (err error) {
 	const minPeriod = 5 * time.Second
 	if *p.Period < minPeriod {

internal/portforward/loop.go

@@ -48,6 +48,10 @@ func NewLoop(settings settings.PortForwarding, routing Routing,
 	}
 }
 
+func (l *Loop) String() string {
+	return "port forwarding loop"
+}
+
 func (l *Loop) Start(_ context.Context) (runError <-chan error, _ error) {
 	l.runCtx, l.runCancel = context.WithCancel(context.Background())
 	runDone := make(chan struct{})

internal/publicip/data.go

@@ -0,0 +1,24 @@
+package publicip
+
+import "github.com/qdm12/gluetun/internal/models"
+
+// GetData returns the public IP data obtained from the last
+// fetch. It is notably used by the HTTP control server.
+func (l *Loop) GetData() (data models.PublicIP) {
+	l.ipDataMutex.RLock()
+	defer l.ipDataMutex.RUnlock()
+	return l.ipData
+}
+
+// ClearData is used when the VPN connection goes down
+// and the public IP is not known anymore.
+func (l *Loop) ClearData() (err error) {
+	l.ipDataMutex.Lock()
+	defer l.ipDataMutex.Unlock()
+	l.ipData = models.PublicIP{}
+
+	l.settingsMutex.RLock()
+	filepath := *l.settings.IPFilepath
+	l.settingsMutex.RUnlock()
+	return persistPublicIP(filepath, "", l.puid, l.pgid)
+}

internal/publicip/interfaces.go

@@ -11,3 +11,9 @@ type Fetcher interface {
 	FetchInfo(ctx context.Context, ip netip.Addr) (
 		result ipinfo.Response, err error)
 }
+
+type Logger interface {
+	Info(s string)
+	Warn(s string)
+	Error(s string)
+}

internal/publicip/loop.go

@@ -1,64 +1,172 @@
 package publicip
 
 import (
+	"context"
+	"fmt"
+	"net/netip"
+	"sync"
 	"time"
 
 	"github.com/qdm12/gluetun/internal/configuration/settings"
-	"github.com/qdm12/gluetun/internal/constants"
-	"github.com/qdm12/gluetun/internal/loopstate"
 	"github.com/qdm12/gluetun/internal/models"
-	"github.com/qdm12/gluetun/internal/publicip/state"
+	"github.com/qdm12/gluetun/internal/publicip/ipinfo"
 )
 
 type Loop struct {
-	statusManager *loopstate.State
-	state         *state.State
-	// Objects
+	// State
+	settings      settings.PublicIP
+	settingsMutex sync.RWMutex
+	ipData        models.PublicIP
+	ipDataMutex   sync.RWMutex
+	// Fixed injected objets
 	fetcher Fetcher
 	logger  Logger
-	// Fixed settings
+	// Fixed parameters
 	puid int
 	pgid int
 	// Internal channels and locks
-	start        chan struct{}
-	running      chan models.LoopStatus
-	stop         chan struct{}
-	stopped      chan struct{}
-	updateTicker chan struct{}
-	backoffTime  time.Duration
-	userTrigger  bool
+	// runCtx is used to detect when the loop has exited
+	// when performing an update
+	runCtx        context.Context //nolint:containedctx
+	runCancel     context.CancelFunc
+	runTrigger    chan<- struct{}
+	updateTrigger chan<- settings.PublicIP
+	updatedResult <-chan error
+	runDone       <-chan struct{}
 	// Mock functions
 	timeNow func() time.Time
 }
 
-const defaultBackoffTime = 5 * time.Second
-
 func NewLoop(fetcher Fetcher, logger Logger,
 	settings settings.PublicIP, puid, pgid int) *Loop {
-	start := make(chan struct{})
-	running := make(chan models.LoopStatus)
-	stop := make(chan struct{})
-	stopped := make(chan struct{})
-	updateTicker := make(chan struct{})
+	return &Loop{
+		settings: settings,
+		fetcher:  fetcher,
+		logger:   logger,
+		puid:     puid,
+		pgid:     pgid,
+		timeNow:  time.Now,
+	}
+}
 
-	statusManager := loopstate.New(constants.Stopped, start, running, stop, stopped)
-	state := state.New(statusManager, settings, updateTicker)
+func (l *Loop) String() string {
+	return "public ip loop"
+}
 
-	return &Loop{
-		statusManager: statusManager,
-		state:         state,
-		// Objects
-		fetcher:      fetcher,
-		logger:       logger,
-		puid:         puid,
-		pgid:         pgid,
-		start:        start,
-		running:      running,
-		stop:         stop,
-		stopped:      stopped,
-		updateTicker: updateTicker,
-		userTrigger:  true,
-		backoffTime:  defaultBackoffTime,
-		timeNow:      time.Now,
+func (l *Loop) Start(_ context.Context) (_ <-chan error, err error) {
+	l.runCtx, l.runCancel = context.WithCancel(context.Background())
+	runDone := make(chan struct{})
+	l.runDone = runDone
+	runTrigger := make(chan struct{})
+	l.runTrigger = runTrigger
+	updateTrigger := make(chan settings.PublicIP)
+	l.updateTrigger = updateTrigger
+	updatedResult := make(chan error)
+	l.updatedResult = updatedResult
+
+	go l.run(l.runCtx, runDone, runTrigger, updateTrigger, updatedResult)
+
+	return nil, nil //nolint:nilnil
+}
+
+func (l *Loop) run(runCtx context.Context, runDone chan<- struct{},
+	runTrigger <-chan struct{}, updateTrigger <-chan settings.PublicIP,
+	updatedResult chan<- error) {
+	defer close(runDone)
+
+	timer := time.NewTimer(time.Hour)
+	defer timer.Stop()
+	_ = timer.Stop()
+	timerIsReadyToReset := true
+	lastFetch := time.Unix(0, 0)
+
+	for {
+		select {
+		case <-runCtx.Done():
+			return
+		case <-runTrigger:
+		case <-timer.C:
+			timerIsReadyToReset = true
+		case partialUpdate := <-updateTrigger:
+			var err error
+			timerIsReadyToReset, err = l.update(partialUpdate, lastFetch, timer, timerIsReadyToReset)
+			updatedResult <- err
+			continue
+		}
+
+		result, exit := l.fetchIPData(runCtx)
+		if exit {
+			return
+		}
+
+		message := "Public IP address is " + result.IP.String()
+		message += " (" + result.Country + ", " + result.Region + ", " + result.City + ")"
+		l.logger.Info(message)
+
+		l.ipDataMutex.Lock()
+		l.ipData = result.ToPublicIPModel()
+		l.ipDataMutex.Unlock()
+
+		filepath := *l.settings.IPFilepath
+		err := persistPublicIP(filepath, result.IP.String(), l.puid, l.pgid)
+		if err != nil { // non critical error, which can be fixed with settings updates.
+			l.logger.Error(err.Error())
+		}
+
+		lastFetch = l.timeNow()
+		timerIsReadyToReset = l.updateTimer(*l.settings.Period, lastFetch, timer, timerIsReadyToReset)
+	}
+}
+
+func (l *Loop) fetchIPData(ctx context.Context) (result ipinfo.Response, exit bool) {
+	// keep retrying since settings updates won't change the
+	// behavior of the following code.
+	const defaultBackoffTime = 5 * time.Second
+	backoffTime := defaultBackoffTime
+	for {
+		var err error
+		result, err = l.fetcher.FetchInfo(ctx, netip.Addr{})
+		if err == nil {
+			return result, false
+		}
+
+		exit = ctx.Err() != nil
+		if exit {
+			return result, true
+		}
+
+		l.logger.Error(fmt.Sprintf("%s - retrying in %s", err, backoffTime))
+		select {
+		case <-ctx.Done():
+			return result, true
+		case <-time.After(backoffTime):
+		}
+		const backoffTimeMultipler = 2
+		backoffTime *= backoffTimeMultipler
 	}
 }
+
+func (l *Loop) StartSingleRun() {
+	l.runTrigger <- struct{}{}
+}
+
+func (l *Loop) UpdateWith(partialUpdate settings.PublicIP) (err error) {
+	select {
+	case l.updateTrigger <- partialUpdate:
+		select {
+		case err = <-l.updatedResult:
+			return err
+		case <-l.runCtx.Done():
+			return l.runCtx.Err()
+		}
+	case <-l.runCtx.Done():
+		// loop has been stopped, no update can be done
+		return l.runCtx.Err()
+	}
+}
+
+func (l *Loop) Stop() (err error) {
+	l.runCancel()
+	<-l.runDone
+	return l.ClearData()
+}