wip

internal/configuration/settings/shadowsocks.go

@@ -22,7 +22,7 @@ func (s Shadowsocks) validate() (err error) {
 	return s.Settings.Validate()
 }
 
-func (s *Shadowsocks) copy() (copied Shadowsocks) {
+func (s *Shadowsocks) Copy() (copied Shadowsocks) {
 	return Shadowsocks{
 		Enabled:  gosettings.CopyPointer(s.Enabled),
 		Settings: s.Settings.Copy(),
@@ -32,7 +32,7 @@ func (s *Shadowsocks) copy() (copied Shadowsocks) {
 // overrideWith overrides fields of the receiver
 // settings object with any field set in the other
 // settings.
-func (s *Shadowsocks) overrideWith(other Shadowsocks) {
+func (s *Shadowsocks) OverrideWith(other Shadowsocks) {
 	s.Enabled = gosettings.OverrideWithPointer(s.Enabled, other.Enabled)
 	s.Settings.OverrideWith(other.Settings)
 }

internal/shadowsocks/loop.go

@@ -2,14 +2,11 @@ package shadowsocks
 
 import (
 	"context"
-	"fmt"
-	"sync"
 	"time"
 
 	"github.com/qdm12/gluetun/internal/configuration/settings"
 	"github.com/qdm12/gluetun/internal/constants"
 	"github.com/qdm12/gluetun/internal/models"
-	shadowsockslib "github.com/qdm12/ss-server/pkg/tcpudp"
 )
 
 type Loop struct {
@@ -17,10 +14,9 @@ type Loop struct {
 	// Other objects
 	logger Logger
 	// Internal channels and locks
-	loopLock      sync.Mutex
+	refreshing  bool
-	running       chan models.LoopStatus
+	refresh     chan struct{}
-	stop, stopped chan struct{}
+	changed     chan models.LoopStatus
-	start         chan struct{}
 	backoffTime time.Duration
 
 	runCancel context.CancelFunc
@@ -36,10 +32,8 @@ func NewLoop(settings settings.Shadowsocks, logger Logger) *Loop {
 			settings: settings,
 		},
 		logger:      logger,
-		start:       make(chan struct{}),
+		refresh:     make(chan struct{}, 1), // capacity of 1 to handle crash auto-restart
-		running:     make(chan models.LoopStatus),
+		changed:     make(chan models.LoopStatus),
-		stop:        make(chan struct{}),
-		stopped:     make(chan struct{}),
 		backoffTime: defaultBackoffTime,
 	}
 }
@@ -55,13 +49,6 @@ func (l *Loop) Start(ctx context.Context) (runError <-chan error, err error) {
 
 	<-ready
 
-	if *l.GetSettings().Enabled {
-		_, err = l.SetStatus(ctx, constants.Running)
-		if err != nil {
-			return nil, fmt.Errorf("setting running status: %w", err)
-		}
-	}
-
 	return nil, nil //nolint:nilnil
 }
 
@@ -69,80 +56,61 @@ func (l *Loop) run(ctx context.Context, ready, done chan<- struct{}) {
 	defer close(done)
 	close(ready)
 
-	select {
-	case <-l.start:
-	case <-ctx.Done():
-		return
-	}
-
-	crashed := false
-
 	for ctx.Err() == nil {
+		// What if update and crash at the same time ish?
 		settings := l.GetSettings()
-		server, err := shadowsockslib.NewServer(settings.Settings, l.logger)
+
+		var service *service
+		var runError <-chan error
+		var err error
+		if *settings.Enabled {
+			service = newService(settings.Settings, l.logger)
+			runError, err = service.Start(ctx)
 			if err != nil {
-			crashed = true
+				runErrorCh := make(chan error, 1)
-			l.logAndWait(ctx, err)
+				runError = runErrorCh
-			continue
+				runErrorCh <- err
-		}
+			} else if l.refreshing {
-
+				l.changed <- constants.Running
-		shadowsocksCtx, shadowsocksCancel := context.WithCancel(ctx)
+			} else { // auto-restart due to crash
-
-		waitError := make(chan error)
-		go func() {
-			waitError <- server.Listen(shadowsocksCtx)
-		}()
-		if err != nil {
-			crashed = true
-			shadowsocksCancel()
-			l.logAndWait(ctx, err)
-			continue
-		}
-
-		isStableTimer := time.NewTimer(time.Second)
-
-		stayHere := true
-		for stayHere {
-			select {
-			case <-ctx.Done():
-				shadowsocksCancel()
-				<-waitError
-				close(waitError)
-				return
-			case <-isStableTimer.C:
-				if !crashed {
-					l.running <- constants.Running
-					crashed = false
-				} else {
-					l.backoffTime = defaultBackoffTime
 				l.state.setStatusWithLock(constants.Running)
+				l.backoffTime = defaultBackoffTime
+			}
+		} else {
+			if l.refreshing {
+				l.changed <- constants.Stopped
+			} else { // auto-restart due to crash
+				l.state.setStatusWithLock(constants.Stopped)
+				l.backoffTime = defaultBackoffTime
+			}
+		}
+		l.refreshing = false
+
+		select {
+		case <-l.refresh:
+			l.refreshing = true
+			if service != nil {
+				err = service.Stop()
+				if err != nil {
+					l.logger.Error("stopping service: " + err.Error())
+				}
+			}
+		case err = <-runError:
+			if l.refreshing {
+				l.changed <- constants.Crashed
+			} else {
+				l.state.setStatusWithLock(constants.Crashed)
+			}
+			l.logAndWait(ctx, err)
+		case <-ctx.Done():
+			if service != nil {
+				err = service.Stop()
+				if err != nil {
+					l.logger.Error("stopping service: " + err.Error())
+				}
 			}
-			case <-l.start:
-				l.logger.Info("starting")
-				shadowsocksCancel()
-				<-waitError
-				close(waitError)
-				stayHere = false
-			case <-l.stop:
-				l.logger.Info("stopping")
-				shadowsocksCancel()
-				<-waitError
-				close(waitError)
-				l.stopped <- struct{}{}
-			case err := <-waitError: // unexpected error
-				shadowsocksCancel()
-				close(waitError)
-				if ctx.Err() != nil {
 			return
 		}
-				l.state.setStatusWithLock(constants.Crashed)
-				l.logAndWait(ctx, err)
-				crashed = true
-				stayHere = false
-			}
-		}
-		shadowsocksCancel() // repetition for linter only
-		isStableTimer.Stop()
 	}
 }
 
@@ -162,8 +130,7 @@ func (l *Loop) logAndWait(ctx context.Context, err error) {
 	select {
 	case <-timer.C:
 	case <-ctx.Done():
-		if !timer.Stop() {
+		_ = timer.Stop()
-			<-timer.C
+	case <-l.refresh: // user-triggered refresh
-		}
 	}
 }

internal/shadowsocks/noop.go

@@ -0,0 +1,14 @@
+package shadowsocks
+
+import "context"
+
+type noopService struct{}
+
+func (n *noopService) Start(_ context.Context) (
+	runError <-chan error, err error) {
+	return nil, nil
+}
+
+func (n *noopService) Stop() (err error) {
+	return nil
+}

internal/shadowsocks/service.go

@@ -0,0 +1,67 @@
+package shadowsocks
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	"github.com/qdm12/ss-server/pkg/tcpudp"
+)
+
+type service struct {
+	// Injected settings
+	settings tcpudp.Settings
+	logger   Logger
+	// Internal fields
+	cancel context.CancelFunc
+	done   <-chan struct{}
+}
+
+func newService(settings tcpudp.Settings,
+	logger Logger) *service {
+	return &service{
+		settings: settings,
+		logger:   logger,
+	}
+}
+
+func (s *service) Start(ctx context.Context) (runError <-chan error, err error) {
+	server, err := tcpudp.NewServer(s.settings, s.logger)
+	if err != nil {
+		return nil, fmt.Errorf("creating server: %w", err)
+	}
+
+	shadowsocksCtx, shadowsocksCancel := context.WithCancel(context.Background())
+	s.cancel = shadowsocksCancel
+	runErrorCh := make(chan error)
+	done := make(chan struct{})
+	s.done = done
+	go func() {
+		defer close(done)
+		err = server.Listen(shadowsocksCtx)
+		if shadowsocksCtx.Err() == nil {
+			runErrorCh <- fmt.Errorf("listening: %w", err)
+		}
+	}()
+
+	const minStabilityTime = 100 * time.Millisecond
+	isStableTimer := time.NewTimer(minStabilityTime)
+	select {
+	case <-isStableTimer.C:
+	case err = <-runErrorCh:
+		return nil, fmt.Errorf("server became unstable within %s: %w",
+			minStabilityTime, err)
+	case <-ctx.Done():
+		shadowsocksCancel()
+		<-done
+		return nil, ctx.Err()
+	}
+
+	return runErrorCh, nil
+}
+
+func (s *service) Stop() (err error) {
+	s.cancel()
+	<-s.done
+	return nil
+}

internal/shadowsocks/state.go

@@ -1,14 +1,10 @@
 package shadowsocks
 
 import (
-	"context"
-	"errors"
-	"fmt"
 	"reflect"
 	"sync"
 
 	"github.com/qdm12/gluetun/internal/configuration/settings"
-	"github.com/qdm12/gluetun/internal/constants"
 	"github.com/qdm12/gluetun/internal/models"
 )
 
@@ -25,95 +21,34 @@ func (s *state) setStatusWithLock(status models.LoopStatus) {
 	s.status = status
 }
 
+// GetStatus returns the status of the loop for informative purposes.
+// In no case it should be used programmatically to avoid any
+// TOCTOU race conditions.
 func (l *Loop) GetStatus() (status models.LoopStatus) {
 	l.state.statusMu.RLock()
 	defer l.state.statusMu.RUnlock()
 	return l.state.status
 }
 
-var ErrInvalidStatus = errors.New("invalid status")
-
-func (l *Loop) SetStatus(ctx context.Context, 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 := constants.Starting // for canceled context
-		select {
-		case <-ctx.Done():
-		case 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{}{}
-		newStatus := constants.Stopping // for canceled context
-		select {
-		case <-ctx.Done():
-		case <-l.stopped:
-			newStatus = constants.Stopped
-		}
-		l.state.statusMu.Lock()
-		l.state.status = newStatus
-		return status.String(), nil
-	default:
-		return "", fmt.Errorf("%w: %s: it can only be one of: %s, %s",
-			ErrInvalidStatus, status, constants.Running, constants.Stopped)
-	}
-}
-
 func (l *Loop) GetSettings() (settings settings.Shadowsocks) {
 	l.state.settingsMu.RLock()
 	defer l.state.settingsMu.RUnlock()
 	return l.state.settings
 }
 
-func (l *Loop) SetSettings(ctx context.Context, settings settings.Shadowsocks) (
+func (l *Loop) UpdateSettings(updateSettings settings.Shadowsocks) (outcome string) {
-	outcome string,
-) {
 	l.state.settingsMu.Lock()
-	settingsUnchanged := reflect.DeepEqual(settings, l.state.settings)
+	previousSettings := l.state.settings.Copy()
-	if settingsUnchanged {
+	l.state.settings.OverrideWith(updateSettings)
+	settingsUnchanged := reflect.DeepEqual(previousSettings, l.state.settings)
 	l.state.settingsMu.Unlock()
+	if settingsUnchanged {
 		return "settings left unchanged"
 	}
-	newEnabled := *settings.Enabled
+	l.refresh <- struct{}{}
-	previousEnabled := *l.state.settings.Enabled
+	newStatus := <-l.changed
-	l.state.settings = settings
+	l.state.statusMu.Lock()
-	l.state.settingsMu.Unlock()
+	l.state.status = newStatus
-	// Either restart or set changed status
+	l.state.statusMu.Unlock()
-	switch {
+	return "settings updated (service " + newStatus.String() + ")"
-	case !newEnabled && !previousEnabled:
-	case newEnabled && previousEnabled:
-		_, _ = l.SetStatus(ctx, constants.Stopped)
-		_, _ = l.SetStatus(ctx, constants.Running)
-	case newEnabled && !previousEnabled:
-		_, _ = l.SetStatus(ctx, constants.Running)
-	case !newEnabled && previousEnabled:
-		_, _ = l.SetStatus(ctx, constants.Stopped)
-	}
-	return "settings updated"
 }