diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index a47712e2..22fbaf5c 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -64,10 +64,8 @@ type apiService struct { 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 + started chan string // signals startup complete by sending the listener address, for testing only + startedOnce bool // the service has started successfully at least once guiErrors logger.Recorder systemLog logger.Recorder @@ -119,7 +117,7 @@ type connectionsIntf interface { Status() map[string]interface{} } -func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKeyFile, assetDir string, m modelIntf, eventSub events.BufferedSubscription, discoverer discover.CachingMux, connectionsService connectionsIntf, errors, systemLog logger.Recorder) (*apiService, error) { +func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKeyFile, assetDir string, m modelIntf, eventSub events.BufferedSubscription, discoverer discover.CachingMux, connectionsService connectionsIntf, errors, systemLog logger.Recorder) *apiService { service := &apiService{ id: id, cfg: cfg, @@ -133,7 +131,6 @@ func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKey systemConfigMut: sync.NewMutex(), stop: make(chan struct{}), configChanged: make(chan struct{}), - listenerMut: sync.NewMutex(), guiErrors: errors, systemLog: systemLog, } @@ -157,9 +154,7 @@ func newAPIService(id protocol.DeviceID, cfg configIntf, httpsCertFile, httpsKey } } - var err error - service.listener, err = service.getListener(cfg.GUI()) - return service, err + return service } func (s *apiService) getListener(guiCfg config.GUIConfiguration) (net.Listener, error) { @@ -226,9 +221,22 @@ func sendJSON(w http.ResponseWriter, jsonObject interface{}) { } func (s *apiService) Serve() { - s.listenerMut.Lock() - listener := s.listener - s.listenerMut.Unlock() + listener, err := s.getListener(s.cfg.GUI()) + if err != nil { + if !s.startedOnce { + // This is during initialization. A failure here should be fatal + // as there will be no way for the user to communicate with us + // otherwise anyway. + l.Fatalln("Starting API/GUI:", err) + } + + // We let this be a loud user-visible warning as it may be the only + // indication they get that the GUI won't be available on startup. + l.Warnln("Starting API/GUI:", err) + return + } + s.startedOnce = true + defer listener.Close() if listener == nil { // Not much we can do here other than exit quickly. The supervisor @@ -348,33 +356,33 @@ func (s *apiService) Serve() { l.Infoln("Access the GUI via the following URL:", guiCfg.URL()) if s.started != nil { // only set when run by the tests - close(s.started) + s.started <- listener.Addr().String() } - 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. + // Serve in the background + + serveError := make(chan error, 1) + go func() { + serveError <- srv.Serve(listener) + }() + + // Wait for stop, restart or error signals + select { case <-s.stop: + // Shutting down permanently + l.Debugln("shutting down (stop)") case <-s.configChanged: - case <-time.After(time.Second): - l.Warnln("API:", err) + // Soft restart due to configuration change + l.Debugln("restarting (config changed)") + case <-serveError: + // Restart due to listen/serve failure + l.Warnln("GUI/API:", err, "(restarting)") } } func (s *apiService) Stop() { - s.listenerMut.Lock() - listener := s.listener - s.listenerMut.Unlock() - close(s.stop) - - // 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 { @@ -382,6 +390,9 @@ func (s *apiService) String() string { } func (s *apiService) VerifyConfiguration(from, to config.Configuration) error { + if _, err := net.ResolveTCPAddr("tcp", to.GUI.Address()); err != nil { + return err + } return nil } @@ -390,27 +401,7 @@ func (s *apiService) CommitConfiguration(from, to config.Configuration) bool { return true } - // Order here is important. We must close the listener to stop Serve(). We - // 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 configChanged channel after the listener has closed. - - s.listenerMut.Lock() - defer s.listenerMut.Unlock() - - s.listener.Close() - - var err error - s.listener, err = s.getListener(to.GUI) - if err != nil { - // Ideally this should be a verification error, but we check it by - // creating a new listener which requires shutting down the previous - // one first, which is too destructive for the VerifyConfiguration - // method. - return false - } - + // Tell the serve loop to restart s.configChanged <- struct{}{} return true diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go index f97d6b8c..2f6ece6a 100644 --- a/cmd/syncthing/gui_test.go +++ b/cmd/syncthing/gui_test.go @@ -68,11 +68,8 @@ func TestStopAfterBrokenConfig(t *testing.T) { } w := config.Wrap("/dev/null", cfg) - srv, err := newAPIService(protocol.LocalDeviceID, w, "../../test/h1/https-cert.pem", "../../test/h1/https-key.pem", "", nil, nil, nil, nil, nil, nil) - if err != nil { - t.Fatal(err) - } - srv.started = make(chan struct{}) + srv := newAPIService(protocol.LocalDeviceID, w, "../../test/h1/https-cert.pem", "../../test/h1/https-key.pem", "", nil, nil, nil, nil, nil, nil) + srv.started = make(chan string) sup := suture.NewSimple("test") sup.Add(srv) @@ -90,8 +87,8 @@ func TestStopAfterBrokenConfig(t *testing.T) { RawUseTLS: false, }, } - if srv.CommitConfiguration(cfg, newCfg) { - t.Fatal("Config commit should have failed") + if err := srv.VerifyConfiguration(cfg, newCfg); err == nil { + t.Fatal("Verify config should have failed") } // Nonetheless, it should be fine to Stop() it without panic. @@ -475,30 +472,26 @@ func startHTTP(cfg *mockedConfig) (string, error) { connections := new(mockedConnections) errorLog := new(mockedLoggerRecorder) systemLog := new(mockedLoggerRecorder) + addrChan := make(chan string) // Instantiate the API service - svc, err := newAPIService(protocol.LocalDeviceID, cfg, httpsCertFile, httpsKeyFile, assetDir, model, + svc := newAPIService(protocol.LocalDeviceID, cfg, httpsCertFile, httpsKeyFile, assetDir, model, eventSub, discoverer, connections, errorLog, systemLog) - if err != nil { - return "", err - } - - // Make sure the API service is listening, and get the URL to use. - addr := svc.listener.Addr() - if addr == nil { - return "", fmt.Errorf("Nil listening address from API service") - } - tcpAddr, err := net.ResolveTCPAddr("tcp", addr.String()) - if err != nil { - return "", fmt.Errorf("Weird address from API service: %v", err) - } - baseURL := fmt.Sprintf("http://127.0.0.1:%d", tcpAddr.Port) + svc.started = addrChan // Actually start the API service supervisor := suture.NewSimple("API test") supervisor.Add(svc) supervisor.ServeBackground() + // Make sure the API service is listening, and get the URL to use. + addr := <-addrChan + tcpAddr, err := net.ResolveTCPAddr("tcp", addr) + if err != nil { + return "", fmt.Errorf("Weird address from API service: %v", err) + } + baseURL := fmt.Sprintf("http://127.0.0.1:%d", tcpAddr.Port) + return baseURL, nil } diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index c5d89858..40152558 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -933,10 +933,7 @@ func setupGUI(mainService *suture.Supervisor, cfg *config.Wrapper, m *model.Mode l.Warnln("Insecure admin access is enabled.") } - api, err := newAPIService(myID, cfg, locations[locHTTPSCertFile], locations[locHTTPSKeyFile], runtimeOptions.assetDir, m, apiSub, discoverer, connectionsService, errors, systemLog) - if err != nil { - l.Fatalln("Cannot start GUI:", err) - } + api := newAPIService(myID, cfg, locations[locHTTPSCertFile], locations[locHTTPSKeyFile], runtimeOptions.assetDir, m, apiSub, discoverer, connectionsService, errors, systemLog) cfg.Subscribe(api) mainService.Add(api)