Code maintenance: Shadowsocks loop refactor

cmd/gluetun/main.go

@@ -266,16 +266,12 @@ func _main(background context.Context, buildInfo models.BuildInformation,
 	go httpProxyLooper.Run(ctx, wg)
 
 	shadowsocksLooper := shadowsocks.NewLooper(allSettings.ShadowSocks, logger)
-	restartShadowsocks := shadowsocksLooper.Restart
 	wg.Add(1)
 	go shadowsocksLooper.Run(ctx, wg)
 
 	if allSettings.HTTPProxy.Enabled {
 		_, _ = httpProxyLooper.SetStatus(constants.Running)
 	}
-	if allSettings.ShadowSocks.Enabled {
-		restartShadowsocks()
-	}
 
 	wg.Add(1)
 	go routeReadyEvents(ctx, wg, buildInfo, tunnelReadyCh, dnsReadyCh,

internal/shadowsocks/loop.go

@@ -6,6 +6,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/qdm12/gluetun/internal/constants"
+	"github.com/qdm12/gluetun/internal/models"
 	"github.com/qdm12/gluetun/internal/settings"
 	"github.com/qdm12/golibs/logging"
 	shadowsockslib "github.com/qdm12/ss-server/pkg"
@@ -13,20 +15,21 @@ import (
 
 type Looper interface {
 	Run(ctx context.Context, wg *sync.WaitGroup)
-	Restart()
+	SetStatus(status models.LoopStatus) (outcome string, err error)
-	Start()
+	GetStatus() (status models.LoopStatus)
-	Stop()
 	GetSettings() (settings settings.ShadowSocks)
-	SetSettings(settings settings.ShadowSocks)
+	SetSettings(settings settings.ShadowSocks) (outcome string)
 }
 
 type looper struct {
-	settings      settings.ShadowSocks
+	state state
-	settingsMutex sync.RWMutex
+	// Other objects
-	logger        logging.Logger
+	logger logging.Logger
-	restart       chan struct{}
+	// Internal channels and locks
+	loopLock      sync.Mutex
+	running       chan models.LoopStatus
+	stop, stopped chan struct{}
 	start         chan struct{}
-	stop          chan struct{}
 	backoffTime   time.Duration
 }
 
@@ -48,91 +51,55 @@ const defaultBackoffTime = 10 * time.Second
 
 func NewLooper(settings settings.ShadowSocks, logger logging.Logger) Looper {
 	return &looper{
-		settings:    settings,
+		state: state{
+			status:   constants.Stopped,
+			settings: settings,
+		},
 		logger:      logger.WithPrefix("shadowsocks: "),
-		restart:     make(chan struct{}),
 		start:       make(chan struct{}),
+		running:     make(chan models.LoopStatus),
 		stop:        make(chan struct{}),
+		stopped:     make(chan struct{}),
 		backoffTime: defaultBackoffTime,
 	}
 }
 
-func (l *looper) Restart() { l.restart <- struct{}{} }
-func (l *looper) Start()   { l.start <- struct{}{} }
-func (l *looper) Stop()    { l.stop <- struct{}{} }
-
-func (l *looper) GetSettings() (settings settings.ShadowSocks) {
-	l.settingsMutex.RLock()
-	defer l.settingsMutex.RUnlock()
-	return l.settings
-}
-
-func (l *looper) SetSettings(settings settings.ShadowSocks) {
-	l.settingsMutex.Lock()
-	defer l.settingsMutex.Unlock()
-	l.settings = settings
-}
-
-func (l *looper) isEnabled() bool {
-	l.settingsMutex.RLock()
-	defer l.settingsMutex.RUnlock()
-	return l.settings.Enabled
-}
-
-func (l *looper) setEnabled(enabled bool) {
-	l.settingsMutex.Lock()
-	defer l.settingsMutex.Unlock()
-	l.settings.Enabled = enabled
-}
-
 func (l *looper) Run(ctx context.Context, wg *sync.WaitGroup) {
 	defer wg.Done()
-	waitForStart := true
+
-	for waitForStart {
+	crashed := false
-		select {
+
-		case <-l.stop:
+	if l.GetSettings().Enabled {
-			l.logger.Info("not started yet")
+		go func() {
-		case <-l.start:
+			_, _ = l.SetStatus(constants.Running)
-			waitForStart = false
+		}()
-		case <-l.restart:
-			waitForStart = false
-		case <-ctx.Done():
-			return
-		}
 	}
+
+	select {
+	case <-l.start:
+	case <-ctx.Done():
+		return
+	}
+
 	defer l.logger.Warn("loop exited")
 
-	l.setEnabled(true)
-
 	for ctx.Err() == nil {
-		for !l.isEnabled() {
-			// wait for a signal to re-enable
-			select {
-			case <-l.stop:
-				l.logger.Info("already disabled")
-			case <-l.restart:
-				l.setEnabled(true)
-			case <-l.start:
-				l.setEnabled(true)
-			case <-ctx.Done():
-				return
-			}
-		}
-
 		settings := l.GetSettings()
 		server, err := shadowsockslib.NewServer(settings.Method, settings.Password, adaptLogger(l.logger, settings.Log))
 		if err != nil {
+			crashed = true
 			l.logAndWait(ctx, err)
 			continue
 		}
 
-		shadowsocksCtx, shadowsocksCancel := context.WithCancel(context.Background())
+		shadowsocksCtx, shadowsocksCancel := context.WithCancel(ctx)
 
 		waitError := make(chan error)
 		go func() {
 			waitError <- server.Listen(shadowsocksCtx, fmt.Sprintf("0.0.0.0:%d", settings.Port))
 		}()
 		if err != nil {
+			crashed = true
 			shadowsocksCancel()
 			l.logAndWait(ctx, err)
 			continue
@@ -150,26 +117,32 @@ func (l *looper) Run(ctx context.Context, wg *sync.WaitGroup) {
 				close(waitError)
 				return
 			case <-isStableTimer.C:
-				l.backoffTime = defaultBackoffTime
+				if !crashed {
-			case <-l.restart: // triggered restart
+					l.running <- constants.Running
-				l.logger.Info("restarting")
+					crashed = false
+				} else {
+					l.backoffTime = defaultBackoffTime
+					l.state.setStatusWithLock(constants.Running)
+				}
+			case <-l.start:
+				l.logger.Info("starting")
 				shadowsocksCancel()
 				<-waitError
 				close(waitError)
 				stayHere = false
-			case <-l.start:
-				l.logger.Info("already started")
 			case <-l.stop:
 				l.logger.Info("stopping")
 				shadowsocksCancel()
 				<-waitError
 				close(waitError)
-				l.setEnabled(false)
+				l.stopped <- struct{}{}
-				stayHere = false
 			case err := <-waitError: // unexpected error
 				shadowsocksCancel()
 				close(waitError)
+				l.state.setStatusWithLock(constants.Crashed)
 				l.logAndWait(ctx, err)
+				crashed = true
+				stayHere = false
 			}
 		}
 		shadowsocksCancel() // repetition for linter only

internal/shadowsocks/state.go

@@ -0,0 +1,101 @@
+package shadowsocks
+
+import (
+	"fmt"
+	"reflect"
+	"sync"
+
+	"github.com/qdm12/gluetun/internal/constants"
+	"github.com/qdm12/gluetun/internal/models"
+	"github.com/qdm12/gluetun/internal/settings"
+)
+
+type state struct {
+	status     models.LoopStatus
+	settings   settings.ShadowSocks
+	statusMu   sync.RWMutex
+	settingsMu sync.RWMutex
+}
+
+func (s *state) setStatusWithLock(status models.LoopStatus) {
+	s.statusMu.Lock()
+	defer s.statusMu.Unlock()
+	s.status = status
+}
+
+func (l *looper) GetStatus() (status models.LoopStatus) {
+	l.state.statusMu.RLock()
+	defer l.state.statusMu.RUnlock()
+	return l.state.status
+}
+
+func (l *looper) SetStatus(status models.LoopStatus) (outcome string, err error) {
+	l.state.statusMu.Lock()
+	defer l.state.statusMu.Unlock()
+	existingStatus := l.state.status
+
+	switch status {
+	case constants.Running:
+		switch existingStatus {
+		case constants.Starting, constants.Running, constants.Stopping, constants.Crashed:
+			return fmt.Sprintf("already %s", existingStatus), nil
+		}
+		l.loopLock.Lock()
+		defer l.loopLock.Unlock()
+		l.state.status = constants.Starting
+		l.state.statusMu.Unlock()
+		l.start <- struct{}{}
+		newStatus := <-l.running
+		l.state.statusMu.Lock()
+		l.state.status = newStatus
+		return newStatus.String(), nil
+	case constants.Stopped:
+		switch existingStatus {
+		case constants.Stopped, constants.Stopping, constants.Starting, constants.Crashed:
+			return fmt.Sprintf("already %s", existingStatus), nil
+		}
+		l.loopLock.Lock()
+		defer l.loopLock.Unlock()
+		l.state.status = constants.Stopping
+		l.state.statusMu.Unlock()
+		l.stop <- struct{}{}
+		<-l.stopped
+		l.state.statusMu.Lock()
+		l.state.status = status
+		return status.String(), nil
+	default:
+		return "", fmt.Errorf("status %q can only be %q or %q",
+			status, constants.Running, constants.Stopped)
+	}
+}
+
+func (l *looper) GetSettings() (settings settings.ShadowSocks) {
+	l.state.settingsMu.RLock()
+	defer l.state.settingsMu.RUnlock()
+	return l.state.settings
+}
+
+func (l *looper) SetSettings(settings settings.ShadowSocks) (outcome string) {
+	l.state.settingsMu.Lock()
+	settingsUnchanged := reflect.DeepEqual(settings, l.state.settings)
+	if settingsUnchanged {
+		l.state.settingsMu.Unlock()
+		return "settings left unchanged"
+	}
+	newEnabled := settings.Enabled
+	previousEnabled := l.state.settings.Enabled
+	l.state.settings = settings
+	l.state.settingsMu.Unlock()
+	// Either restart or set changed status
+	switch {
+	case !newEnabled && !previousEnabled:
+	case newEnabled && previousEnabled:
+		_, _ = l.SetStatus(constants.Stopped)
+		_, _ = l.SetStatus(constants.Running)
+	case newEnabled && !previousEnabled:
+		_, _ = l.SetStatus(constants.Running)
+	case !newEnabled && previousEnabled:
+		_, _ = l.SetStatus(constants.Stopped)
+	}
+	return "settings updated"
+}