From 76ad925842c108845eda8338e647e484fcdb81f6 Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Wed, 3 Jun 2015 09:47:39 +0200 Subject: [PATCH] Refactor config commit stuff to support restartless updates better Includes restartless updates of the GUI settings (listening port etc) as a proof of concept. --- cmd/syncthing/connections.go | 21 ++++ cmd/syncthing/debug.go | 5 +- cmd/syncthing/gui.go | 111 +++++++++++++++------ cmd/syncthing/main.go | 11 +- cmd/syncthing/usage_report.go | 20 +++- internal/config/commit_test.go | 83 ++++++++++++++++ internal/config/config.go | 3 - internal/config/debug.go | 19 ++++ internal/config/wrapper.go | 160 +++++++++++++++++++----------- internal/db/blockmap.go | 24 +++-- internal/model/model.go | 32 ++++++ internal/model/model_test.go | 15 +-- internal/model/progressemitter.go | 24 +++-- 13 files changed, 412 insertions(+), 116 deletions(-) create mode 100644 internal/config/commit_test.go create mode 100644 internal/config/debug.go diff --git a/cmd/syncthing/connections.go b/cmd/syncthing/connections.go index 4508455a9..7a376c296 100644 --- a/cmd/syncthing/connections.go +++ b/cmd/syncthing/connections.go @@ -353,3 +353,24 @@ func (s *connectionSvc) shouldLimit(addr net.Addr) bool { } return !tcpaddr.IP.IsLoopback() } + +func (s *connectionSvc) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +func (s *connectionSvc) CommitConfiguration(from, to config.Configuration) bool { + // We require a restart if a device as been removed. + + newDevices := make(map[protocol.DeviceID]bool, len(to.Devices)) + for _, dev := range to.Devices { + newDevices[dev.DeviceID] = true + } + + for _, dev := range from.Devices { + if !newDevices[dev.DeviceID] { + return false + } + } + + return true +} diff --git a/cmd/syncthing/debug.go b/cmd/syncthing/debug.go index 36e297aa9..51530e8a7 100644 --- a/cmd/syncthing/debug.go +++ b/cmd/syncthing/debug.go @@ -12,6 +12,7 @@ import ( ) var ( - debugNet = strings.Contains(os.Getenv("STTRACE"), "net") || os.Getenv("STTRACE") == "all" - debugHTTP = strings.Contains(os.Getenv("STTRACE"), "http") || os.Getenv("STTRACE") == "all" + debugNet = strings.Contains(os.Getenv("STTRACE"), "net") || os.Getenv("STTRACE") == "all" + debugHTTP = strings.Contains(os.Getenv("STTRACE"), "http") || os.Getenv("STTRACE") == "all" + debugSuture = strings.Contains(os.Getenv("STTRACE"), "suture") || os.Getenv("STTRACE") == "all" ) diff --git a/cmd/syncthing/gui.go b/cmd/syncthing/gui.go index 20dfcf693..a4f70c002 100644 --- a/cmd/syncthing/gui.go +++ b/cmd/syncthing/gui.go @@ -53,27 +53,29 @@ var ( ) type apiSvc struct { - cfg config.GUIConfiguration - assetDir string - model *model.Model - fss *folderSummarySvc - listener net.Listener + cfg config.GUIConfiguration + assetDir string + model *model.Model + listener net.Listener + fss *folderSummarySvc + stop chan struct{} + systemConfigMut sync.Mutex } func newAPISvc(cfg config.GUIConfiguration, assetDir string, m *model.Model) (*apiSvc, error) { svc := &apiSvc{ - cfg: cfg, - assetDir: assetDir, - model: m, - fss: newFolderSummarySvc(m), + cfg: cfg, + assetDir: assetDir, + model: m, + systemConfigMut: sync.NewMutex(), } var err error - svc.listener, err = svc.getListener() + svc.listener, err = svc.getListener(cfg) return svc, err } -func (s *apiSvc) getListener() (net.Listener, error) { +func (s *apiSvc) getListener(cfg config.GUIConfiguration) (net.Listener, error) { cert, err := tls.LoadX509KeyPair(locations[locHTTPSCertFile], locations[locHTTPSKeyFile]) if err != nil { l.Infoln("Loading HTTPS certificate:", err) @@ -110,7 +112,7 @@ func (s *apiSvc) getListener() (net.Listener, error) { }, } - rawListener, err := net.Listen("tcp", s.cfg.Address) + rawListener, err := net.Listen("tcp", cfg.Address) if err != nil { return nil, err } @@ -120,6 +122,8 @@ func (s *apiSvc) getListener() (net.Listener, error) { } func (s *apiSvc) Serve() { + s.stop = make(chan struct{}) + l.AddHandler(logger.LevelWarn, s.showGuiError) sub := events.Default.Subscribe(events.AllEvents) eventSub = events.NewBufferedSubscription(sub, 1000) @@ -210,15 +214,63 @@ func (s *apiSvc) Serve() { ReadTimeout: 10 * time.Second, } + s.fss = newFolderSummarySvc(s.model) + defer s.fss.Stop() s.fss.ServeBackground() + l.Infoln("API listening on", s.listener.Addr()) err := srv.Serve(s.listener) - l.Warnln("API:", err) + + // 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 <-time.After(time.Second): + l.Warnln("API:", err) + } } func (s *apiSvc) Stop() { + close(s.stop) s.listener.Close() - s.fss.Stop() +} + +func (s *apiSvc) String() string { + return fmt.Sprintf("apiSvc@%p", s) +} + +func (s *apiSvc) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +func (s *apiSvc) CommitConfiguration(from, to config.Configuration) bool { + if to.GUI == from.GUI { + 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 stop channel after the listener has closed. + + 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 + } + s.cfg = to.GUI + + close(s.stop) + + return true } func getPostHandler(get, post http.Handler) http.Handler { @@ -464,43 +516,46 @@ func (s *apiSvc) getSystemConfig(w http.ResponseWriter, r *http.Request) { } func (s *apiSvc) postSystemConfig(w http.ResponseWriter, r *http.Request) { - var newCfg config.Configuration - err := json.NewDecoder(r.Body).Decode(&newCfg) + s.systemConfigMut.Lock() + defer s.systemConfigMut.Unlock() + + var to config.Configuration + err := json.NewDecoder(r.Body).Decode(&to) if err != nil { l.Warnln("decoding posted config:", err) http.Error(w, err.Error(), 500) return } - if newCfg.GUI.Password != cfg.GUI().Password { - if newCfg.GUI.Password != "" { - hash, err := bcrypt.GenerateFromPassword([]byte(newCfg.GUI.Password), 0) + if to.GUI.Password != cfg.GUI().Password { + if to.GUI.Password != "" { + hash, err := bcrypt.GenerateFromPassword([]byte(to.GUI.Password), 0) if err != nil { l.Warnln("bcrypting password:", err) http.Error(w, err.Error(), 500) return } - newCfg.GUI.Password = string(hash) + to.GUI.Password = string(hash) } } // Fixup usage reporting settings - if curAcc := cfg.Options().URAccepted; newCfg.Options.URAccepted > curAcc { + if curAcc := cfg.Options().URAccepted; to.Options.URAccepted > curAcc { // UR was enabled - newCfg.Options.URAccepted = usageReportVersion - newCfg.Options.URUniqueID = randomString(8) - } else if newCfg.Options.URAccepted < curAcc { + to.Options.URAccepted = usageReportVersion + to.Options.URUniqueID = randomString(8) + } else if to.Options.URAccepted < curAcc { // UR was disabled - newCfg.Options.URAccepted = -1 - newCfg.Options.URUniqueID = "" + to.Options.URAccepted = -1 + to.Options.URUniqueID = "" } // Activate and save - configInSync = !config.ChangeRequiresRestart(cfg.Raw(), newCfg) - cfg.Replace(newCfg) + resp := cfg.Replace(to) + configInSync = !resp.RequiresRestart cfg.Save() } diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 9f2685830..6ceb60f19 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -155,6 +155,7 @@ are mostly useful for developers. Use with care. - "model" (the model package) - "scanner" (the scanner package) - "stats" (the stats package) + - "suture" (the suture package; service management) - "upnp" (the upnp package) - "xdr" (the xdr package) - "all" (all of the above) @@ -420,11 +421,12 @@ func upgradeViaRest() error { func syncthingMain() { // Create a main service manager. We'll add things to this as we go along. - // We want any logging it does to go through our log system, with INFO - // severity. + // We want any logging it does to go through our log system. mainSvc := suture.New("main", suture.Spec{ Log: func(line string) { - l.Infoln(line) + if debugSuture { + l.Debugln(line) + } }, }) mainSvc.ServeBackground() @@ -586,6 +588,7 @@ func syncthingMain() { } m := model.NewModel(cfg, myID, myName, "syncthing", Version, ldb) + cfg.Subscribe(m) if t := os.Getenv("STDEADLOCKTIMEOUT"); len(t) > 0 { it, err := strconv.Atoi(t) @@ -643,6 +646,7 @@ func syncthingMain() { } connectionSvc := newConnectionSvc(cfg, myID, m, tlsCfg) + cfg.Subscribe(connectionSvc) mainSvc.Add(connectionSvc) if cpuProfile { @@ -792,6 +796,7 @@ func setupGUI(mainSvc *suture.Supervisor, cfg *config.Wrapper, m *model.Model) { if err != nil { l.Fatalln("Cannot start GUI:", err) } + cfg.Subscribe(api) mainSvc.Add(api) if opts.StartBrowser && !noBrowser && !stRestarting { diff --git a/cmd/syncthing/usage_report.go b/cmd/syncthing/usage_report.go index b033ff6af..87f8d7ba8 100644 --- a/cmd/syncthing/usage_report.go +++ b/cmd/syncthing/usage_report.go @@ -11,6 +11,7 @@ import ( "crypto/rand" "crypto/sha256" "encoding/json" + "fmt" "net" "net/http" "runtime" @@ -37,7 +38,7 @@ func newUsageReportingManager(m *model.Model, cfg *config.Wrapper) *usageReporti } // Start UR if it's enabled. - mgr.Changed(cfg.Raw()) + mgr.CommitConfiguration(config.Configuration{}, cfg.Raw()) // Listen to future config changes so that we can start and stop as // appropriate. @@ -46,8 +47,12 @@ func newUsageReportingManager(m *model.Model, cfg *config.Wrapper) *usageReporti return mgr } -func (m *usageReportingManager) Changed(cfg config.Configuration) error { - if cfg.Options.URAccepted >= usageReportVersion && m.sup == nil { +func (m *usageReportingManager) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +func (m *usageReportingManager) CommitConfiguration(from, to config.Configuration) bool { + if to.Options.URAccepted >= usageReportVersion && m.sup == nil { // Usage reporting was turned on; lets start it. svc := &usageReportingService{ model: m.model, @@ -55,12 +60,17 @@ func (m *usageReportingManager) Changed(cfg config.Configuration) error { m.sup = suture.NewSimple("usageReporting") m.sup.Add(svc) m.sup.ServeBackground() - } else if cfg.Options.URAccepted < usageReportVersion && m.sup != nil { + } else if to.Options.URAccepted < usageReportVersion && m.sup != nil { // Usage reporting was turned off m.sup.Stop() m.sup = nil } - return nil + + return true +} + +func (m *usageReportingManager) String() string { + return fmt.Sprintf("usageReportingManager@%p", m) } // reportData returns the data to be sent in a usage report. It's used in diff --git a/internal/config/commit_test.go b/internal/config/commit_test.go new file mode 100644 index 000000000..69965aa9f --- /dev/null +++ b/internal/config/commit_test.go @@ -0,0 +1,83 @@ +package config + +import ( + "errors" + "testing" +) + +type requiresRestart struct{} + +func (requiresRestart) VerifyConfiguration(_, _ Configuration) error { + return nil +} +func (requiresRestart) CommitConfiguration(_, _ Configuration) bool { + return false +} +func (requiresRestart) String() string { + return "requiresRestart" +} + +type validationError struct{} + +func (validationError) VerifyConfiguration(_, _ Configuration) error { + return errors.New("some error") +} +func (validationError) CommitConfiguration(_, _ Configuration) bool { + return true +} +func (validationError) String() string { + return "validationError" +} + +func TestReplaceCommit(t *testing.T) { + w := Wrap("/dev/null", Configuration{Version: 0}) + if w.Raw().Version != 0 { + t.Fatal("Config incorrect") + } + + // 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") + } + if resp.RequiresRestart { + t.Fatal("Should not require restart") + } + if w.Raw().Version != 1 { + t.Fatal("Config should have changed") + } + + // 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{}) + + resp = w.Replace(Configuration{Version: 2}) + if resp.ValidationError != nil { + t.Fatal("Should not have a validation error") + } + if !resp.RequiresRestart { + t.Fatal("Should require restart") + } + if w.Raw().Version != 2 { + t.Fatal("Config should have changed") + } + + // Now with a subscriber that throws a validation error. The config should + // not change. + + w.Subscribe(validationError{}) + + resp = w.Replace(Configuration{Version: 3}) + if resp.ValidationError == nil { + t.Fatal("Should have a validation error") + } + if resp.RequiresRestart { + t.Fatal("Should not require restart") + } + if w.Raw().Version != 2 { + t.Fatal("Config should not have changed") + } +} diff --git a/internal/config/config.go b/internal/config/config.go index dd8d976f4..025754e2f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,14 +20,11 @@ import ( "strconv" "strings" - "github.com/calmh/logger" "github.com/syncthing/protocol" "github.com/syncthing/syncthing/internal/osutil" "golang.org/x/crypto/bcrypt" ) -var l = logger.DefaultLogger - const ( OldestHandledVersion = 5 CurrentVersion = 10 diff --git a/internal/config/debug.go b/internal/config/debug.go new file mode 100644 index 000000000..fcff21183 --- /dev/null +++ b/internal/config/debug.go @@ -0,0 +1,19 @@ +// Copyright (C) 2015 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +package config + +import ( + "os" + "strings" + + "github.com/calmh/logger" +) + +var ( + debug = strings.Contains(os.Getenv("STTRACE"), "config") || os.Getenv("STTRACE") == "all" + l = logger.DefaultLogger +) diff --git a/internal/config/wrapper.go b/internal/config/wrapper.go index ba7dfce20..64106383c 100644 --- a/internal/config/wrapper.go +++ b/internal/config/wrapper.go @@ -17,17 +17,39 @@ import ( "github.com/syncthing/syncthing/internal/sync" ) -// An interface to handle configuration changes, and a wrapper type รก la -// http.Handler - -type Handler interface { - Changed(Configuration) error +// The Committer interface is implemented by objects that need to know about +// or have a say in configuration changes. +// +// When the configuration is about to be changed, VerifyConfiguration() is +// called for each subscribing object, with the old and new configuration. A +// nil error is returned if the new configuration is acceptable (i.e. does not +// contain any errors that would prevent it from being a valid config). +// Otherwise an error describing the problem is returned. +// +// If any subscriber returns an error from VerifyConfiguration(), the +// configuration change is not committed and an error is returned to whoever +// tried to commit the broken config. +// +// If all verification calls returns nil, CommitConfiguration() is called for +// each subscribing object. The callee returns true if the new configuration +// has been successfully applied, otherwise false. Any Commit() call returning +// false will result in a "restart needed" respone to the API/user. Note that +// the new configuration will still have been applied by those who were +// capable of doing so. +type Committer interface { + VerifyConfiguration(from, to Configuration) error + CommitConfiguration(from, to Configuration) (handled bool) + String() string } -type HandlerFunc func(Configuration) error +type CommitResponse struct { + ValidationError error + RequiresRestart bool +} -func (fn HandlerFunc) Changed(cfg Configuration) error { - return fn(cfg) +var ResponseNoRestart = CommitResponse{ + ValidationError: nil, + RequiresRestart: false, } // A wrapper around a Configuration that manages loads, saves and published @@ -42,7 +64,7 @@ type Wrapper struct { replaces chan Configuration mut sync.Mutex - subs []Handler + subs []Committer sMut sync.Mutex } @@ -56,7 +78,6 @@ func Wrap(path string, cfg Configuration) *Wrapper { sMut: sync.NewMutex(), } w.replaces = make(chan Configuration) - go w.Serve() return w } @@ -77,21 +98,6 @@ func Load(path string, myID protocol.DeviceID) (*Wrapper, error) { return Wrap(path, cfg), nil } -// Serve handles configuration replace events and calls any interested -// handlers. It is started automatically by Wrap() and Load() and should not -// be run manually. -func (w *Wrapper) Serve() { - for cfg := range w.replaces { - w.sMut.Lock() - subs := w.subs - w.sMut.Unlock() - - for _, h := range subs { - h.Changed(cfg) - } - } -} - // Stop stops the Serve() loop. Set and Replace operations will panic after a // Stop. func (w *Wrapper) Stop() { @@ -100,9 +106,9 @@ func (w *Wrapper) Stop() { // Subscribe registers the given handler to be called on any future // configuration changes. -func (w *Wrapper) Subscribe(h Handler) { +func (w *Wrapper) Subscribe(c Committer) { w.sMut.Lock() - w.subs = append(w.subs, h) + w.subs = append(w.subs, c) w.sMut.Unlock() } @@ -112,14 +118,50 @@ func (w *Wrapper) Raw() Configuration { } // Replace swaps the current configuration object for the given one. -func (w *Wrapper) Replace(cfg Configuration) { +func (w *Wrapper) Replace(cfg Configuration) CommitResponse { w.mut.Lock() defer w.mut.Unlock() + return w.replaceLocked(cfg) +} - w.cfg = cfg +func (w *Wrapper) replaceLocked(to Configuration) CommitResponse { + from := w.cfg + + for _, sub := range w.subs { + if debug { + l.Debugln(sub, "verifying configuration") + } + if err := sub.VerifyConfiguration(from, to); err != nil { + if debug { + l.Debugln(sub, "rejected config:", err) + } + return CommitResponse{ + ValidationError: err, + } + } + } + + allOk := true + for _, sub := range w.subs { + if debug { + l.Debugln(sub, "committing configuration") + } + ok := sub.CommitConfiguration(from, to) + if !ok { + if debug { + l.Debugln(sub, "requires restart") + } + allOk = false + } + } + + w.cfg = to w.deviceMap = nil w.folderMap = nil - w.replaces <- cfg.Copy() + + return CommitResponse{ + RequiresRestart: !allOk, + } } // Devices returns a map of devices. Device structures should not be changed, @@ -138,22 +180,24 @@ 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) { +func (w *Wrapper) SetDevice(dev DeviceConfiguration) CommitResponse { w.mut.Lock() defer w.mut.Unlock() - w.deviceMap = nil - - for i := range w.cfg.Devices { - if w.cfg.Devices[i].DeviceID == dev.DeviceID { - w.cfg.Devices[i] = dev - w.replaces <- w.cfg.Copy() - return + newCfg := w.cfg.Copy() + replaced := false + for i := range newCfg.Devices { + if newCfg.Devices[i].DeviceID == dev.DeviceID { + newCfg.Devices[i] = dev + replaced = true + break } } + if !replaced { + newCfg.Devices = append(w.cfg.Devices, dev) + } - w.cfg.Devices = append(w.cfg.Devices, dev) - w.replaces <- w.cfg.Copy() + return w.replaceLocked(newCfg) } // Folders returns a map of folders. Folder structures should not be changed, @@ -172,22 +216,24 @@ 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) { +func (w *Wrapper) SetFolder(fld FolderConfiguration) CommitResponse { w.mut.Lock() defer w.mut.Unlock() - w.folderMap = nil - - for i := range w.cfg.Folders { - if w.cfg.Folders[i].ID == fld.ID { - w.cfg.Folders[i] = fld - w.replaces <- w.cfg.Copy() - return + newCfg := w.cfg.Copy() + replaced := false + for i := range newCfg.Folders { + if newCfg.Folders[i].ID == fld.ID { + newCfg.Folders[i] = fld + replaced = true + break } } + if !replaced { + newCfg.Folders = append(w.cfg.Folders, fld) + } - w.cfg.Folders = append(w.cfg.Folders, fld) - w.replaces <- w.cfg.Copy() + return w.replaceLocked(newCfg) } // Options returns the current options configuration object. @@ -198,11 +244,12 @@ func (w *Wrapper) Options() OptionsConfiguration { } // SetOptions replaces the current options configuration object. -func (w *Wrapper) SetOptions(opts OptionsConfiguration) { +func (w *Wrapper) SetOptions(opts OptionsConfiguration) CommitResponse { w.mut.Lock() defer w.mut.Unlock() - w.cfg.Options = opts - w.replaces <- w.cfg.Copy() + newCfg := w.cfg.Copy() + newCfg.Options = opts + return w.replaceLocked(newCfg) } // GUI returns the current GUI configuration object. @@ -213,11 +260,12 @@ func (w *Wrapper) GUI() GUIConfiguration { } // SetGUI replaces the current GUI configuration object. -func (w *Wrapper) SetGUI(gui GUIConfiguration) { +func (w *Wrapper) SetGUI(gui GUIConfiguration) CommitResponse { w.mut.Lock() defer w.mut.Unlock() - w.cfg.GUI = gui - w.replaces <- w.cfg.Copy() + newCfg := w.cfg.Copy() + newCfg.GUI = gui + return w.replaceLocked(newCfg) } // IgnoredDevice returns whether or not connection attempts from the given diff --git a/internal/db/blockmap.go b/internal/db/blockmap.go index 496e971be..4846facad 100644 --- a/internal/db/blockmap.go +++ b/internal/db/blockmap.go @@ -15,6 +15,7 @@ package db import ( "bytes" "encoding/binary" + "fmt" "sort" "github.com/syncthing/protocol" @@ -125,15 +126,22 @@ func NewBlockFinder(db *leveldb.DB, cfg *config.Wrapper) *BlockFinder { db: db, mut: sync.NewRWMutex(), } - f.Changed(cfg.Raw()) + + f.CommitConfiguration(config.Configuration{}, cfg.Raw()) cfg.Subscribe(f) + return f } -// Changed implements config.Handler interface -func (f *BlockFinder) Changed(cfg config.Configuration) error { - folders := make([]string, len(cfg.Folders)) - for i, folder := range cfg.Folders { +// VerifyConfiguration implementes the config.Committer interface +func (f *BlockFinder) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +// CommitConfiguration implementes the config.Committer interface +func (f *BlockFinder) CommitConfiguration(from, to config.Configuration) bool { + folders := make([]string, len(to.Folders)) + for i, folder := range to.Folders { folders[i] = folder.ID } @@ -143,7 +151,11 @@ func (f *BlockFinder) Changed(cfg config.Configuration) error { f.folders = folders f.mut.Unlock() - return nil + return true +} + +func (f *BlockFinder) String() string { + return fmt.Sprintf("BlockFinder@%p", f) } // Iterate takes an iterator function which iterates over all matching blocks diff --git a/internal/model/model.go b/internal/model/model.go index 6a9a82dce..2e1840632 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -17,6 +17,7 @@ import ( "net" "os" "path/filepath" + "reflect" "runtime" "strings" stdsync "sync" @@ -1718,6 +1719,37 @@ func (m *Model) String() string { return fmt.Sprintf("model@%p", m) } +func (m *Model) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +func (m *Model) CommitConfiguration(from, to config.Configuration) bool { + // TODO: This should not use reflect, and should take more care to try to handle stuff without restart. + + // Adding, removing or changing folders requires restart + if !reflect.DeepEqual(from.Folders, to.Folders) { + return false + } + + // Removing a device requres restart + toDevs := make(map[protocol.DeviceID]bool, len(from.Devices)) + for _, dev := range to.Devices { + toDevs[dev.DeviceID] = true + } + for _, dev := range from.Devices { + if _, ok := toDevs[dev.DeviceID]; !ok { + return false + } + } + + // All of the generic options require restart + if !reflect.DeepEqual(from.Options, to.Options) { + return false + } + + return true +} + func symlinkInvalid(isLink bool) bool { if !symlinks.Supported && isLink { SymlinkWarning.Do(func() { diff --git a/internal/model/model_test.go b/internal/model/model_test.go index 004dce692..c7f02311a 100644 --- a/internal/model/model_test.go +++ b/internal/model/model_test.go @@ -317,21 +317,22 @@ func TestDeviceRename(t *testing.T) { defer os.Remove("tmpconfig.xml") - cfg := config.New(device1) - cfg.Devices = []config.DeviceConfiguration{ + rawCfg := config.New(device1) + rawCfg.Devices = []config.DeviceConfiguration{ { DeviceID: device1, }, } + cfg := config.Wrap("tmpconfig.xml", rawCfg) db, _ := leveldb.Open(storage.NewMemStorage(), nil) - m := NewModel(config.Wrap("tmpconfig.xml", cfg), protocol.LocalDeviceID, "device", "syncthing", "dev", db) - if cfg.Devices[0].Name != "" { + m := NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", db) + if cfg.Devices()[device1].Name != "" { t.Errorf("Device already has a name") } m.ClusterConfig(device1, ccm) - if cfg.Devices[0].Name != "" { + if cfg.Devices()[device1].Name != "" { t.Errorf("Device already has a name") } @@ -342,13 +343,13 @@ func TestDeviceRename(t *testing.T) { }, } m.ClusterConfig(device1, ccm) - if cfg.Devices[0].Name != "tester" { + if cfg.Devices()[device1].Name != "tester" { t.Errorf("Device did not get a name") } ccm.Options[0].Value = "tester2" m.ClusterConfig(device1, ccm) - if cfg.Devices[0].Name != "tester" { + if cfg.Devices()[device1].Name != "tester" { t.Errorf("Device name got overwritten") } diff --git a/internal/model/progressemitter.go b/internal/model/progressemitter.go index 397127aca..c38db315f 100755 --- a/internal/model/progressemitter.go +++ b/internal/model/progressemitter.go @@ -7,6 +7,7 @@ package model import ( + "fmt" "path/filepath" "reflect" "time" @@ -37,8 +38,10 @@ func NewProgressEmitter(cfg *config.Wrapper) *ProgressEmitter { timer: time.NewTimer(time.Millisecond), mut: sync.NewMutex(), } - t.Changed(cfg.Raw()) + + t.CommitConfiguration(config.Configuration{}, cfg.Raw()) cfg.Subscribe(t) + return t } @@ -81,17 +84,22 @@ func (t *ProgressEmitter) Serve() { } } -// Changed implements the config.Handler Interface to handle configuration -// changes -func (t *ProgressEmitter) Changed(cfg config.Configuration) error { +// VerifyConfiguration implements the config.Committer interface +func (t *ProgressEmitter) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +// CommitConfiguration implements the config.Committer interface +func (t *ProgressEmitter) CommitConfiguration(from, to config.Configuration) bool { t.mut.Lock() defer t.mut.Unlock() - t.interval = time.Duration(cfg.Options.ProgressUpdateIntervalS) * time.Second + t.interval = time.Duration(to.Options.ProgressUpdateIntervalS) * time.Second if debug { l.Debugln("progress emitter: updated interval", t.interval) } - return nil + + return true } // Stop stops the emitter. @@ -138,3 +146,7 @@ func (t *ProgressEmitter) BytesCompleted(folder string) (bytes int64) { } return } + +func (t *ProgressEmitter) String() string { + return fmt.Sprintf("ProgressEmitter@%p", t) +}