lib/connections, lib/nat: Correctly dis-/enable nat (fixes #6552) (#6719)

lib/connections/service.go

@@ -123,7 +123,6 @@ type service struct {
 	tlsDefaultCommonName string
 	limiter              *limiter
 	natService           *nat.Service
-	natServiceToken      *suture.ServiceToken
 	evLogger             events.Logger
 
 	listenersMut       sync.RWMutex
@@ -188,6 +187,7 @@ func NewService(cfg config.Wrapper, myID protocol.DeviceID, mdl Model, tlsCfg *t
 	service.Add(util.AsService(service.connect, fmt.Sprintf("%s/connect", service)))
 	service.Add(util.AsService(service.handle, fmt.Sprintf("%s/handle", service)))
 	service.Add(service.listenerSupervisor)
+	service.Add(service.natService)
 
 	return service
 }
@@ -652,16 +652,6 @@ func (s *service) CommitConfiguration(from, to config.Configuration) bool {
 	}
 	s.listenersMut.Unlock()
 
-	if to.Options.NATEnabled && s.natServiceToken == nil {
-		l.Debugln("Starting NAT service")
-		token := s.Add(s.natService)
-		s.natServiceToken = &token
-	} else if !to.Options.NATEnabled && s.natServiceToken != nil {
-		l.Debugln("Stopping NAT service")
-		s.Remove(*s.natServiceToken)
-		s.natServiceToken = nil
-	}
-
 	return true
 }
 

lib/nat/service.go

@@ -45,9 +45,36 @@ func NewService(id protocol.DeviceID, cfg config.Wrapper) *Service {
 		mut:   sync.NewRWMutex(),
 	}
 	s.Service = util.AsService(s.serve, s.String())
+	cfg.Subscribe(s)
 	return s
 }
 
+func (s *Service) VerifyConfiguration(from, to config.Configuration) error {
+	return nil
+}
+
+func (s *Service) CommitConfiguration(from, to config.Configuration) bool {
+	if !from.Options.NATEnabled && to.Options.NATEnabled {
+		s.mut.Lock()
+		l.Debugln("Starting NAT service")
+		s.timer.Reset(0)
+		s.mut.Unlock()
+	} else if from.Options.NATEnabled && !to.Options.NATEnabled {
+		s.mut.Lock()
+		l.Debugln("Stopping NAT service")
+		if !s.timer.Stop() {
+			<-s.timer.C
+		}
+		s.mut.Unlock()
+	}
+	return true
+}
+
+func (s *Service) Stop() {
+	s.cfg.Unsubscribe(s)
+	s.Service.Stop()
+}
+
 func (s *Service) serve(ctx context.Context) {
 	announce := stdsync.Once{}
 

lib/nat/structs_test.go

@@ -7,9 +7,13 @@
 package nat
 
 import (
+	"io/ioutil"
 	"net"
+	"os"
 	"testing"
 
+	"github.com/syncthing/syncthing/lib/config"
+	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/protocol"
 )
 
@@ -56,7 +60,15 @@ func TestMappingValidGateway(t *testing.T) {
 }
 
 func TestMappingClearAddresses(t *testing.T) {
-	natSvc := NewService(protocol.EmptyDeviceID, nil)
+	tmpFile, err := ioutil.TempFile("", "syncthing-testConfig-")
+	if err != nil {
+		t.Fatal(err)
+	}
+	w := config.Wrap(tmpFile.Name(), config.Configuration{}, events.NoopLogger)
+	defer os.RemoveAll(tmpFile.Name())
+	tmpFile.Close()
+
+	natSvc := NewService(protocol.EmptyDeviceID, w)
 	// Mock a mapped port; avoids the need to actually map a port
 	ip := net.ParseIP("192.168.0.1")
 	m := natSvc.NewMapping(TCP, ip, 1024)