From 44d30c83bf28a2f97e655538a5f0489ac96b4740 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Mon, 4 Jul 2016 20:32:34 +0000 Subject: [PATCH] lib/config, cmd/syncthing: Handle committing configuration better (fixes #3077) This slightly changes the interface used for committing configuration changes. The two parts are now: - VerifyConfiguration, which runs synchronously and locked, and can abort the config change. These callbacks shouldn't *do* anything apart from looking at the config changes and saying yes or no. No change from previously. - CommitConfiguration, which runs asynchronously (one goroutine per call) *after* replacing the config and releasing any locks. Returning false from these methods sets the "requires restart" flag, which now lives in the config.Wrapper. This should be deadlock free as the CommitConfiguration calls can take as long as they like and can wait for locks to be released when they need to tweak things. I think this should be safe compared to before as the CommitConfiguration calls were always made from a random background goroutine (typically one from the HTTP server), so it was always concurrent with everything else anyway. Hence the CommitResponse type is gone, instead you get an error back on verification failure only, and need to explicitly check w.RequiresRestart() afterwards if you care. As an added bonus this fixes a bug where we would reset the "requires restart" indicator if a config that did not require restart was saved, even if we already were in the requires-restart state. GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3386 --- cmd/syncthing/gui.go | 20 ++++++---- cmd/syncthing/mocked_config_test.go | 8 +++- lib/config/commit_test.go | 41 +++++++++++-------- lib/config/wrapper.go | 61 +++++++++++++++++------------ 4 files changed, 79 insertions(+), 51 deletions(-) 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) +}