From 9d3f3847eddc2352024546cb428b377d58933368 Mon Sep 17 00:00:00 2001 From: Audrius Butkevicius Date: Tue, 3 Oct 2017 23:53:02 +0100 Subject: [PATCH] lib/model: Fix removal of paused folders, improve tests (fixes #4405) --- lib/config/wrapper.go | 39 ++++++++++++++++++++-------- lib/model/model.go | 20 ++++----------- lib/model/model_test.go | 57 ++++++++++++++++++++++++++++------------- 3 files changed, 73 insertions(+), 43 deletions(-) diff --git a/lib/config/wrapper.go b/lib/config/wrapper.go index 234da75a..7f757f39 100644 --- a/lib/config/wrapper.go +++ b/lib/config/wrapper.go @@ -128,15 +128,26 @@ func (w *Wrapper) RawCopy() Configuration { return w.cfg.Copy() } +// ReplaceBlocking swaps the current configuration object for the given one, +// and waits for subscribers to be notified. +func (w *Wrapper) ReplaceBlocking(cfg Configuration) error { + w.mut.Lock() + wg := sync.NewWaitGroup() + err := w.replaceLocked(cfg, wg) + w.mut.Unlock() + wg.Wait() + return err +} + // Replace swaps the current configuration object for the given one. func (w *Wrapper) Replace(cfg Configuration) error { w.mut.Lock() defer w.mut.Unlock() - return w.replaceLocked(cfg) + return w.replaceLocked(cfg, nil) } -func (w *Wrapper) replaceLocked(to Configuration) error { +func (w *Wrapper) replaceLocked(to Configuration, wg sync.WaitGroup) error { from := w.cfg if err := to.clean(); err != nil { @@ -155,14 +166,22 @@ func (w *Wrapper) replaceLocked(to Configuration) error { w.deviceMap = nil w.folderMap = nil - w.notifyListeners(from, to) + w.notifyListeners(from, to, wg) return nil } -func (w *Wrapper) notifyListeners(from, to Configuration) { +func (w *Wrapper) notifyListeners(from, to Configuration, wg sync.WaitGroup) { + if wg != nil { + wg.Add(len(w.subs)) + } for _, sub := range w.subs { - go w.notifyListener(sub, from.Copy(), to.Copy()) + go func(commiter Committer) { + w.notifyListener(commiter, from.Copy(), to.Copy()) + if wg != nil { + wg.Done() + } + }(sub) } } @@ -210,7 +229,7 @@ func (w *Wrapper) SetDevices(devs []DeviceConfiguration) error { } } - return w.replaceLocked(newCfg) + return w.replaceLocked(newCfg, nil) } // SetDevice adds a new device to the configuration, or overwrites an existing @@ -237,7 +256,7 @@ func (w *Wrapper) RemoveDevice(id protocol.DeviceID) error { return nil } - return w.replaceLocked(newCfg) + return w.replaceLocked(newCfg, nil) } // Folders returns a map of folders. Folder structures should not be changed, @@ -273,7 +292,7 @@ func (w *Wrapper) SetFolder(fld FolderConfiguration) error { newCfg.Folders = append(w.cfg.Folders, fld) } - return w.replaceLocked(newCfg) + return w.replaceLocked(newCfg, nil) } // Options returns the current options configuration object. @@ -289,7 +308,7 @@ func (w *Wrapper) SetOptions(opts OptionsConfiguration) error { defer w.mut.Unlock() newCfg := w.cfg.Copy() newCfg.Options = opts - return w.replaceLocked(newCfg) + return w.replaceLocked(newCfg, nil) } // GUI returns the current GUI configuration object. @@ -305,7 +324,7 @@ func (w *Wrapper) SetGUI(gui GUIConfiguration) error { defer w.mut.Unlock() newCfg := w.cfg.Copy() newCfg.GUI = gui - return w.replaceLocked(newCfg) + return w.replaceLocked(newCfg, nil) } // IgnoredDevice returns whether or not connection attempts from the given diff --git a/lib/model/model.go b/lib/model/model.go index a3c01d5a..e362479a 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -335,25 +335,15 @@ func (m *Model) addFolderLocked(cfg config.FolderConfiguration) { m.folderIgnores[cfg.ID] = ignores } -func (m *Model) RemoveFolder(folder string) { +func (m *Model) RemoveFolder(cfg config.FolderConfiguration) { m.fmut.Lock() m.pmut.Lock() - // Delete syncthing specific files - folderCfg, ok := m.folderCfgs[folder] - if !ok { - // Folder might be paused - folderCfg, ok = m.cfg.Folder(folder) - } - if !ok { - panic("bug: remove non existing folder") - } - fs := folderCfg.Filesystem() - fs.RemoveAll(".stfolder") + cfg.Filesystem().RemoveAll(".stfolder") - m.tearDownFolderLocked(folder) + m.tearDownFolderLocked(cfg.ID) // Remove it from the database - db.DropFolder(m.db, folder) + db.DropFolder(m.db, cfg.ID) m.pmut.Unlock() m.fmut.Unlock() @@ -2496,7 +2486,7 @@ func (m *Model) CommitConfiguration(from, to config.Configuration) bool { toCfg, ok := toFolders[folderID] if !ok { // The folder was removed. - m.RemoveFolder(folderID) + m.RemoveFolder(fromCfg) continue } diff --git a/lib/model/model_test.go b/lib/model/model_test.go index c54e211e..89ad49f4 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -1907,48 +1907,69 @@ func TestIssue3164(t *testing.T) { func TestIssue4357(t *testing.T) { db := db.OpenMemory() - m := NewModel(defaultConfig, protocol.LocalDeviceID, "syncthing", "dev", db, nil) + cfg := defaultConfig.RawCopy() + // Create a separate wrapper not to polute other tests. + wrapper := config.Wrap("/tmp/test", config.Configuration{}) + m := NewModel(wrapper, protocol.LocalDeviceID, "syncthing", "dev", db, nil) m.ServeBackground() defer m.Stop() - cfg := m.cfg.RawCopy() - m.CommitConfiguration(config.Configuration{}, cfg) + + // Replace kicks off CommitConfiguration in multiple routines which we + // have no idea when they get called, so sleep a bit :( + // We could make replace blocking for all routines to come back without + // holding the lock, but that's a bit much just to make a test. + replaceCfg := func(cfg config.Configuration) { + t.Helper() + if err := wrapper.ReplaceBlocking(cfg); err != nil { + t.Error(err) + } + } + + // Force the model to wire itself and add the folders + replaceCfg(cfg) if _, ok := m.folderCfgs["default"]; !ok { t.Error("Folder should be running") } - newCfg := m.cfg.RawCopy() + newCfg := wrapper.RawCopy() newCfg.Folders[0].Paused = true - m.CommitConfiguration(cfg, newCfg) + + replaceCfg(newCfg) if _, ok := m.folderCfgs["default"]; ok { t.Error("Folder should not be running") } - // Should not panic when removing a paused folder. - m.RemoveFolder("default") + if _, ok := m.cfg.Folder("default"); !ok { + t.Error("should still have folder in config") + } + + replaceCfg(config.Configuration{}) + + if _, ok := m.cfg.Folder("default"); ok { + t.Error("should not have folder in config") + } // Add the folder back, should be running - m.CommitConfiguration(config.Configuration{}, cfg) + replaceCfg(cfg) if _, ok := m.folderCfgs["default"]; !ok { t.Error("Folder should be running") } + if _, ok := m.cfg.Folder("default"); !ok { + t.Error("should still have folder in config") + } // Should not panic when removing a running folder. - m.RemoveFolder("default") + replaceCfg(config.Configuration{}) + if _, ok := m.folderCfgs["default"]; ok { t.Error("Folder should not be running") } - - // Should panic when removing a non-existing folder - defer func() { - if recover() == nil { - t.Error("expected a panic") - } - }() - m.RemoveFolder("non-existing") - + if _, ok := m.cfg.Folder("default"); ok { + t.Error("should not have folder in config") + } } func TestScanNoDatabaseWrite(t *testing.T) {