Improve API/GUI shutdown handling (fixes #2694)

This fixes both a race condition where we could assign s.stop from one
goroutine and then read it from another without locking, and handles the
fact that listener may be nil at shutdown if we've had a bad
CommitConfiguration call in the meantime.

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
+	systemConfigMut sync.Mutex    // serializes posts to /rest/system/config
+	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()
+}