diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index 38b9672e0..6410d15df 100644 --- a/cmd/syncthing/gui.go +++ b/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 } diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index f286fc2e4..2abfb0c3c 100644 --- a/cmd/syncthing/gui_test.go +++ b/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() +}