Merge pull request #2696 from calmh/fix2694

Improve API/GUI shutdown handling (fixes #2694)

cmd/syncthing/gui.go

@@ -57,10 +57,14 @@ type apiService struct {
 	eventSub        *events.BufferedSubscription
 	discoverer      *discover.CachingMux
 	relayService    *relay.Service
-	listener        net.Listener
 	fss             *folderSummaryService
-	stop            chan struct{}
+	systemConfigMut sync.Mutex    // serializes posts to /rest/system/config
-	systemConfigMut sync.Mutex
+	stop            chan struct{} // signals intentional stop
+	configChanged   chan struct{} // signals intentional listener close due to config change
+	started         chan struct{} // signals startup complete, for testing only
+
+	listener    net.Listener
+	listenerMut sync.Mutex
 
 	guiErrors *logger.Recorder
 	systemLog *logger.Recorder
@@ -76,6 +80,9 @@ func newAPIService(id protocol.DeviceID, cfg *config.Wrapper, assetDir string, m
 		discoverer:      discoverer,
 		relayService:    relayService,
 		systemConfigMut: sync.NewMutex(),
+		stop:            make(chan struct{}),
+		configChanged:   make(chan struct{}),
+		listenerMut:     sync.NewMutex(),
 		guiErrors:       errors,
 		systemLog:       systemLog,
 	}
@@ -146,7 +153,15 @@ func sendJSON(w http.ResponseWriter, jsonObject interface{}) {
 }
 
 func (s *apiService) Serve() {
-	s.stop = make(chan struct{})
+	s.listenerMut.Lock()
+	listener := s.listener
+	s.listenerMut.Unlock()
+
+	if listener == nil {
+		// Not much we can do here other than exit quickly. The supervisor
+		// will log an error at some point.
+		return
+	}
 
 	// The GET handlers
 	getRestMux := http.NewServeMux()
@@ -249,23 +264,37 @@ func (s *apiService) Serve() {
 	defer s.fss.Stop()
 	s.fss.ServeBackground()
 
-	l.Infoln("API listening on", s.listener.Addr())
+	l.Infoln("API listening on", listener.Addr())
 	l.Infoln("GUI URL is", guiCfg.URL())
-	err := srv.Serve(s.listener)
+	if s.started != nil {
+		// only set when run by the tests
+		close(s.started)
+	}
+	err := srv.Serve(listener)
 
 	// The return could be due to an intentional close. Wait for the stop
 	// signal before returning. IF there is no stop signal within a second, we
 	// assume it was unintentional and log the error before retrying.
 	select {
 	case <-s.stop:
+	case <-s.configChanged:
 	case <-time.After(time.Second):
 		l.Warnln("API:", err)
 	}
 }
 
 func (s *apiService) Stop() {
+	s.listenerMut.Lock()
+	listener := s.listener
+	s.listenerMut.Unlock()
+
 	close(s.stop)
-	s.listener.Close()
+
+	// listener may be nil here if we've had a config change to a broken
+	// configuration, in which case we shouldn't try to close it.
+	if listener != nil {
+		listener.Close()
+	}
 }
 
 func (s *apiService) String() string {
@@ -285,7 +314,10 @@ func (s *apiService) CommitConfiguration(from, to config.Configuration) bool {
 	// must create a new listener before Serve() starts again. We can't create
 	// a new listener on the same port before the previous listener is closed.
 	// To assist in this little dance the Serve() method will wait for a
-	// signal on the stop channel after the listener has closed.
+	// signal on the configChanged channel after the listener has closed.
+
+	s.listenerMut.Lock()
+	defer s.listenerMut.Unlock()
 
 	s.listener.Close()
 
@@ -299,7 +331,7 @@ func (s *apiService) CommitConfiguration(from, to config.Configuration) bool {
 		return false
 	}
 
-	close(s.stop)
+	s.configChanged <- struct{}{}
 
 	return true
 }

cmd/syncthing/gui_test.go

@@ -6,7 +6,13 @@
 
 package main
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/syncthing/syncthing/lib/config"
+	"github.com/syncthing/syncthing/lib/protocol"
+	"github.com/thejerf/suture"
+)
 
 func TestCSRFToken(t *testing.T) {
 	t1 := newCsrfToken()
@@ -40,3 +46,46 @@ func TestCSRFToken(t *testing.T) {
 		t.Fatal("t3 should have expired by now")
 	}
 }
+
+func TestStopAfterBrokenConfig(t *testing.T) {
+	baseDirs["config"] = "../../test/h1" // to load HTTPS keys
+	expandLocations()
+
+	cfg := config.Configuration{
+		GUI: config.GUIConfiguration{
+			RawAddress: "127.0.0.1:0",
+			RawUseTLS:  false,
+		},
+	}
+	w := config.Wrap("/dev/null", cfg)
+
+	srv, err := newAPIService(protocol.LocalDeviceID, w, "", nil, nil, nil, nil, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	srv.started = make(chan struct{})
+
+	sup := suture.NewSimple("test")
+	sup.Add(srv)
+	sup.ServeBackground()
+
+	<-srv.started
+
+	// Service is now running, listening on a random port on localhost. Now we
+	// request a config change to a completely invalid listen address. The
+	// commit will fail and the service will be in a broken state.
+
+	newCfg := config.Configuration{
+		GUI: config.GUIConfiguration{
+			RawAddress: "totally not a valid address",
+			RawUseTLS:  false,
+		},
+	}
+	if srv.CommitConfiguration(cfg, newCfg) {
+		t.Fatal("Config commit should have failed")
+	}
+
+	// Nonetheless, it should be fine to Stop() it without panic.
+
+	sup.Stop()
+}