diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index 8503158db..64c1c3401 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -41,8 +41,7 @@ import ( ) var ( - configInSync = true - startTime = time.Now() + startTime = time.Now() ) type apiService struct { @@ -100,12 +99,13 @@ type configIntf interface { GUI() config.GUIConfiguration Raw() config.Configuration Options() config.OptionsConfiguration - Replace(cfg config.Configuration) config.CommitResponse + Replace(cfg config.Configuration) error Subscribe(c config.Committer) Folders() map[string]config.FolderConfiguration Devices() map[protocol.DeviceID]config.DeviceConfiguration Save() error ListenAddresses() []string + RequiresRestart() bool } type connectionsIntf interface { @@ -722,13 +722,19 @@ func (s *apiService) postSystemConfig(w http.ResponseWriter, r *http.Request) { // Activate and save - resp := s.cfg.Replace(to) - configInSync = !resp.RequiresRestart - s.cfg.Save() + if err := s.cfg.Replace(to); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + if err := s.cfg.Save(); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } } func (s *apiService) getSystemConfigInsync(w http.ResponseWriter, r *http.Request) { - sendJSON(w, map[string]bool{"configInSync": configInSync}) + sendJSON(w, map[string]bool{"configInSync": !s.cfg.RequiresRestart()}) } func (s *apiService) postSystemRestart(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/syncthing/mocked_config_test.go b/cmd/syncthing/mocked_config_test.go index d4e734a73..55686f51f 100644 --- a/cmd/syncthing/mocked_config_test.go +++ b/cmd/syncthing/mocked_config_test.go @@ -31,8 +31,8 @@ func (c *mockedConfig) Options() config.OptionsConfiguration { return config.OptionsConfiguration{} } -func (c *mockedConfig) Replace(cfg config.Configuration) config.CommitResponse { - return config.CommitResponse{} +func (c *mockedConfig) Replace(cfg config.Configuration) error { + return nil } func (c *mockedConfig) Subscribe(cm config.Committer) {} @@ -48,3 +48,7 @@ func (c *mockedConfig) Devices() map[protocol.DeviceID]config.DeviceConfiguratio func (c *mockedConfig) Save() error { return nil } + +func (c *mockedConfig) RequiresRestart() bool { + return false +} diff --git a/lib/config/commit_test.go b/lib/config/commit_test.go index 3cff5cd04..759834647 100644 --- a/lib/config/commit_test.go +++ b/lib/config/commit_test.go @@ -11,12 +11,18 @@ import ( "testing" ) -type requiresRestart struct{} +type requiresRestart struct { + committed chan struct{} +} func (requiresRestart) VerifyConfiguration(_, _ Configuration) error { return nil } -func (requiresRestart) CommitConfiguration(_, _ Configuration) bool { +func (c requiresRestart) CommitConfiguration(_, _ Configuration) bool { + select { + case c.committed <- struct{}{}: + default: + } return false } func (requiresRestart) String() string { @@ -28,7 +34,7 @@ type validationError struct{} func (validationError) VerifyConfiguration(_, _ Configuration) error { return errors.New("some error") } -func (validationError) CommitConfiguration(_, _ Configuration) bool { +func (c validationError) CommitConfiguration(_, _ Configuration) bool { return true } func (validationError) String() string { @@ -44,11 +50,11 @@ func TestReplaceCommit(t *testing.T) { // Replace config. We should get back a clean response and the config // should change. - resp := w.Replace(Configuration{Version: 1}) - if resp.ValidationError != nil { - t.Fatal("Should not have a validation error") + err := w.Replace(Configuration{Version: 1}) + if err != nil { + t.Fatal("Should not have a validation error:", err) } - if resp.RequiresRestart { + if w.RequiresRestart() { t.Fatal("Should not require restart") } if w.Raw().Version != 1 { @@ -58,13 +64,16 @@ func TestReplaceCommit(t *testing.T) { // Now with a subscriber requiring restart. We should get a clean response // but with the restart flag set, and the config should change. - w.Subscribe(requiresRestart{}) + sub0 := requiresRestart{committed: make(chan struct{}, 1)} + w.Subscribe(sub0) - resp = w.Replace(Configuration{Version: 2}) - if resp.ValidationError != nil { - t.Fatal("Should not have a validation error") + err = w.Replace(Configuration{Version: 2}) + if err != nil { + t.Fatal("Should not have a validation error:", err) } - if !resp.RequiresRestart { + + <-sub0.committed + if !w.RequiresRestart() { t.Fatal("Should require restart") } if w.Raw().Version != 2 { @@ -76,12 +85,12 @@ func TestReplaceCommit(t *testing.T) { w.Subscribe(validationError{}) - resp = w.Replace(Configuration{Version: 3}) - if resp.ValidationError == nil { + err = w.Replace(Configuration{Version: 3}) + if err == nil { t.Fatal("Should have a validation error") } - if resp.RequiresRestart { - t.Fatal("Should not require restart") + if !w.RequiresRestart() { + t.Fatal("Should still require restart") } if w.Raw().Version != 2 { t.Fatal("Config should not have changed") diff --git a/lib/config/wrapper.go b/lib/config/wrapper.go index 212a70af8..a848849cd 100644 --- a/lib/config/wrapper.go +++ b/lib/config/wrapper.go @@ -8,6 +8,7 @@ package config import ( "os" + "sync/atomic" "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/osutil" @@ -41,11 +42,6 @@ type Committer interface { String() string } -type CommitResponse struct { - ValidationError error - RequiresRestart bool -} - // A wrapper around a Configuration that manages loads, saves and published // notifications of changes to registered Handlers @@ -58,6 +54,8 @@ type Wrapper struct { replaces chan Configuration subs []Committer mut sync.Mutex + + requiresRestart uint32 // an atomic bool } // Wrap wraps an existing Configuration structure and ties it to a file on @@ -128,32 +126,21 @@ func (w *Wrapper) Raw() Configuration { } // Replace swaps the current configuration object for the given one. -func (w *Wrapper) Replace(cfg Configuration) CommitResponse { +func (w *Wrapper) Replace(cfg Configuration) error { w.mut.Lock() defer w.mut.Unlock() + return w.replaceLocked(cfg) } -func (w *Wrapper) replaceLocked(to Configuration) CommitResponse { +func (w *Wrapper) replaceLocked(to Configuration) error { from := w.cfg for _, sub := range w.subs { l.Debugln(sub, "verifying configuration") if err := sub.VerifyConfiguration(from, to); err != nil { l.Debugln(sub, "rejected config:", err) - return CommitResponse{ - ValidationError: err, - } - } - } - - allOk := true - for _, sub := range w.subs { - l.Debugln(sub, "committing configuration") - ok := sub.CommitConfiguration(from, to) - if !ok { - l.Debugln(sub, "requires restart") - allOk = false + return err } } @@ -161,8 +148,22 @@ func (w *Wrapper) replaceLocked(to Configuration) CommitResponse { w.deviceMap = nil w.folderMap = nil - return CommitResponse{ - RequiresRestart: !allOk, + w.notifyListeners(from, to) + + return nil +} + +func (w *Wrapper) notifyListeners(from, to Configuration) { + for _, sub := range w.subs { + go w.notifyListener(sub, from, to) + } +} + +func (w *Wrapper) notifyListener(sub Committer, from, to Configuration) { + l.Debugln(sub, "committing configuration") + if !sub.CommitConfiguration(from, to) { + l.Debugln(sub, "requires restart") + w.setRequiresRestart() } } @@ -182,7 +183,7 @@ func (w *Wrapper) Devices() map[protocol.DeviceID]DeviceConfiguration { // SetDevice adds a new device to the configuration, or overwrites an existing // device with the same ID. -func (w *Wrapper) SetDevice(dev DeviceConfiguration) CommitResponse { +func (w *Wrapper) SetDevice(dev DeviceConfiguration) error { w.mut.Lock() defer w.mut.Unlock() @@ -218,7 +219,7 @@ func (w *Wrapper) Folders() map[string]FolderConfiguration { // SetFolder adds a new folder to the configuration, or overwrites an existing // folder with the same ID. -func (w *Wrapper) SetFolder(fld FolderConfiguration) CommitResponse { +func (w *Wrapper) SetFolder(fld FolderConfiguration) error { w.mut.Lock() defer w.mut.Unlock() @@ -246,7 +247,7 @@ func (w *Wrapper) Options() OptionsConfiguration { } // SetOptions replaces the current options configuration object. -func (w *Wrapper) SetOptions(opts OptionsConfiguration) CommitResponse { +func (w *Wrapper) SetOptions(opts OptionsConfiguration) error { w.mut.Lock() defer w.mut.Unlock() newCfg := w.cfg.Copy() @@ -262,7 +263,7 @@ func (w *Wrapper) GUI() GUIConfiguration { } // SetGUI replaces the current GUI configuration object. -func (w *Wrapper) SetGUI(gui GUIConfiguration) CommitResponse { +func (w *Wrapper) SetGUI(gui GUIConfiguration) error { w.mut.Lock() defer w.mut.Unlock() newCfg := w.cfg.Copy() @@ -332,3 +333,11 @@ func (w *Wrapper) ListenAddresses() []string { } return util.UniqueStrings(addresses) } + +func (w *Wrapper) RequiresRestart() bool { + return atomic.LoadUint32(&w.requiresRestart) != 0 +} + +func (w *Wrapper) setRequiresRestart() { + atomic.StoreUint32(&w.requiresRestart, 1) +}