diff --git a/cmd/strelaysrv/main.go b/cmd/strelaysrv/main.go index b568d0817..0d914e23f 100644 --- a/cmd/strelaysrv/main.go +++ b/cmd/strelaysrv/main.go @@ -186,10 +186,10 @@ func main() { } wrapper := config.Wrap("config", config.New(id), id, events.NoopLogger) - wrapper.SetOptions(config.OptionsConfiguration{ - NATLeaseM: natLease, - NATRenewalM: natRenewal, - NATTimeoutS: natTimeout, + wrapper.Modify(func(cfg *config.Configuration) { + cfg.Options.NATLeaseM = natLease + cfg.Options.NATRenewalM = natRenewal + cfg.Options.NATTimeoutS = natTimeout }) natSvc := nat.NewService(id, wrapper) mapping := mapping{natSvc.NewMapping(nat.TCP, addr.IP, addr.Port)} diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 385b29d29..82fb26796 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -607,7 +607,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) { go evLogger.Serve(ctx) defer cancel() - cfg, err := syncthing.LoadConfigAtStartup(locations.Get(locations.ConfigFile), cert, evLogger, runtimeOptions.allowNewerConfig, noDefaultFolder) + cfgWrapper, err := syncthing.LoadConfigAtStartup(locations.Get(locations.ConfigFile), cert, evLogger, runtimeOptions.allowNewerConfig, noDefaultFolder) if err != nil { l.Warnln("Failed to initialize config:", err) os.Exit(svcutil.ExitError.AsInt()) @@ -618,20 +618,20 @@ func syncthingMain(runtimeOptions RuntimeOptions) { // environment variable is set. if build.IsCandidate && !upgrade.DisabledByCompilation && !runtimeOptions.NoUpgrade { - l.Infoln("Automatic upgrade is always enabled for candidate releases.") - if opts := cfg.Options(); opts.AutoUpgradeIntervalH == 0 || opts.AutoUpgradeIntervalH > 24 { - opts.AutoUpgradeIntervalH = 12 - // Set the option into the config as well, as the auto upgrade - // loop expects to read a valid interval from there. - cfg.SetOptions(opts) - cfg.Save() - } - // We don't tweak the user's choice of upgrading to pre-releases or - // not, as otherwise they cannot step off the candidate channel. + cfgWrapper.Modify(func(cfg *config.Configuration) { + l.Infoln("Automatic upgrade is always enabled for candidate releases.") + if cfg.Options.AutoUpgradeIntervalH == 0 || cfg.Options.AutoUpgradeIntervalH > 24 { + cfg.Options.AutoUpgradeIntervalH = 12 + // Set the option into the config as well, as the auto upgrade + // loop expects to read a valid interval from there. + } + // We don't tweak the user's choice of upgrading to pre-releases or + // not, as otherwise they cannot step off the candidate channel. + }) } dbFile := locations.Get(locations.Database) - ldb, err := syncthing.OpenDBBackend(dbFile, cfg.Options().DatabaseTuning) + ldb, err := syncthing.OpenDBBackend(dbFile, cfgWrapper.Options().DatabaseTuning) if err != nil { l.Warnln("Error opening database:", err) os.Exit(1) @@ -642,7 +642,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) { // later after App is initialised. autoUpgradePossible := autoUpgradePossible(runtimeOptions) - if autoUpgradePossible && cfg.Options().AutoUpgradeEnabled() { + if autoUpgradePossible && cfgWrapper.Options().AutoUpgradeEnabled() { // try to do upgrade directly and log the error if relevant. release, err := initialAutoUpgradeCheck(db.NewMiscDataNamespace(ldb)) if err == nil { @@ -661,9 +661,9 @@ func syncthingMain(runtimeOptions RuntimeOptions) { } if runtimeOptions.unpaused { - setPauseState(cfg, false) + setPauseState(cfgWrapper, false) } else if runtimeOptions.paused { - setPauseState(cfg, true) + setPauseState(cfgWrapper, true) } appOpts := runtimeOptions.Options @@ -681,14 +681,14 @@ func syncthingMain(runtimeOptions RuntimeOptions) { appOpts.DBIndirectGCInterval = dur } - app, err := syncthing.New(cfg, ldb, evLogger, cert, appOpts) + app, err := syncthing.New(cfgWrapper, ldb, evLogger, cert, appOpts) if err != nil { l.Warnln("Failed to start Syncthing:", err) os.Exit(svcutil.ExitError.AsInt()) } if autoUpgradePossible { - go autoUpgrade(cfg, app, evLogger) + go autoUpgrade(cfgWrapper, app, evLogger) } setupSignalHandling(app) @@ -709,7 +709,7 @@ func syncthingMain(runtimeOptions RuntimeOptions) { } } - go standbyMonitor(app, cfg) + go standbyMonitor(app, cfgWrapper) if err := app.Start(); err != nil { os.Exit(svcutil.ExitError.AsInt()) @@ -717,10 +717,10 @@ func syncthingMain(runtimeOptions RuntimeOptions) { cleanConfigDirectory() - if cfg.Options().StartBrowser && !runtimeOptions.noBrowser && !runtimeOptions.stRestarting { + if cfgWrapper.Options().StartBrowser && !runtimeOptions.noBrowser && !runtimeOptions.stRestarting { // Can potentially block if the utility we are invoking doesn't // fork, and just execs, hence keep it in its own routine. - go func() { _ = openURL(cfg.GUI().URL()) }() + go func() { _ = openURL(cfgWrapper.GUI().URL()) }() } status := app.Wait() @@ -988,15 +988,16 @@ func showPaths(options RuntimeOptions) { fmt.Printf("Default sync folder directory:\n\t%s\n\n", locations.Get(locations.DefFolder)) } -func setPauseState(cfg config.Wrapper, paused bool) { - raw := cfg.RawCopy() - for i := range raw.Devices { - raw.Devices[i].Paused = paused - } - for i := range raw.Folders { - raw.Folders[i].Paused = paused - } - if _, err := cfg.Replace(raw); err != nil { +func setPauseState(cfgWrapper config.Wrapper, paused bool) { + _, err := cfgWrapper.Modify(func(cfg *config.Configuration) { + for i := range cfg.Devices { + cfg.Devices[i].Paused = paused + } + for i := range cfg.Folders { + cfg.Folders[i].Paused = paused + } + }) + if err != nil { l.Warnln("Cannot adjust paused state:", err) os.Exit(svcutil.ExitError.AsInt()) } diff --git a/lib/api/api.go b/lib/api/api.go index a94083281..83e5eb1b6 100644 --- a/lib/api/api.go +++ b/lib/api/api.go @@ -294,7 +294,6 @@ func (s *service) Serve(ctx context.Context) error { Router: restMux, id: s.id, cfg: s.cfg, - mut: sync.NewMutex(), } configBuilder.registerConfig("/rest/config") @@ -1402,31 +1401,36 @@ func (s *service) makeDevicePauseHandler(paused bool) http.HandlerFunc { var qs = r.URL.Query() var deviceStr = qs.Get("device") - var cfgs []config.DeviceConfiguration - - if deviceStr == "" { - for _, cfg := range s.cfg.Devices() { - cfg.Paused = paused - cfgs = append(cfgs, cfg) + var msg string + var status int + _, err := s.cfg.Modify(func(cfg *config.Configuration) { + if deviceStr == "" { + for i := range cfg.Devices { + cfg.Devices[i].Paused = paused + } + return } - } else { + device, err := protocol.DeviceIDFromString(deviceStr) if err != nil { - http.Error(w, err.Error(), 500) + msg = err.Error() + status = 500 return } - cfg, ok := s.cfg.Devices()[device] + _, i, ok := cfg.Device(device) if !ok { - http.Error(w, "not found", http.StatusNotFound) + msg = "not found" + status = http.StatusNotFound return } - cfg.Paused = paused - cfgs = append(cfgs, cfg) - } + cfg.Devices[i].Paused = paused + }) - if _, err := s.cfg.SetDevices(cfgs); err != nil { + if msg != "" { + http.Error(w, msg, status) + } else if err != nil { http.Error(w, err.Error(), 500) } } diff --git a/lib/api/api_test.go b/lib/api/api_test.go index fcc4c00c4..8da511f4f 100644 --- a/lib/api/api_test.go +++ b/lib/api/api_test.go @@ -1253,6 +1253,11 @@ func TestConfigChanges(t *testing.T) { defer os.Remove(tmpFile.Name()) w := config.Wrap(tmpFile.Name(), cfg, protocol.LocalDeviceID, events.NoopLogger) tmpFile.Close() + if cfgService, ok := w.(suture.Service); ok { + cfgCtx, cfgCancel := context.WithCancel(context.Background()) + go cfgService.Serve(cfgCtx) + defer cfgCancel() + } baseURL, cancel, err := startHTTP(w) if err != nil { t.Fatal("Unexpected error from getting base URL:", err) diff --git a/lib/api/confighandler.go b/lib/api/confighandler.go index dc580adbe..c832039b8 100644 --- a/lib/api/confighandler.go +++ b/lib/api/confighandler.go @@ -17,14 +17,12 @@ import ( "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/protocol" - "github.com/syncthing/syncthing/lib/sync" ) type configMuxBuilder struct { *httprouter.Router id protocol.DeviceID cfg config.Wrapper - mut sync.Mutex } func (c *configMuxBuilder) registerConfig(path string) { @@ -59,14 +57,14 @@ func (c *configMuxBuilder) registerFolders(path string) { }) c.HandlerFunc(http.MethodPut, path, func(w http.ResponseWriter, r *http.Request) { - c.mut.Lock() - defer c.mut.Unlock() var folders []config.FolderConfiguration if err := unmarshalTo(r.Body, &folders); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetFolders(folders) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.SetFolders(folders) + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -75,19 +73,7 @@ func (c *configMuxBuilder) registerFolders(path string) { }) c.HandlerFunc(http.MethodPost, path, func(w http.ResponseWriter, r *http.Request) { - c.mut.Lock() - defer c.mut.Unlock() - var folder config.FolderConfiguration - if err := unmarshalTo(r.Body, &folder); err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - waiter, err := c.cfg.SetFolder(folder) - if err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - c.finish(w, waiter) + c.adjustFolder(w, r, config.FolderConfiguration{}) }) } @@ -97,14 +83,14 @@ func (c *configMuxBuilder) registerDevices(path string) { }) c.HandlerFunc(http.MethodPut, path, func(w http.ResponseWriter, r *http.Request) { - c.mut.Lock() - defer c.mut.Unlock() var devices []config.DeviceConfiguration if err := unmarshalTo(r.Body, &devices); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetDevices(devices) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.SetDevices(devices) + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -113,14 +99,14 @@ func (c *configMuxBuilder) registerDevices(path string) { }) c.HandlerFunc(http.MethodPost, path, func(w http.ResponseWriter, r *http.Request) { - c.mut.Lock() - defer c.mut.Unlock() var device config.DeviceConfiguration if err := unmarshalTo(r.Body, &device); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetDevice(device) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(device) + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -153,8 +139,6 @@ func (c *configMuxBuilder) registerFolder(path string) { }) c.Handle(http.MethodDelete, path, func(w http.ResponseWriter, _ *http.Request, p httprouter.Params) { - c.mut.Lock() - defer c.mut.Unlock() waiter, err := c.cfg.RemoveFolder(p.ByName("id")) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) @@ -196,8 +180,6 @@ func (c *configMuxBuilder) registerDevice(path string) { }) c.Handle(http.MethodDelete, path, func(w http.ResponseWriter, _ *http.Request, p httprouter.Params) { - c.mut.Lock() - defer c.mut.Unlock() id, err := protocol.DeviceIDFromString(p.ByName("id")) waiter, err := c.cfg.RemoveDevice(id) if err != nil { @@ -251,22 +233,27 @@ func (c *configMuxBuilder) registerGUI(path string) { } func (c *configMuxBuilder) adjustConfig(w http.ResponseWriter, r *http.Request) { - c.mut.Lock() - defer c.mut.Unlock() - cfg, err := config.ReadJSON(r.Body, c.id) + to, err := config.ReadJSON(r.Body, c.id) r.Body.Close() if err != nil { l.Warnln("Decoding posted config:", err) http.Error(w, err.Error(), http.StatusBadRequest) return } - if cfg.GUI.Password, err = checkGUIPassword(c.cfg.GUI().Password, cfg.GUI.Password); err != nil { - l.Warnln("bcrypting password:", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - waiter, err := c.cfg.Replace(cfg) - if err != nil { + var errMsg string + var status int + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + if to.GUI.Password, err = checkGUIPassword(cfg.GUI.Password, to.GUI.Password); err != nil { + l.Warnln("bcrypting password:", err) + errMsg = err.Error() + status = http.StatusInternalServerError + return + } + *cfg = to + }) + if errMsg != "" { + http.Error(w, errMsg, status) + } else if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } @@ -274,13 +261,13 @@ func (c *configMuxBuilder) adjustConfig(w http.ResponseWriter, r *http.Request) } func (c *configMuxBuilder) adjustFolder(w http.ResponseWriter, r *http.Request, folder config.FolderConfiguration) { - c.mut.Lock() - defer c.mut.Unlock() if err := unmarshalTo(r.Body, &folder); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetFolder(folder) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.SetFolder(folder) + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -289,13 +276,13 @@ func (c *configMuxBuilder) adjustFolder(w http.ResponseWriter, r *http.Request, } func (c *configMuxBuilder) adjustDevice(w http.ResponseWriter, r *http.Request, device config.DeviceConfiguration) { - c.mut.Lock() - defer c.mut.Unlock() if err := unmarshalTo(r.Body, &device); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetDevice(device) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(device) + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -304,13 +291,13 @@ func (c *configMuxBuilder) adjustDevice(w http.ResponseWriter, r *http.Request, } func (c *configMuxBuilder) adjustOptions(w http.ResponseWriter, r *http.Request, opts config.OptionsConfiguration) { - c.mut.Lock() - defer c.mut.Unlock() if err := unmarshalTo(r.Body, &opts); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetOptions(opts) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.Options = opts + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -319,21 +306,26 @@ func (c *configMuxBuilder) adjustOptions(w http.ResponseWriter, r *http.Request, } func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui config.GUIConfiguration) { - c.mut.Lock() - defer c.mut.Unlock() oldPassword := gui.Password err := unmarshalTo(r.Body, &gui) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - if gui.Password, err = checkGUIPassword(oldPassword, gui.Password); err != nil { - l.Warnln("bcrypting password:", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - waiter, err := c.cfg.SetGUI(gui) - if err != nil { + var errMsg string + var status int + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + if gui.Password, err = checkGUIPassword(oldPassword, gui.Password); err != nil { + l.Warnln("bcrypting password:", err) + errMsg = err.Error() + status = http.StatusInternalServerError + return + } + cfg.GUI = gui + }) + if errMsg != "" { + http.Error(w, errMsg, status) + } else if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return } @@ -341,13 +333,13 @@ func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui } func (c *configMuxBuilder) adjustLDAP(w http.ResponseWriter, r *http.Request, ldap config.LDAPConfiguration) { - c.mut.Lock() - defer c.mut.Unlock() if err := unmarshalTo(r.Body, &ldap); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - waiter, err := c.cfg.SetLDAP(ldap) + waiter, err := c.cfg.Modify(func(cfg *config.Configuration) { + cfg.LDAP = ldap + }) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/lib/api/mocked_config_test.go b/lib/api/mocked_config_test.go index dd4a68ce2..55e03aa5d 100644 --- a/lib/api/mocked_config_test.go +++ b/lib/api/mocked_config_test.go @@ -28,10 +28,6 @@ func (c *mockedConfig) LDAP() config.LDAPConfiguration { return config.LDAPConfiguration{} } -func (c *mockedConfig) SetLDAP(config.LDAPConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - func (c *mockedConfig) RawCopy() config.Configuration { cfg := config.Configuration{} util.SetDefaults(&cfg.Options) @@ -42,11 +38,13 @@ func (c *mockedConfig) Options() config.OptionsConfiguration { return config.OptionsConfiguration{} } -func (c *mockedConfig) Replace(cfg config.Configuration) (config.Waiter, error) { +func (c *mockedConfig) Modify(config.ModifyFunction) (config.Waiter, error) { return noopWaiter{}, nil } -func (c *mockedConfig) Subscribe(cm config.Committer) {} +func (c *mockedConfig) Subscribe(cm config.Committer) config.Configuration { + return config.Configuration{} +} func (c *mockedConfig) Unsubscribe(cm config.Committer) {} @@ -62,14 +60,6 @@ func (c *mockedConfig) DeviceList() []config.DeviceConfiguration { return nil } -func (c *mockedConfig) SetDevice(config.DeviceConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - -func (c *mockedConfig) SetDevices([]config.DeviceConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - func (c *mockedConfig) Save() error { return nil } @@ -86,14 +76,6 @@ func (c *mockedConfig) ConfigPath() string { return "" } -func (c *mockedConfig) SetGUI(gui config.GUIConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - -func (c *mockedConfig) SetOptions(opts config.OptionsConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - func (c *mockedConfig) Folder(id string) (config.FolderConfiguration, bool) { return config.FolderConfiguration{}, false } @@ -102,14 +84,6 @@ func (c *mockedConfig) FolderList() []config.FolderConfiguration { return nil } -func (c *mockedConfig) SetFolder(fld config.FolderConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - -func (c *mockedConfig) SetFolders(folders []config.FolderConfiguration) (config.Waiter, error) { - return noopWaiter{}, nil -} - func (c *mockedConfig) RemoveFolder(id string) (config.Waiter, error) { return noopWaiter{}, nil } diff --git a/lib/config/commit_test.go b/lib/config/commit_test.go index aca431cbc..ac229e68e 100644 --- a/lib/config/commit_test.go +++ b/lib/config/commit_test.go @@ -41,10 +41,20 @@ func (validationError) String() string { return "validationError" } -func TestReplaceCommit(t *testing.T) { - t.Skip("broken, fails randomly, #3834") +func replace(t testing.TB, w Wrapper, to Configuration) { + t.Helper() + waiter, err := w.Modify(func(cfg *Configuration) { + *cfg = to + }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() +} +func TestReplaceCommit(t *testing.T) { w := wrap("/dev/null", Configuration{Version: 0}, device1) + defer w.stop() if w.RawCopy().Version != 0 { t.Fatal("Config incorrect") } @@ -52,10 +62,7 @@ func TestReplaceCommit(t *testing.T) { // Replace config. We should get back a clean response and the config // should change. - _, err := w.Replace(Configuration{Version: 1}) - if err != nil { - t.Fatal("Should not have a validation error:", err) - } + replace(t, w, Configuration{Version: 1}) if w.RequiresRestart() { t.Fatal("Should not require restart") } @@ -69,11 +76,7 @@ func TestReplaceCommit(t *testing.T) { sub0 := requiresRestart{committed: make(chan struct{}, 1)} w.Subscribe(sub0) - _, err = w.Replace(Configuration{Version: 2}) - if err != nil { - t.Fatal("Should not have a validation error:", err) - } - + replace(t, w, Configuration{Version: 1}) <-sub0.committed if !w.RequiresRestart() { t.Fatal("Should require restart") @@ -87,7 +90,9 @@ func TestReplaceCommit(t *testing.T) { w.Subscribe(validationError{}) - _, err = w.Replace(Configuration{Version: 3}) + _, err := w.Modify(func(cfg *Configuration) { + *cfg = Configuration{Version: 3} + }) if err == nil { t.Fatal("Should have a validation error") } diff --git a/lib/config/config.go b/lib/config/config.go index 8cab094a1..47b68ba29 100644 --- a/lib/config/config.go +++ b/lib/config/config.go @@ -372,6 +372,15 @@ func (cfg *Configuration) applyMigrations() { migrationsMut.Unlock() } +func (cfg *Configuration) Device(id protocol.DeviceID) (DeviceConfiguration, int, bool) { + for i, device := range cfg.Devices { + if device.DeviceID == id { + return device, i, true + } + } + return DeviceConfiguration{}, 0, false +} + // DeviceMap returns a map of device ID to device configuration for the given configuration. func (cfg *Configuration) DeviceMap() map[protocol.DeviceID]DeviceConfiguration { m := make(map[protocol.DeviceID]DeviceConfiguration, len(cfg.Devices)) @@ -381,6 +390,44 @@ func (cfg *Configuration) DeviceMap() map[protocol.DeviceID]DeviceConfiguration return m } +func (cfg *Configuration) SetDevice(device DeviceConfiguration) { + cfg.SetDevices([]DeviceConfiguration{device}) +} + +func (cfg *Configuration) SetDevices(devices []DeviceConfiguration) { + inds := make(map[protocol.DeviceID]int, len(cfg.Devices)) + for i, device := range cfg.Devices { + inds[device.DeviceID] = i + } + filtered := devices[:0] + for _, device := range devices { + if i, ok := inds[device.DeviceID]; ok { + cfg.Devices[i] = device + } else { + filtered = append(filtered, device) + } + } + cfg.Devices = append(cfg.Devices, filtered...) +} + +func (cfg *Configuration) Folder(id string) (FolderConfiguration, int, bool) { + for i, folder := range cfg.Folders { + if folder.ID == id { + return folder, i, true + } + } + return FolderConfiguration{}, 0, false +} + +// FolderMap returns a map of folder ID to folder configuration for the given configuration. +func (cfg *Configuration) FolderMap() map[string]FolderConfiguration { + m := make(map[string]FolderConfiguration, len(cfg.Folders)) + for _, folder := range cfg.Folders { + m[folder.ID] = folder + } + return m +} + // FolderPasswords returns the folder passwords set for this device, for // folders that have an encryption password set. func (cfg Configuration) FolderPasswords(device protocol.DeviceID) map[string]string { @@ -397,6 +444,26 @@ nextFolder: return res } +func (cfg *Configuration) SetFolder(folder FolderConfiguration) { + cfg.SetFolders([]FolderConfiguration{folder}) +} + +func (cfg *Configuration) SetFolders(folders []FolderConfiguration) { + inds := make(map[string]int, len(cfg.Folders)) + for i, folder := range cfg.Folders { + inds[folder.ID] = i + } + filtered := folders[:0] + for _, folder := range folders { + if i, ok := inds[folder.ID]; ok { + cfg.Folders[i] = folder + } else { + filtered = append(filtered, folder) + } + } + cfg.Folders = append(cfg.Folders, filtered...) +} + func ensureDevicePresent(devices []FolderDeviceConfiguration, myID protocol.DeviceID) []FolderDeviceConfiguration { for _, device := range devices { if device.DeviceID.Equals(myID) { diff --git a/lib/config/config_test.go b/lib/config/config_test.go index 49f2a8fd3..14df9ca78 100644 --- a/lib/config/config_test.go +++ b/lib/config/config_test.go @@ -8,9 +8,11 @@ package config import ( "bytes" + "context" "encoding/json" "encoding/xml" "fmt" + "io" "io/ioutil" "os" "path/filepath" @@ -21,6 +23,7 @@ import ( "testing" "github.com/d4l3k/messagediff" + "github.com/thejerf/suture/v4" "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/fs" @@ -95,7 +98,8 @@ func TestDeviceConfig(t *testing.T) { } os.RemoveAll(filepath.Join("testdata", DefaultMarkerName)) - wr, err := load(cfgFile, device1) + wr, wrCancel, err := copyAndLoad(cfgFile, device1) + defer wrCancel() if err != nil { t.Fatal(err) } @@ -107,7 +111,7 @@ func TestDeviceConfig(t *testing.T) { t.Fatal("Unexpected file") } - cfg := wr.(*wrapper).cfg + cfg := wr.Wrapper.(*wrapper).cfg expectedFolders := []FolderConfiguration{ { @@ -170,7 +174,8 @@ func TestDeviceConfig(t *testing.T) { } func TestNoListenAddresses(t *testing.T) { - cfg, err := load("testdata/nolistenaddress.xml", device1) + cfg, cfgCancel, err := copyAndLoad("testdata/nolistenaddress.xml", device1) + defer cfgCancel() if err != nil { t.Error(err) } @@ -228,7 +233,8 @@ func TestOverriddenValues(t *testing.T) { } os.Unsetenv("STNOUPGRADE") - cfg, err := load("testdata/overridenvalues.xml", device1) + cfg, cfgCancel, err := copyAndLoad("testdata/overridenvalues.xml", device1) + defer cfgCancel() if err != nil { t.Error(err) } @@ -269,7 +275,8 @@ func TestDeviceAddressesDynamic(t *testing.T) { }, } - cfg, err := load("testdata/deviceaddressesdynamic.xml", device4) + cfg, cfgCancel, err := copyAndLoad("testdata/deviceaddressesdynamic.xml", device4) + defer cfgCancel() if err != nil { t.Error(err) } @@ -314,7 +321,8 @@ func TestDeviceCompression(t *testing.T) { }, } - cfg, err := load("testdata/devicecompression.xml", device4) + cfg, cfgCancel, err := copyAndLoad("testdata/devicecompression.xml", device4) + defer cfgCancel() if err != nil { t.Error(err) } @@ -356,7 +364,8 @@ func TestDeviceAddressesStatic(t *testing.T) { }, } - cfg, err := load("testdata/deviceaddressesstatic.xml", device4) + cfg, cfgCancel, err := copyAndLoad("testdata/deviceaddressesstatic.xml", device4) + defer cfgCancel() if err != nil { t.Error(err) } @@ -368,7 +377,8 @@ func TestDeviceAddressesStatic(t *testing.T) { } func TestVersioningConfig(t *testing.T) { - cfg, err := load("testdata/versioningconfig.xml", device4) + cfg, cfgCancel, err := copyAndLoad("testdata/versioningconfig.xml", device4) + defer cfgCancel() if err != nil { t.Error(err) } @@ -395,7 +405,8 @@ func TestIssue1262(t *testing.T) { t.Skipf("path gets converted to absolute as part of the filesystem initialization on linux") } - cfg, err := load("testdata/issue-1262.xml", device4) + cfg, cfgCancel, err := copyAndLoad("testdata/issue-1262.xml", device4) + defer cfgCancel() if err != nil { t.Fatal(err) } @@ -409,7 +420,8 @@ func TestIssue1262(t *testing.T) { } func TestIssue1750(t *testing.T) { - cfg, err := load("testdata/issue-1750.xml", device4) + cfg, cfgCancel, err := copyAndLoad("testdata/issue-1750.xml", device4) + defer cfgCancel() if err != nil { t.Fatal(err) } @@ -505,6 +517,7 @@ func TestFolderCheckPath(t *testing.T) { func TestNewSaveLoad(t *testing.T) { path := "testdata/temp.xml" os.Remove(path) + defer os.Remove(path) exists := func(path string) bool { _, err := os.Stat(path) @@ -513,6 +526,7 @@ func TestNewSaveLoad(t *testing.T) { intCfg := New(device1) cfg := wrap(path, intCfg, device1) + defer cfg.stop() if exists(path) { t.Error(path, "exists") @@ -527,6 +541,7 @@ func TestNewSaveLoad(t *testing.T) { } cfg2, err := load(path, device1) + defer cfg2.stop() if err != nil { t.Error(err) } @@ -534,8 +549,6 @@ func TestNewSaveLoad(t *testing.T) { if diff, equal := messagediff.PrettyDiff(cfg.RawCopy(), cfg2.RawCopy()); !equal { t.Errorf("Configs are not equal. Diff:\n%s", diff) } - - os.Remove(path) } func TestPrepare(t *testing.T) { @@ -553,7 +566,8 @@ func TestPrepare(t *testing.T) { } func TestCopy(t *testing.T) { - wrapper, err := load("testdata/example.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/example.xml", device1) + defer wrapperCancel() if err != nil { t.Fatal(err) } @@ -592,7 +606,8 @@ func TestCopy(t *testing.T) { } func TestPullOrder(t *testing.T) { - wrapper, err := load("testdata/pullorder.xml", device1) + wrapper, wrapperCleanup, err := copyAndLoad("testdata/pullorder.xml", device1) + defer wrapperCleanup() if err != nil { t.Fatal(err) } @@ -632,8 +647,9 @@ func TestPullOrder(t *testing.T) { if err != nil { t.Fatal(err) } - wrapper = wrap("testdata/pullorder.xml", cfg, device1) - folders = wrapper.Folders() + wrapper2 := wrap(wrapper.ConfigPath(), cfg, device1) + defer wrapper2.stop() + folders = wrapper2.Folders() for _, tc := range expected { if actual := folders[tc.name].Order; actual != tc.order { @@ -643,7 +659,8 @@ func TestPullOrder(t *testing.T) { } func TestLargeRescanInterval(t *testing.T) { - wrapper, err := load("testdata/largeinterval.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/largeinterval.xml", device1) + defer wrapperCancel() if err != nil { t.Fatal(err) } @@ -681,7 +698,8 @@ func TestGUIConfigURL(t *testing.T) { func TestDuplicateDevices(t *testing.T) { // Duplicate devices should be removed - wrapper, err := load("testdata/dupdevices.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/dupdevices.xml", device1) + defer wrapperCancel() if err != nil { t.Fatal(err) } @@ -699,7 +717,8 @@ func TestDuplicateDevices(t *testing.T) { func TestDuplicateFolders(t *testing.T) { // Duplicate folders are a loading error - _, err := load("testdata/dupfolders.xml", device1) + _, _Cancel, err := copyAndLoad("testdata/dupfolders.xml", device1) + defer _Cancel() if err == nil || !strings.Contains(err.Error(), errFolderIDDuplicate.Error()) { t.Fatal(`Expected error to mention "duplicate folder ID":`, err) } @@ -710,7 +729,8 @@ func TestEmptyFolderPaths(t *testing.T) { // get messed up by the prepare steps (e.g., become the current dir or // get a slash added so that it becomes the root directory or similar). - _, err := load("testdata/nopath.xml", device1) + _, _Cancel, err := copyAndLoad("testdata/nopath.xml", device1) + defer _Cancel() if err == nil || !strings.Contains(err.Error(), errFolderPathEmpty.Error()) { t.Fatal("Expected error due to empty folder path, got", err) } @@ -779,7 +799,8 @@ func TestIgnoredDevices(t *testing.T) { // Verify that ignored devices that are also present in the // configuration are not in fact ignored. - wrapper, err := load("testdata/ignoreddevices.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/ignoreddevices.xml", device1) + defer wrapperCancel() if err != nil { t.Fatal(err) } @@ -797,7 +818,8 @@ func TestIgnoredFolders(t *testing.T) { // configuration are not in fact ignored. // Also, verify that folders that are shared with a device are not ignored. - wrapper, err := load("testdata/ignoredfolders.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/ignoredfolders.xml", device1) + defer wrapperCancel() if err != nil { t.Fatal(err) } @@ -833,7 +855,8 @@ func TestIgnoredFolders(t *testing.T) { func TestGetDevice(t *testing.T) { // Verify that the Device() call does the right thing - wrapper, err := load("testdata/ignoreddevices.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/ignoreddevices.xml", device1) + defer wrapperCancel() if err != nil { t.Fatal(err) } @@ -860,7 +883,8 @@ func TestGetDevice(t *testing.T) { } func TestSharesRemovedOnDeviceRemoval(t *testing.T) { - wrapper, err := load("testdata/example.xml", device1) + wrapper, wrapperCancel, err := copyAndLoad("testdata/example.xml", device1) + defer wrapperCancel() if err != nil { t.Errorf("Failed: %s", err) } @@ -872,10 +896,7 @@ func TestSharesRemovedOnDeviceRemoval(t *testing.T) { t.Error("Should have less devices") } - _, err = wrapper.Replace(raw) - if err != nil { - t.Errorf("Failed: %s", err) - } + replace(t, wrapper, raw) raw = wrapper.RawCopy() if len(raw.Folders[0].Devices) > len(raw.Devices) { @@ -947,6 +968,7 @@ func TestIssue4219(t *testing.T) { } w := wrap("/tmp/cfg", cfg, myID) + defer w.stop() if !w.IgnoredFolder(device2, "t1") { t.Error("Folder device2 t1 should be ignored") } @@ -1157,13 +1179,80 @@ func defaultConfigAsMap() map[string]interface{} { return tmp } -func load(path string, myID protocol.DeviceID) (Wrapper, error) { - cfg, _, err := Load(path, myID, events.NoopLogger) - return cfg, err +func copyToTmp(path string) (string, error) { + orig, err := os.Open(path) + if err != nil { + return "", err + } + defer orig.Close() + temp, err := ioutil.TempFile("", "syncthing-configTest-") + if err != nil { + return "", err + } + defer temp.Close() + if _, err := io.Copy(temp, orig); err != nil { + return "", err + } + return temp.Name(), nil } -func wrap(path string, cfg Configuration, myID protocol.DeviceID) Wrapper { - return Wrap(path, cfg, myID, events.NoopLogger) +func copyAndLoad(path string, myID protocol.DeviceID) (*testWrapper, func(), error) { + temp, err := copyToTmp(path) + if err != nil { + return nil, func() {}, err + } + wrapper, err := load(temp, myID) + if err != nil { + return nil, func() {}, err + } + return wrapper, func() { + wrapper.stop() + os.Remove(temp) + }, nil +} + +func load(path string, myID protocol.DeviceID) (*testWrapper, error) { + cfg, _, err := Load(path, myID, events.NoopLogger) + if err != nil { + return nil, err + } + return startWrapper(cfg), nil +} + +func wrap(path string, cfg Configuration, myID protocol.DeviceID) *testWrapper { + wrapper := Wrap(path, cfg, myID, events.NoopLogger) + return startWrapper(wrapper) +} + +type testWrapper struct { + Wrapper + cancel context.CancelFunc + done chan struct{} +} + +func (w *testWrapper) stop() { + w.cancel() + <-w.done +} + +func startWrapper(wrapper Wrapper) *testWrapper { + tw := &testWrapper{ + Wrapper: wrapper, + done: make(chan struct{}), + } + s, ok := wrapper.(suture.Service) + if !ok { + tw.cancel = func() {} + close(tw.done) + return tw + } + ctx, cancel := context.WithCancel(context.Background()) + tw.cancel = cancel + go func() { + s.Serve(ctx) + close(tw.done) + }() + return tw } func TestInternalVersioningConfiguration(t *testing.T) { diff --git a/lib/config/wrapper.go b/lib/config/wrapper.go index ad678cae0..a59150e3e 100644 --- a/lib/config/wrapper.go +++ b/lib/config/wrapper.go @@ -7,8 +7,12 @@ package config import ( + "context" + "errors" "os" + "reflect" "sync/atomic" + "time" "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/osutil" @@ -16,6 +20,13 @@ import ( "github.com/syncthing/syncthing/lib/sync" ) +const ( + maxModifications = 1000 + minSaveInterval = 5 * time.Second +) + +var errTooManyModifications = errors.New("too many concurrent config modifications") + // The Committer interface is implemented by objects that need to know about // or have a say in configuration changes. // @@ -35,6 +46,10 @@ import ( // false will result in a "restart needed" response to the API/user. Note that // the new configuration will still have been applied by those who were // capable of doing so. +// +// A Committer must take care not to hold any locks while changing the +// configuration (e.g. calling Wrapper.SetFolder), that are also acquired in any +// methods of the Committer interface. type Committer interface { VerifyConfiguration(from, to Configuration) error CommitConfiguration(from, to Configuration) (handled bool) @@ -50,45 +65,47 @@ type noopWaiter struct{} func (noopWaiter) Wait() {} -// A Wrapper around a Configuration that manages loads, saves and published -// notifications of changes to registered Handlers +// ModifyFunction gets a pointer to a copy of the currently active configuration +// for modification. +type ModifyFunction func(*Configuration) + +// Wrapper handles a Configuration, i.e. it provides methods to access, change +// and save the config, and notifies registered subscribers (Committer) of +// changes. +// +// Modify allows changing the currently active configuration through the given +// ModifyFunction. It can be called concurrently: All calls will be queued and +// called in order. type Wrapper interface { ConfigPath() string MyID() protocol.DeviceID RawCopy() Configuration - Replace(cfg Configuration) (Waiter, error) RequiresRestart() bool Save() error - GUI() GUIConfiguration - SetGUI(gui GUIConfiguration) (Waiter, error) - LDAP() LDAPConfiguration - SetLDAP(ldap LDAPConfiguration) (Waiter, error) + Modify(ModifyFunction) (Waiter, error) + RemoveFolder(id string) (Waiter, error) + RemoveDevice(id protocol.DeviceID) (Waiter, error) + GUI() GUIConfiguration + LDAP() LDAPConfiguration Options() OptionsConfiguration - SetOptions(opts OptionsConfiguration) (Waiter, error) Folder(id string) (FolderConfiguration, bool) Folders() map[string]FolderConfiguration FolderList() []FolderConfiguration - RemoveFolder(id string) (Waiter, error) - SetFolder(fld FolderConfiguration) (Waiter, error) - SetFolders(folders []FolderConfiguration) (Waiter, error) FolderPasswords(device protocol.DeviceID) map[string]string Device(id protocol.DeviceID) (DeviceConfiguration, bool) Devices() map[protocol.DeviceID]DeviceConfiguration DeviceList() []DeviceConfiguration - RemoveDevice(id protocol.DeviceID) (Waiter, error) - SetDevice(DeviceConfiguration) (Waiter, error) - SetDevices([]DeviceConfiguration) (Waiter, error) IgnoredDevices() []ObservedDevice IgnoredDevice(id protocol.DeviceID) bool IgnoredFolder(device protocol.DeviceID, folder string) bool - Subscribe(c Committer) + Subscribe(c Committer) Configuration Unsubscribe(c Committer) } @@ -97,6 +114,7 @@ type wrapper struct { path string evLogger events.Logger myID protocol.DeviceID + queue chan modifyEntry waiter Waiter // Latest ongoing config change subs []Committer @@ -107,12 +125,15 @@ type wrapper struct { // Wrap wraps an existing Configuration structure and ties it to a file on // disk. +// The returned Wrapper is a suture.Service, thus needs to be started (added to +// a supervisor). func Wrap(path string, cfg Configuration, myID protocol.DeviceID, evLogger events.Logger) Wrapper { w := &wrapper{ cfg: cfg, path: path, evLogger: evLogger, myID: myID, + queue: make(chan modifyEntry, maxModifications), waiter: noopWaiter{}, // Noop until first config change mut: sync.NewMutex(), } @@ -121,6 +142,8 @@ func Wrap(path string, cfg Configuration, myID protocol.DeviceID, evLogger event // Load loads an existing file on disk and returns a new configuration // wrapper. +// The returned Wrapper is a suture.Service, thus needs to be started (added to +// a supervisor). func Load(path string, myID protocol.DeviceID, evLogger events.Logger) (Wrapper, int, error) { fd, err := os.Open(path) if err != nil { @@ -145,11 +168,13 @@ func (w *wrapper) MyID() protocol.DeviceID { } // Subscribe registers the given handler to be called on any future -// configuration changes. -func (w *wrapper) Subscribe(c Committer) { +// configuration changes. It returns the config that is in effect while +// subscribing, that can be used for initial setup. +func (w *wrapper) Subscribe(c Committer) Configuration { w.mut.Lock() + defer w.mut.Unlock() w.subs = append(w.subs, c) - w.mut.Unlock() + return w.cfg.Copy() } // Unsubscribe de-registers the given handler from any future calls to @@ -179,11 +204,84 @@ func (w *wrapper) RawCopy() Configuration { return w.cfg.Copy() } -// Replace swaps the current configuration object for the given one. -func (w *wrapper) Replace(cfg Configuration) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - return w.replaceLocked(cfg.Copy()) +func (w *wrapper) Modify(fn ModifyFunction) (Waiter, error) { + return w.modifyQueued(fn) +} + +func (w *wrapper) modifyQueued(modifyFunc ModifyFunction) (Waiter, error) { + e := modifyEntry{ + modifyFunc: modifyFunc, + res: make(chan modifyResult), + } + select { + case w.queue <- e: + default: + return noopWaiter{}, errTooManyModifications + } + res := <-e.res + return res.w, res.err +} + +func (w *wrapper) Serve(ctx context.Context) error { + defer w.serveSave() + + var e modifyEntry + saveTimer := time.NewTimer(0) + <-saveTimer.C + saveTimerRunning := false + for { + select { + case e = <-w.queue: + case <-saveTimer.C: + w.serveSave() + saveTimerRunning = false + continue + case <-ctx.Done(): + return ctx.Err() + } + + var waiter Waiter = noopWaiter{} + var err error + + // Let the caller modify the config. + to := w.RawCopy() + e.modifyFunc(&to) + + // Check if the config was actually changed at all. + w.mut.Lock() + if !reflect.DeepEqual(w.cfg, to) { + waiter, err = w.replaceLocked(to) + if !saveTimerRunning { + saveTimer.Reset(minSaveInterval) + saveTimerRunning = true + } + } + w.mut.Unlock() + + e.res <- modifyResult{ + w: waiter, + err: err, + } + + // Wait for all subscriber to handle the config change before continuing + // to process the next change. + done := make(chan struct{}) + go func() { + waiter.Wait() + close(done) + }() + select { + case <-done: + case <-ctx.Done(): + return ctx.Err() + } + } +} + +func (w *wrapper) serveSave() { + if err := w.Save(); err != nil { + l.Warnln("Failed to save config:", err) + } } func (w *wrapper) replaceLocked(to Configuration) (Waiter, error) { @@ -246,55 +344,16 @@ func (w *wrapper) DeviceList() []DeviceConfiguration { return w.cfg.Copy().Devices } -// SetDevices adds new devices to the configuration, or overwrites existing -// devices with the same ID. -func (w *wrapper) SetDevices(devs []DeviceConfiguration) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - - newCfg := w.cfg.Copy() - var replaced bool - for oldIndex := range devs { - replaced = false - for newIndex := range newCfg.Devices { - if newCfg.Devices[newIndex].DeviceID == devs[oldIndex].DeviceID { - newCfg.Devices[newIndex] = devs[oldIndex].Copy() - replaced = true - break - } - } - if !replaced { - newCfg.Devices = append(newCfg.Devices, devs[oldIndex].Copy()) - } - } - - return w.replaceLocked(newCfg) -} - -// SetDevice adds a new device to the configuration, or overwrites an existing -// device with the same ID. -func (w *wrapper) SetDevice(dev DeviceConfiguration) (Waiter, error) { - return w.SetDevices([]DeviceConfiguration{dev}) -} - // RemoveDevice removes the device from the configuration func (w *wrapper) RemoveDevice(id protocol.DeviceID) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - - newCfg := w.cfg.Copy() - for i := range newCfg.Devices { - if newCfg.Devices[i].DeviceID == id { - newCfg.Devices = append(newCfg.Devices[:i], newCfg.Devices[i+1:]...) - return w.replaceLocked(newCfg) + return w.modifyQueued(func(cfg *Configuration) { + if _, i, ok := cfg.Device(id); ok { + cfg.Devices = append(cfg.Devices[:i], cfg.Devices[i+1:]...) } - } - - return noopWaiter{}, nil + }) } -// Folders returns a map of folders. Folder structures should not be changed, -// other than for the purpose of updating via SetFolder(). +// Folders returns a map of folders. func (w *wrapper) Folders() map[string]FolderConfiguration { w.mut.Lock() defer w.mut.Unlock() @@ -312,51 +371,13 @@ func (w *wrapper) FolderList() []FolderConfiguration { return w.cfg.Copy().Folders } -// SetFolder adds a new folder to the configuration, or overwrites an existing -// folder with the same ID. -func (w *wrapper) SetFolder(fld FolderConfiguration) (Waiter, error) { - return w.SetFolders([]FolderConfiguration{fld}) -} - -// SetFolders adds new folders to the configuration, or overwrites existing -// folders with the same ID. -func (w *wrapper) SetFolders(folders []FolderConfiguration) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - - newCfg := w.cfg.Copy() - - inds := make(map[string]int, len(w.cfg.Folders)) - for i, folder := range newCfg.Folders { - inds[folder.ID] = i - } - filtered := folders[:0] - for _, folder := range folders { - if i, ok := inds[folder.ID]; ok { - newCfg.Folders[i] = folder - } else { - filtered = append(filtered, folder) - } - } - newCfg.Folders = append(newCfg.Folders, filtered...) - - return w.replaceLocked(newCfg) -} - // RemoveFolder removes the folder from the configuration func (w *wrapper) RemoveFolder(id string) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - - newCfg := w.cfg.Copy() - for i := range newCfg.Folders { - if newCfg.Folders[i].ID == id { - newCfg.Folders = append(newCfg.Folders[:i], newCfg.Folders[i+1:]...) - return w.replaceLocked(newCfg) + return w.modifyQueued(func(cfg *Configuration) { + if _, i, ok := cfg.Folder(id); ok { + cfg.Folders = append(cfg.Folders[:i], cfg.Folders[i+1:]...) } - } - - return noopWaiter{}, nil + }) } // FolderPasswords returns the folder passwords set for this device, for @@ -374,29 +395,12 @@ func (w *wrapper) Options() OptionsConfiguration { return w.cfg.Options.Copy() } -// SetOptions replaces the current options configuration object. -func (w *wrapper) SetOptions(opts OptionsConfiguration) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - newCfg := w.cfg.Copy() - newCfg.Options = opts.Copy() - return w.replaceLocked(newCfg) -} - func (w *wrapper) LDAP() LDAPConfiguration { w.mut.Lock() defer w.mut.Unlock() return w.cfg.LDAP.Copy() } -func (w *wrapper) SetLDAP(ldap LDAPConfiguration) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - newCfg := w.cfg.Copy() - newCfg.LDAP = ldap.Copy() - return w.replaceLocked(newCfg) -} - // GUI returns the current GUI configuration object. func (w *wrapper) GUI() GUIConfiguration { w.mut.Lock() @@ -404,15 +408,6 @@ func (w *wrapper) GUI() GUIConfiguration { return w.cfg.GUI.Copy() } -// SetGUI replaces the current GUI configuration object. -func (w *wrapper) SetGUI(gui GUIConfiguration) (Waiter, error) { - w.mut.Lock() - defer w.mut.Unlock() - newCfg := w.cfg.Copy() - newCfg.GUI = gui.Copy() - return w.replaceLocked(newCfg) -} - // IgnoredDevice returns whether or not connection attempts from the given // device should be silently ignored. func (w *wrapper) IgnoredDevice(id protocol.DeviceID) bool { @@ -449,24 +444,22 @@ func (w *wrapper) IgnoredFolder(device protocol.DeviceID, folder string) bool { func (w *wrapper) Device(id protocol.DeviceID) (DeviceConfiguration, bool) { w.mut.Lock() defer w.mut.Unlock() - for _, device := range w.cfg.Devices { - if device.DeviceID == id { - return device.Copy(), true - } + device, _, ok := w.cfg.Device(id) + if !ok { + return DeviceConfiguration{}, false } - return DeviceConfiguration{}, false + return device.Copy(), ok } // Folder returns the configuration for the given folder and an "ok" bool. func (w *wrapper) Folder(id string) (FolderConfiguration, bool) { w.mut.Lock() defer w.mut.Unlock() - for _, folder := range w.cfg.Folders { - if folder.ID == id { - return folder.Copy(), true - } + fcfg, _, ok := w.cfg.Folder(id) + if !ok { + return FolderConfiguration{}, false } - return FolderConfiguration{}, false + return fcfg.Copy(), ok } // Save writes the configuration to disk, and generates a ConfigSaved event. @@ -502,3 +495,13 @@ func (w *wrapper) RequiresRestart() bool { func (w *wrapper) setRequiresRestart() { atomic.StoreUint32(&w.requiresRestart, 1) } + +type modifyEntry struct { + modifyFunc ModifyFunction + res chan modifyResult +} + +type modifyResult struct { + w Waiter + err error +} diff --git a/lib/connections/limiter_test.go b/lib/connections/limiter_test.go index 6483e3e20..beb482c5a 100644 --- a/lib/connections/limiter_test.go +++ b/lib/connections/limiter_test.go @@ -8,6 +8,7 @@ package connections import ( "bytes" + "context" crand "crypto/rand" "io" "math/rand" @@ -16,6 +17,7 @@ import ( "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/events" "github.com/syncthing/syncthing/lib/protocol" + "github.com/thejerf/suture/v4" "golang.org/x/time/rate" ) @@ -29,24 +31,34 @@ func init() { device4, _ = protocol.DeviceIDFromString("P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ2") } -func initConfig() config.Wrapper { - cfg := config.Wrap("/dev/null", config.New(device1), device1, events.NoopLogger) +func initConfig() (config.Wrapper, context.CancelFunc) { + wrapper := config.Wrap("/dev/null", config.New(device1), device1, events.NoopLogger) dev1Conf = config.NewDeviceConfiguration(device1, "device1") dev2Conf = config.NewDeviceConfiguration(device2, "device2") dev3Conf = config.NewDeviceConfiguration(device3, "device3") dev4Conf = config.NewDeviceConfiguration(device4, "device4") + var cancel context.CancelFunc = func() {} + if wrapperService, ok := wrapper.(suture.Service); ok { + var ctx context.Context + ctx, cancel = context.WithCancel(context.Background()) + go wrapperService.Serve(ctx) + } + dev2Conf.MaxRecvKbps = rand.Int() % 100000 dev2Conf.MaxSendKbps = rand.Int() % 100000 - waiter, _ := cfg.SetDevices([]config.DeviceConfiguration{dev1Conf, dev2Conf, dev3Conf, dev4Conf}) + waiter, _ := wrapper.Modify(func(cfg *config.Configuration) { + cfg.SetDevices([]config.DeviceConfiguration{dev1Conf, dev2Conf, dev3Conf, dev4Conf}) + }) waiter.Wait() - return cfg + return wrapper, cancel } func TestLimiterInit(t *testing.T) { - cfg := initConfig() - lim := newLimiter(device1, cfg) + wrapper, wrapperCancel := initConfig() + defer wrapperCancel() + lim := newLimiter(device1, wrapper) device2ReadLimit := dev2Conf.MaxRecvKbps device2WriteLimit := dev2Conf.MaxSendKbps @@ -70,8 +82,9 @@ func TestLimiterInit(t *testing.T) { } func TestSetDeviceLimits(t *testing.T) { - cfg := initConfig() - lim := newLimiter(device1, cfg) + wrapper, wrapperCancel := initConfig() + defer wrapperCancel() + lim := newLimiter(device1, wrapper) // should still be inf/inf because this is local device dev1ReadLimit := rand.Int() % 100000 @@ -87,7 +100,9 @@ func TestSetDeviceLimits(t *testing.T) { dev3ReadLimit := rand.Int() % 10000 dev3Conf.MaxRecvKbps = dev3ReadLimit - waiter, _ := cfg.SetDevices([]config.DeviceConfiguration{dev1Conf, dev2Conf, dev3Conf, dev4Conf}) + waiter, _ := wrapper.Modify(func(cfg *config.Configuration) { + cfg.SetDevices([]config.DeviceConfiguration{dev1Conf, dev2Conf, dev3Conf, dev4Conf}) + }) waiter.Wait() expectedR := map[protocol.DeviceID]*rate.Limiter{ @@ -108,10 +123,11 @@ func TestSetDeviceLimits(t *testing.T) { } func TestRemoveDevice(t *testing.T) { - cfg := initConfig() - lim := newLimiter(device1, cfg) + wrapper, wrapperCancel := initConfig() + defer wrapperCancel() + lim := newLimiter(device1, wrapper) - waiter, _ := cfg.RemoveDevice(device3) + waiter, _ := wrapper.RemoveDevice(device3) waiter.Wait() expectedR := map[protocol.DeviceID]*rate.Limiter{ device2: rate.NewLimiter(rate.Limit(dev2Conf.MaxRecvKbps*1024), limiterBurstSize), @@ -128,15 +144,18 @@ func TestRemoveDevice(t *testing.T) { } func TestAddDevice(t *testing.T) { - cfg := initConfig() - lim := newLimiter(device1, cfg) + wrapper, wrapperCancel := initConfig() + defer wrapperCancel() + lim := newLimiter(device1, wrapper) addedDevice, _ := protocol.DeviceIDFromString("XZJ4UNS-ENI7QGJ-J45DT6G-QSGML2K-6I4XVOG-NAZ7BF5-2VAOWNT-TFDOMQU") addDevConf := config.NewDeviceConfiguration(addedDevice, "addedDevice") addDevConf.MaxRecvKbps = 120 addDevConf.MaxSendKbps = 240 - waiter, _ := cfg.SetDevice(addDevConf) + waiter, _ := wrapper.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(addDevConf) + }) waiter.Wait() expectedR := map[protocol.DeviceID]*rate.Limiter{ @@ -159,17 +178,20 @@ func TestAddDevice(t *testing.T) { } func TestAddAndRemove(t *testing.T) { - cfg := initConfig() - lim := newLimiter(device1, cfg) + wrapper, wrapperCancel := initConfig() + defer wrapperCancel() + lim := newLimiter(device1, wrapper) addedDevice, _ := protocol.DeviceIDFromString("XZJ4UNS-ENI7QGJ-J45DT6G-QSGML2K-6I4XVOG-NAZ7BF5-2VAOWNT-TFDOMQU") addDevConf := config.NewDeviceConfiguration(addedDevice, "addedDevice") addDevConf.MaxRecvKbps = 120 addDevConf.MaxSendKbps = 240 - waiter, _ := cfg.SetDevice(addDevConf) + waiter, _ := wrapper.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(addDevConf) + }) waiter.Wait() - waiter, _ = cfg.RemoveDevice(device3) + waiter, _ = wrapper.RemoveDevice(device3) waiter.Wait() expectedR := map[protocol.DeviceID]*rate.Limiter{ diff --git a/lib/model/folder_recvonly_test.go b/lib/model/folder_recvonly_test.go index 95f5a5778..12ce273fc 100644 --- a/lib/model/folder_recvonly_test.go +++ b/lib/model/folder_recvonly_test.go @@ -25,7 +25,8 @@ func TestRecvOnlyRevertDeletes(t *testing.T) { // Get us a model up and running - m, f := setupROFolder(t) + m, f, wcfgCancel := setupROFolder(t) + defer wcfgCancel() ffs := f.Filesystem() defer cleanupModel(m) @@ -105,7 +106,8 @@ func TestRecvOnlyRevertNeeds(t *testing.T) { // Get us a model up and running - m, f := setupROFolder(t) + m, f, wcfgCancel := setupROFolder(t) + defer wcfgCancel() ffs := f.Filesystem() defer cleanupModel(m) @@ -193,7 +195,8 @@ func TestRecvOnlyRevertNeeds(t *testing.T) { func TestRecvOnlyUndoChanges(t *testing.T) { // Get us a model up and running - m, f := setupROFolder(t) + m, f, wcfgCancel := setupROFolder(t) + defer wcfgCancel() ffs := f.Filesystem() defer cleanupModel(m) @@ -261,7 +264,8 @@ func TestRecvOnlyUndoChanges(t *testing.T) { func TestRecvOnlyDeletedRemoteDrop(t *testing.T) { // Get us a model up and running - m, f := setupROFolder(t) + m, f, wcfgCancel := setupROFolder(t) + defer wcfgCancel() ffs := f.Filesystem() defer cleanupModel(m) @@ -324,7 +328,8 @@ func TestRecvOnlyDeletedRemoteDrop(t *testing.T) { func TestRecvOnlyRemoteUndoChanges(t *testing.T) { // Get us a model up and running - m, f := setupROFolder(t) + m, f, wcfgCancel := setupROFolder(t) + defer wcfgCancel() ffs := f.Filesystem() defer cleanupModel(m) @@ -443,17 +448,17 @@ func setupKnownFiles(t *testing.T, ffs fs.Filesystem, data []byte) []protocol.Fi return knownFiles } -func setupROFolder(t *testing.T) (*testModel, *receiveOnlyFolder) { +func setupROFolder(t *testing.T) (*testModel, *receiveOnlyFolder, context.CancelFunc) { t.Helper() - w := createTmpWrapper(defaultCfg) + w, cancel := createTmpWrapper(defaultCfg) cfg := w.RawCopy() fcfg := testFolderConfigFake() fcfg.ID = "ro" fcfg.Label = "ro" fcfg.Type = config.FolderTypeReceiveOnly cfg.Folders = []config.FolderConfiguration{fcfg} - w.Replace(cfg) + replace(t, w, cfg) m := newModel(t, w, myID, "syncthing", "dev", nil) m.ServeBackground() @@ -464,7 +469,7 @@ func setupROFolder(t *testing.T) (*testModel, *receiveOnlyFolder) { defer m.fmut.RUnlock() f := m.folderRunners["ro"].(*receiveOnlyFolder) - return m, f + return m, f, cancel } func writeFile(fs fs.Filesystem, filename string, data []byte, perm fs.FileMode) error { diff --git a/lib/model/folder_sendrecv_test.go b/lib/model/folder_sendrecv_test.go index 2589ee643..53ab76390 100644 --- a/lib/model/folder_sendrecv_test.go +++ b/lib/model/folder_sendrecv_test.go @@ -92,8 +92,8 @@ func createFile(t *testing.T, name string, fs fs.Filesystem) protocol.FileInfo { } // Sets up a folder and model, but makes sure the services aren't actually running. -func setupSendReceiveFolder(t testing.TB, files ...protocol.FileInfo) (*testModel, *sendReceiveFolder) { - w, fcfg := tmpDefaultWrapper() +func setupSendReceiveFolder(t testing.TB, files ...protocol.FileInfo) (*testModel, *sendReceiveFolder, context.CancelFunc) { + w, fcfg, wCancel := tmpDefaultWrapper() // Initialise model and stop immediately. model := setupModel(t, w) model.cancel() @@ -107,10 +107,11 @@ func setupSendReceiveFolder(t testing.TB, files ...protocol.FileInfo) (*testMode f.updateLocalsFromScanning(files) } - return model, f + return model, f, wCancel } -func cleanupSRFolder(f *sendReceiveFolder, m *testModel) { +func cleanupSRFolder(f *sendReceiveFolder, m *testModel, wrapperCancel context.CancelFunc) { + wrapperCancel() os.Remove(m.cfg.ConfigPath()) os.RemoveAll(f.Filesystem().URI()) } @@ -130,8 +131,8 @@ func TestHandleFile(t *testing.T) { requiredFile := existingFile requiredFile.Blocks = blocks[1:] - m, f := setupSendReceiveFolder(t, existingFile) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t, existingFile) + defer cleanupSRFolder(f, m, wcfgCancel) copyChan := make(chan copyBlocksState, 1) @@ -172,8 +173,8 @@ func TestHandleFileWithTemp(t *testing.T) { requiredFile := existingFile requiredFile.Blocks = blocks[1:] - m, f := setupSendReceiveFolder(t, existingFile) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t, existingFile) + defer cleanupSRFolder(f, m, wcfgCancel) if _, err := prepareTmpFile(f.Filesystem()); err != nil { t.Fatal(err) @@ -228,10 +229,10 @@ func TestCopierFinder(t *testing.T) { requiredFile.Blocks = blocks[1:] requiredFile.Name = "file2" - m, f := setupSendReceiveFolder(t, existingFile) + m, f, wcfgCancel := setupSendReceiveFolder(t, existingFile) f.CopyRangeMethod = method - defer cleanupSRFolder(f, m) + defer cleanupSRFolder(f, m, wcfgCancel) if _, err := prepareTmpFile(f.Filesystem()); err != nil { t.Fatal(err) @@ -309,8 +310,8 @@ func TestCopierFinder(t *testing.T) { func TestWeakHash(t *testing.T) { // Setup the model/pull environment - model, fo := setupSendReceiveFolder(t) - defer cleanupSRFolder(fo, model) + model, fo, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(fo, model, wcfgCancel) ffs := fo.Filesystem() tempFile := fs.TempName("weakhash") @@ -438,8 +439,8 @@ func TestCopierCleanup(t *testing.T) { // Create a file file := setupFile("test", []int{0}) file.Size = 1 - m, f := setupSendReceiveFolder(t, file) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t, file) + defer cleanupSRFolder(f, m, wcfgCancel) file.Blocks = []protocol.BlockInfo{blocks[1]} file.Version = file.Version.Update(myID.Short()) @@ -471,8 +472,8 @@ func TestCopierCleanup(t *testing.T) { func TestDeregisterOnFailInCopy(t *testing.T) { file := setupFile("filex", []int{0, 2, 0, 0, 5, 0, 0, 8}) - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) // Set up our evet subscription early s := m.evLogger.Subscribe(events.ItemFinished) @@ -571,8 +572,8 @@ func TestDeregisterOnFailInCopy(t *testing.T) { func TestDeregisterOnFailInPull(t *testing.T) { file := setupFile("filex", []int{0, 2, 0, 0, 5, 0, 0, 8}) - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) // Set up our evet subscription early s := m.evLogger.Subscribe(events.ItemFinished) @@ -674,8 +675,8 @@ func TestDeregisterOnFailInPull(t *testing.T) { } func TestIssue3164(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() tmpDir := ffs.URI() @@ -765,8 +766,8 @@ func TestDiffEmpty(t *testing.T) { // option is true and the permissions do not match between the file on disk and // in the db. func TestDeleteIgnorePerms(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() f.IgnorePerms = true @@ -803,8 +804,8 @@ func TestCopyOwner(t *testing.T) { // Set up a folder with the CopyParentOwner bit and backed by a fake // filesystem. - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) f.folder.FolderConfiguration = config.NewFolderConfiguration(m.id, f.ID, f.Label, fs.FilesystemTypeFake, "/TestCopyOwner") f.folder.FolderConfiguration.CopyOwnershipFromParent = true @@ -906,8 +907,8 @@ func TestCopyOwner(t *testing.T) { // TestSRConflictReplaceFileByDir checks that a conflict is created when an existing file // is replaced with a directory and versions are conflicting func TestSRConflictReplaceFileByDir(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() name := "foo" @@ -938,8 +939,8 @@ func TestSRConflictReplaceFileByDir(t *testing.T) { // TestSRConflictReplaceFileByLink checks that a conflict is created when an existing file // is replaced with a link and versions are conflicting func TestSRConflictReplaceFileByLink(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() name := "foo" @@ -971,8 +972,8 @@ func TestSRConflictReplaceFileByLink(t *testing.T) { // TestDeleteBehindSymlink checks that we don't delete or schedule a scan // when trying to delete a file behind a symlink. func TestDeleteBehindSymlink(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() destDir := createTmpDir() @@ -1022,8 +1023,8 @@ func TestDeleteBehindSymlink(t *testing.T) { // Reproduces https://github.com/syncthing/syncthing/issues/6559 func TestPullCtxCancel(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) pullChan := make(chan pullBlockState) finisherChan := make(chan *sharedPullerState) @@ -1064,8 +1065,8 @@ func TestPullCtxCancel(t *testing.T) { } func TestPullDeleteUnscannedDir(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() dir := "foobar" @@ -1093,8 +1094,8 @@ func TestPullDeleteUnscannedDir(t *testing.T) { } func TestPullCaseOnlyPerformFinish(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() name := "foo" @@ -1154,8 +1155,8 @@ func TestPullCaseOnlySymlink(t *testing.T) { } func testPullCaseOnlyDirOrSymlink(t *testing.T, dir bool) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) ffs := f.Filesystem() name := "foo" @@ -1209,8 +1210,8 @@ func testPullCaseOnlyDirOrSymlink(t *testing.T, dir bool) { } func TestPullTempFileCaseConflict(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) copyChan := make(chan copyBlocksState, 1) @@ -1235,8 +1236,8 @@ func TestPullTempFileCaseConflict(t *testing.T) { } func TestPullCaseOnlyRename(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) // tempNameConfl := fs.TempName(confl) @@ -1278,8 +1279,8 @@ func TestPullSymlinkOverExistingWindows(t *testing.T) { t.Skip() } - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) name := "foo" if fd, err := f.mtimefs.Create(name); err != nil { @@ -1318,8 +1319,8 @@ func TestPullSymlinkOverExistingWindows(t *testing.T) { } func TestPullDeleteCaseConflict(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) name := "foo" fi := protocol.FileInfo{Name: "Foo"} @@ -1352,8 +1353,8 @@ func TestPullDeleteCaseConflict(t *testing.T) { } func TestPullDeleteIgnoreChildDir(t *testing.T) { - m, f := setupSendReceiveFolder(t) - defer cleanupSRFolder(f, m) + m, f, wcfgCancel := setupSendReceiveFolder(t) + defer cleanupSRFolder(f, m, wcfgCancel) parent := "parent" del := "ignored" diff --git a/lib/model/model.go b/lib/model/model.go index 458380c6c..9f82a9067 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -136,6 +136,7 @@ type model struct { // such as scans and pulls. folderIOLimiter *byteSemaphore fatalChan chan error + started chan struct{} // fields protected by fmut fmut sync.RWMutex @@ -162,7 +163,6 @@ type model struct { // for testing only foldersRunning int32 - started chan struct{} } type folderFactory func(*model, *db.FileSet, *ignore.Matcher, config.FolderConfiguration, versioner.Versioner, events.Logger, *byteSemaphore) service @@ -221,6 +221,7 @@ func NewModel(cfg config.Wrapper, id protocol.DeviceID, clientName, clientVersio globalRequestLimiter: newByteSemaphore(1024 * cfg.Options().MaxConcurrentIncomingRequestKiB()), folderIOLimiter: newByteSemaphore(cfg.Options().MaxFolderConcurrency()), fatalChan: make(chan error), + started: make(chan struct{}), // fields protected by fmut fmut: sync.NewRWMutex(), @@ -243,9 +244,6 @@ func NewModel(cfg config.Wrapper, id protocol.DeviceID, clientName, clientVersio deviceDownloads: make(map[protocol.DeviceID]*deviceDownloadState), remotePausedFolders: make(map[protocol.DeviceID]map[string]struct{}), indexSenders: make(map[protocol.DeviceID]*indexSenderRegistry), - - // for testing only - started: make(chan struct{}), } for devID := range cfg.Devices() { m.deviceStatRefs[devID] = stats.NewDeviceStatisticsReference(m.db, devID) @@ -259,10 +257,10 @@ func NewModel(cfg config.Wrapper, id protocol.DeviceID, clientName, clientVersio func (m *model) serve(ctx context.Context) error { defer m.closeAllConnectionsAndWait() - m.cfg.Subscribe(m) + cfg := m.cfg.Subscribe(m) defer m.cfg.Unsubscribe(m) - if err := m.initFolders(); err != nil { + if err := m.initFolders(cfg); err != nil { close(m.started) return svcutil.AsFatalErr(err, svcutil.ExitError) } @@ -277,17 +275,14 @@ func (m *model) serve(ctx context.Context) error { } } -func (m *model) initFolders() error { - cacheIgnoredFiles := m.cfg.Options().CacheIgnoredFiles - existingDevices := m.cfg.Devices() - existingFolders := m.cfg.Folders() - clusterConfigDevices := make(deviceIDSet, len(existingDevices)) - for _, folderCfg := range existingFolders { +func (m *model) initFolders(cfg config.Configuration) error { + clusterConfigDevices := make(deviceIDSet, len(cfg.Devices)) + for _, folderCfg := range cfg.Folders { if folderCfg.Paused { folderCfg.CreateRoot() continue } - err := m.newFolder(folderCfg, cacheIgnoredFiles) + err := m.newFolder(folderCfg, cfg.Options.CacheIgnoredFiles) if err != nil { return err } @@ -295,7 +290,7 @@ func (m *model) initFolders() error { } ignoredDevices := observedDeviceSet(m.cfg.IgnoredDevices()) - m.cleanPending(existingDevices, existingFolders, ignoredDevices, nil) + m.cleanPending(cfg.DeviceMap(), cfg.FolderMap(), ignoredDevices, nil) m.resendClusterConfig(clusterConfigDevices.AsSlice()) return nil @@ -1184,7 +1179,6 @@ func (m *model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterCon panic("bug: ClusterConfig called on closed or nonexistent connection") } - changed := false deviceCfg, ok := m.cfg.Device(deviceID) if !ok { l.Debugln("Device disappeared from config while processing cluster-config") @@ -1219,21 +1213,33 @@ func (m *model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterCon // Needs to happen outside of the fmut, as can cause CommitConfiguration if deviceCfg.AutoAcceptFolders { - changedFolders := make([]config.FolderConfiguration, 0, len(cm.Folders)) - for _, folder := range cm.Folders { - if fcfg, fchanged := m.handleAutoAccepts(deviceID, folder, ccDeviceInfos[folder.ID]); fchanged { - changedFolders = append(changedFolders, fcfg) + w, _ := m.cfg.Modify(func(cfg *config.Configuration) { + changedFcfg := make(map[string]config.FolderConfiguration) + haveFcfg := cfg.FolderMap() + for _, folder := range cm.Folders { + from, ok := haveFcfg[folder.ID] + if to, changed := m.handleAutoAccepts(deviceID, folder, ccDeviceInfos[folder.ID], from, ok, cfg.Options.DefaultFolderPath); changed { + changedFcfg[folder.ID] = to + } } - } - if len(changedFolders) > 0 { - // Need to wait for the waiter, as this calls CommitConfiguration, - // which sets up the folder and as we return from this call, - // ClusterConfig starts poking at m.folderFiles and other things - // that might not exist until the config is committed. - w, _ := m.cfg.SetFolders(changedFolders) - w.Wait() - changed = true - } + if len(changedFcfg) == 0 { + return + } + for i := range cfg.Folders { + if fcfg, ok := changedFcfg[cfg.Folders[i].ID]; ok { + cfg.Folders[i] = fcfg + delete(changedFcfg, cfg.Folders[i].ID) + } + } + for _, fcfg := range changedFcfg { + cfg.Folders = append(cfg.Folders, fcfg) + } + }) + // Need to wait for the waiter, as this calls CommitConfiguration, + // which sets up the folder and as we return from this call, + // ClusterConfig starts poking at m.folderFiles and other things + // that might not exist until the config is committed. + w.Wait() } tempIndexFolders, paused, err := m.ccHandleFolders(cm.Folders, deviceCfg, ccDeviceInfos, indexSenderRegistry) @@ -1257,11 +1263,12 @@ func (m *model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterCon } if deviceCfg.Introducer { - folders, devices, foldersDevices, introduced := m.handleIntroductions(deviceCfg, cm) - folders, devices, deintroduced := m.handleDeintroductions(deviceCfg, foldersDevices, folders, devices) - if introduced || deintroduced { - changed = true - cfg := m.cfg.RawCopy() + m.cfg.Modify(func(cfg *config.Configuration) { + folders, devices, foldersDevices, introduced := m.handleIntroductions(deviceCfg, cm, cfg.FolderMap(), cfg.DeviceMap()) + folders, devices, deintroduced := m.handleDeintroductions(deviceCfg, foldersDevices, folders, devices) + if !introduced && !deintroduced { + return + } cfg.Folders = make([]config.FolderConfiguration, 0, len(folders)) for _, fcfg := range folders { cfg.Folders = append(cfg.Folders, fcfg) @@ -1270,14 +1277,7 @@ func (m *model) ClusterConfig(deviceID protocol.DeviceID, cm protocol.ClusterCon for _, dcfg := range devices { cfg.Devices = append(cfg.Devices, dcfg) } - m.cfg.Replace(cfg) - } - } - - if changed { - if err := m.cfg.Save(); err != nil { - l.Warnln("Failed to save config", err) - } + }) } return nil @@ -1492,10 +1492,8 @@ func (m *model) resendClusterConfig(ids []protocol.DeviceID) { } // handleIntroductions handles adding devices/folders that are shared by an introducer device -func (m *model) handleIntroductions(introducerCfg config.DeviceConfiguration, cm protocol.ClusterConfig) (map[string]config.FolderConfiguration, map[protocol.DeviceID]config.DeviceConfiguration, folderDeviceSet, bool) { +func (m *model) handleIntroductions(introducerCfg config.DeviceConfiguration, cm protocol.ClusterConfig, folders map[string]config.FolderConfiguration, devices map[protocol.DeviceID]config.DeviceConfiguration) (map[string]config.FolderConfiguration, map[protocol.DeviceID]config.DeviceConfiguration, folderDeviceSet, bool) { changed := false - folders := m.cfg.Folders() - devices := m.cfg.Devices() foldersDevices := make(folderDeviceSet) @@ -1521,7 +1519,7 @@ func (m *model) handleIntroductions(introducerCfg config.DeviceConfiguration, cm foldersDevices.set(device.ID, folder.ID) - if _, ok := m.cfg.Device(device.ID); !ok { + if _, ok := devices[device.ID]; !ok { // The device is currently unknown. Add it to the config. devices[device.ID] = m.introduceDevice(device, introducerCfg) } else if fcfg.SharedWith(device.ID) { @@ -1602,9 +1600,8 @@ func (m *model) handleDeintroductions(introducerCfg config.DeviceConfiguration, // handleAutoAccepts handles adding and sharing folders for devices that have // AutoAcceptFolders set to true. -func (m *model) handleAutoAccepts(deviceID protocol.DeviceID, folder protocol.Folder, ccDeviceInfos *indexSenderStartInfo) (config.FolderConfiguration, bool) { - if cfg, ok := m.cfg.Folder(folder.ID); !ok { - defaultPath := m.cfg.Options().DefaultFolderPath +func (m *model) handleAutoAccepts(deviceID protocol.DeviceID, folder protocol.Folder, ccDeviceInfos *indexSenderStartInfo, cfg config.FolderConfiguration, haveCfg bool, defaultPath string) (config.FolderConfiguration, bool) { + if !haveCfg { defaultPathFs := fs.NewFilesystem(fs.FilesystemTypeBasic, defaultPath) pathAlternatives := []string{ fs.SanitizePath(folder.Label), @@ -2179,9 +2176,16 @@ func (m *model) AddConnection(conn protocol.Connection, hello protocol.Hello) { conn.ClusterConfig(cm) if (device.Name == "" || m.cfg.Options().OverwriteRemoteDevNames) && hello.DeviceName != "" { - device.Name = hello.DeviceName - m.cfg.SetDevice(device) - m.cfg.Save() + m.cfg.Modify(func(cfg *config.Configuration) { + for i := range cfg.Devices { + if cfg.Devices[i].DeviceID == deviceID { + if cfg.Devices[i].Name == "" || cfg.Options.OverwriteRemoteDevNames { + cfg.Devices[i].Name = hello.DeviceName + } + return + } + } + }) } m.deviceWasSeen(deviceID) @@ -2651,6 +2655,9 @@ func (m *model) VerifyConfiguration(from, to config.Configuration) error { 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. + // Delay processing config changes until after the initial setup + <-m.started + // Go through the folder configs and figure out if we need to restart or not. // Tracks devices affected by any configuration change to resend ClusterConfig. diff --git a/lib/model/model_test.go b/lib/model/model_test.go index ff33eab98..26969449a 100644 --- a/lib/model/model_test.go +++ b/lib/model/model_test.go @@ -79,6 +79,7 @@ func TestMain(m *testing.M) { exitCode := m.Run() + defaultCfgWrapperCancel() os.Remove(defaultCfgWrapper.ConfigPath()) defaultFs.Remove(tmpName) defaultFs.RemoveAll(config.DefaultMarkerName) @@ -108,18 +109,8 @@ func prepareTmpFile(to fs.Filesystem) (string, error) { return tmpName, nil } -func createTmpWrapper(cfg config.Configuration) config.Wrapper { - tmpFile, err := ioutil.TempFile("", "syncthing-testConfig-") - if err != nil { - panic(err) - } - wrapper := config.Wrap(tmpFile.Name(), cfg, myID, events.NoopLogger) - tmpFile.Close() - return wrapper -} - -func newState(t testing.TB, cfg config.Configuration) *testModel { - wcfg := createTmpWrapper(cfg) +func newState(t testing.TB, cfg config.Configuration) (*testModel, context.CancelFunc) { + wcfg, cancel := createTmpWrapper(cfg) m := setupModel(t, wcfg) @@ -127,7 +118,7 @@ func newState(t testing.TB, cfg config.Configuration) *testModel { m.AddConnection(&fakeConnection{id: dev.DeviceID, model: m}, protocol.Hello{}) } - return m + return m, cancel } func createClusterConfig(remote protocol.DeviceID, ids ...string) protocol.ClusterConfig { @@ -321,7 +312,6 @@ func TestDeviceRename(t *testing.T) { ClientName: "syncthing", ClientVersion: "v0.9.4", } - defer func() { mustRemove(t, defaultFs.Remove("tmpconfig.xml")) }() rawCfg := config.New(device1) rawCfg.Devices = []config.DeviceConfiguration{ @@ -329,7 +319,8 @@ func TestDeviceRename(t *testing.T) { DeviceID: device1, }, } - cfg := config.Wrap("testdata/tmpconfig.xml", rawCfg, device1, events.NoopLogger) + cfg, cfgCancel := createTmpWrapper(rawCfg) + defer cfgCancel() m := newModel(t, cfg, myID, "syncthing", "dev", nil) @@ -364,7 +355,8 @@ func TestDeviceRename(t *testing.T) { t.Errorf("Device name got overwritten") } - cfgw, _, err := config.Load("testdata/tmpconfig.xml", myID, events.NoopLogger) + must(t, cfg.Save()) + cfgw, _, err := config.Load(cfg.ConfigPath(), myID, events.NoopLogger) if err != nil { t.Error(err) return @@ -375,9 +367,11 @@ func TestDeviceRename(t *testing.T) { m.Closed(conn, protocol.ErrTimeout) - opts := cfg.Options() - opts.OverwriteRemoteDevNames = true - cfg.SetOptions(opts) + waiter, err := cfg.Modify(func(cfg *config.Configuration) { + cfg.Options.OverwriteRemoteDevNames = true + }) + must(t, err) + waiter.Wait() hello.DeviceName = "tester2" m.AddConnection(conn, hello) @@ -426,7 +420,8 @@ func TestClusterConfig(t *testing.T) { }, } - wrapper := createTmpWrapper(cfg) + wrapper, cancel := createTmpWrapper(cfg) + defer cancel() m := newModel(t, wrapper, myID, "syncthing", "dev", nil) m.ServeBackground() defer cleanupModel(m) @@ -494,7 +489,7 @@ func TestIntroducer(t *testing.T) { return false } - m := newState(t, config.Configuration{ + m, cancel := newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -535,7 +530,8 @@ func TestIntroducer(t *testing.T) { } cleanupModel(m) - m = newState(t, config.Configuration{ + cancel() + m, cancel = newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -586,7 +582,8 @@ func TestIntroducer(t *testing.T) { } cleanupModel(m) - m = newState(t, config.Configuration{ + cancel() + m, cancel = newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -634,7 +631,8 @@ func TestIntroducer(t *testing.T) { // 1. Introducer flag no longer set on device cleanupModel(m) - m = newState(t, config.Configuration{ + cancel() + m, cancel = newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -681,7 +679,8 @@ func TestIntroducer(t *testing.T) { // 2. SkipIntroductionRemovals is set cleanupModel(m) - m = newState(t, config.Configuration{ + cancel() + m, cancel = newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -734,7 +733,8 @@ func TestIntroducer(t *testing.T) { // Test device not being removed as it's shared without an introducer. cleanupModel(m) - m = newState(t, config.Configuration{ + cancel() + m, cancel = newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -781,7 +781,8 @@ func TestIntroducer(t *testing.T) { // Test device not being removed as it's shared by a different introducer. cleanupModel(m) - m = newState(t, config.Configuration{ + cancel() + m, cancel = newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -812,6 +813,7 @@ func TestIntroducer(t *testing.T) { }, }) defer cleanupModel(m) + defer cancel() m.ClusterConfig(device1, protocol.ClusterConfig{}) if _, ok := m.cfg.Device(device2); !ok { @@ -828,7 +830,7 @@ func TestIntroducer(t *testing.T) { } func TestIssue4897(t *testing.T) { - m := newState(t, config.Configuration{ + m, cancel := newState(t, config.Configuration{ Devices: []config.DeviceConfiguration{ { DeviceID: device1, @@ -847,6 +849,7 @@ func TestIssue4897(t *testing.T) { }, }) defer cleanupModel(m) + cancel() cm := m.generateClusterConfig(device1) if l := len(cm.Folders); l != 1 { @@ -860,8 +863,9 @@ func TestIssue4897(t *testing.T) { // PR-comments: https://github.com/syncthing/syncthing/pull/5069/files#r203146546 // Issue: https://github.com/syncthing/syncthing/pull/5509 func TestIssue5063(t *testing.T) { - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) defer cleanupModel(m) + defer cancel() m.pmut.Lock() for _, c := range m.conn { @@ -915,8 +919,9 @@ func TestAutoAcceptRejected(t *testing.T) { for i := range tcfg.Devices { tcfg.Devices[i].AutoAcceptFolders = false } - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() id := srand.String(8) defer os.RemoveAll(id) m.ClusterConfig(device1, createClusterConfig(device1, id)) @@ -928,8 +933,9 @@ func TestAutoAcceptRejected(t *testing.T) { func TestAutoAcceptNewFolder(t *testing.T) { // New folder - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) defer cleanupModel(m) + defer cancel() id := srand.String(8) defer os.RemoveAll(id) m.ClusterConfig(device1, createClusterConfig(device1, id)) @@ -939,8 +945,9 @@ func TestAutoAcceptNewFolder(t *testing.T) { } func TestAutoAcceptNewFolderFromTwoDevices(t *testing.T) { - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) defer cleanupModel(m) + defer cancel() id := srand.String(8) defer os.RemoveAll(id) m.ClusterConfig(device1, createClusterConfig(device1, id)) @@ -959,10 +966,11 @@ func TestAutoAcceptNewFolderFromTwoDevices(t *testing.T) { func TestAutoAcceptNewFolderFromOnlyOneDevice(t *testing.T) { modifiedCfg := defaultAutoAcceptCfg.Copy() modifiedCfg.Devices[2].AutoAcceptFolders = false - m := newState(t, modifiedCfg) + m, cancel := newState(t, modifiedCfg) id := srand.String(8) defer os.RemoveAll(id) defer cleanupModel(m) + defer cancel() m.ClusterConfig(device1, createClusterConfig(device1, id)) if fcfg, ok := m.cfg.Folder(id); !ok || !fcfg.SharedWith(device1) { t.Error("expected shared", id) @@ -1002,7 +1010,7 @@ func TestAutoAcceptNewFolderPremutationsNoPanic(t *testing.T) { fcfg.Paused = localFolderPaused cfg.Folders = append(cfg.Folders, fcfg) } - m := newState(t, cfg) + m, cancel := newState(t, cfg) m.ClusterConfig(device1, protocol.ClusterConfig{ Folders: []protocol.Folder{dev1folder}, }) @@ -1010,6 +1018,7 @@ func TestAutoAcceptNewFolderPremutationsNoPanic(t *testing.T) { Folders: []protocol.Folder{dev2folder}, }) cleanupModel(m) + cancel() testOs.RemoveAll(id) testOs.RemoveAll(label) } @@ -1024,8 +1033,9 @@ func TestAutoAcceptMultipleFolders(t *testing.T) { defer os.RemoveAll(id1) id2 := srand.String(8) defer os.RemoveAll(id2) - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) defer cleanupModel(m) + defer cancel() m.ClusterConfig(device1, createClusterConfig(device1, id1, id2)) if fcfg, ok := m.cfg.Folder(id1); !ok || !fcfg.SharedWith(device1) { t.Error("expected shared", id1) @@ -1049,8 +1059,9 @@ func TestAutoAcceptExistingFolder(t *testing.T) { Path: idOther, // To check that path does not get changed. }, } - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() if fcfg, ok := m.cfg.Folder(id); !ok || fcfg.SharedWith(device1) { t.Error("missing folder, or shared", id) } @@ -1075,8 +1086,9 @@ func TestAutoAcceptNewAndExistingFolder(t *testing.T) { Path: id1, // from previous test case, to verify that path doesn't get changed. }, } - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() if fcfg, ok := m.cfg.Folder(id1); !ok || fcfg.SharedWith(device1) { t.Error("missing folder, or shared", id1) } @@ -1105,8 +1117,9 @@ func TestAutoAcceptAlreadyShared(t *testing.T) { }, }, } - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() if fcfg, ok := m.cfg.Folder(id); !ok || !fcfg.SharedWith(device1) { t.Error("missing folder, or not shared", id) } @@ -1126,8 +1139,9 @@ func TestAutoAcceptNameConflict(t *testing.T) { testOs.MkdirAll(label, 0777) defer os.RemoveAll(id) defer os.RemoveAll(label) - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) defer cleanupModel(m) + defer cancel() m.ClusterConfig(device1, protocol.ClusterConfig{ Folders: []protocol.Folder{ { @@ -1143,12 +1157,13 @@ func TestAutoAcceptNameConflict(t *testing.T) { func TestAutoAcceptPrefersLabel(t *testing.T) { // Prefers label, falls back to ID. - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) id := srand.String(8) label := srand.String(8) defer os.RemoveAll(id) defer os.RemoveAll(label) defer cleanupModel(m) + defer cancel() m.ClusterConfig(device1, addFolderDevicesToClusterConfig(protocol.ClusterConfig{ Folders: []protocol.Folder{ { @@ -1166,7 +1181,7 @@ func TestAutoAcceptFallsBackToID(t *testing.T) { testOs := &fatalOs{t} // Prefers label, falls back to ID. - m := newState(t, defaultAutoAcceptCfg) + m, cancel := newState(t, defaultAutoAcceptCfg) id := srand.String(8) label := srand.String(8) t.Log(id, label) @@ -1174,6 +1189,7 @@ func TestAutoAcceptFallsBackToID(t *testing.T) { defer os.RemoveAll(label) defer os.RemoveAll(id) defer cleanupModel(m) + defer cancel() m.ClusterConfig(device1, addFolderDevicesToClusterConfig(protocol.ClusterConfig{ Folders: []protocol.Folder{ { @@ -1204,8 +1220,9 @@ func TestAutoAcceptPausedWhenFolderConfigChanged(t *testing.T) { DeviceID: device1, }) tcfg.Folders = []config.FolderConfiguration{fcfg} - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() if fcfg, ok := m.cfg.Folder(id); !ok || !fcfg.SharedWith(device1) { t.Error("missing folder, or not shared", id) } @@ -1254,8 +1271,9 @@ func TestAutoAcceptPausedWhenFolderConfigNotChanged(t *testing.T) { }, }, fcfg.Devices...) // Need to ensure this device order to avoid folder restart. tcfg.Folders = []config.FolderConfiguration{fcfg} - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() if fcfg, ok := m.cfg.Folder(id); !ok || !fcfg.SharedWith(device1) { t.Error("missing folder, or not shared", id) } @@ -1286,8 +1304,9 @@ func TestAutoAcceptPausedWhenFolderConfigNotChanged(t *testing.T) { func TestAutoAcceptEnc(t *testing.T) { tcfg := defaultAutoAcceptCfg.Copy() - m := newState(t, tcfg) + m, cancel := newState(t, tcfg) defer cleanupModel(m) + defer cancel() id := srand.String(8) defer os.RemoveAll(id) @@ -1577,7 +1596,7 @@ func TestROScanRecovery(t *testing.T) { RescanIntervalS: 1, MarkerName: config.DefaultMarkerName, } - cfg := createTmpWrapper(config.Configuration{ + cfg, cancel := createTmpWrapper(config.Configuration{ Folders: []config.FolderConfiguration{fcfg}, Devices: []config.DeviceConfiguration{ { @@ -1585,6 +1604,7 @@ func TestROScanRecovery(t *testing.T) { }, }, }) + defer cancel() m := newModel(t, cfg, myID, "syncthing", "dev", nil) set := newFileSet(t, "default", defaultFs, m.db) @@ -1629,7 +1649,7 @@ func TestRWScanRecovery(t *testing.T) { RescanIntervalS: 1, MarkerName: config.DefaultMarkerName, } - cfg := createTmpWrapper(config.Configuration{ + cfg, cancel := createTmpWrapper(config.Configuration{ Folders: []config.FolderConfiguration{fcfg}, Devices: []config.DeviceConfiguration{ { @@ -1637,6 +1657,7 @@ func TestRWScanRecovery(t *testing.T) { }, }, }) + defer cancel() m := newModel(t, cfg, myID, "syncthing", "dev", nil) testOs.RemoveAll(fcfg.Path) @@ -1672,7 +1693,8 @@ func TestRWScanRecovery(t *testing.T) { } func TestGlobalDirectoryTree(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() m := setupModel(t, w) defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) @@ -1922,7 +1944,8 @@ func TestGlobalDirectoryTree(t *testing.T) { } func TestGlobalDirectorySelfFixing(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() m := setupModel(t, w) defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) @@ -2160,17 +2183,14 @@ func TestIssue3028(t *testing.T) { func TestIssue4357(t *testing.T) { cfg := defaultCfgWrapper.RawCopy() // Create a separate wrapper not to pollute other tests. - wrapper := createTmpWrapper(config.Configuration{}) + wrapper, cancel := createTmpWrapper(config.Configuration{}) + defer cancel() m := newModel(t, wrapper, myID, "syncthing", "dev", nil) m.ServeBackground() defer cleanupModel(m) // Force the model to wire itself and add the folders - p, err := wrapper.Replace(cfg) - p.Wait() - if err != nil { - t.Error(err) - } + replace(t, wrapper, cfg) if _, ok := m.folderCfgs["default"]; !ok { t.Error("Folder should be running") @@ -2179,11 +2199,7 @@ func TestIssue4357(t *testing.T) { newCfg := wrapper.RawCopy() newCfg.Folders[0].Paused = true - p, err = wrapper.Replace(newCfg) - p.Wait() - if err != nil { - t.Error(err) - } + replace(t, wrapper, newCfg) if _, ok := m.folderCfgs["default"]; ok { t.Error("Folder should not be running") @@ -2193,22 +2209,14 @@ func TestIssue4357(t *testing.T) { t.Error("should still have folder in config") } - p, err = wrapper.Replace(config.Configuration{}) - p.Wait() - if err != nil { - t.Error(err) - } + replace(t, wrapper, config.Configuration{}) if _, ok := m.cfg.Folder("default"); ok { t.Error("should not have folder in config") } // Add the folder back, should be running - p, err = wrapper.Replace(cfg) - p.Wait() - if err != nil { - t.Error(err) - } + replace(t, wrapper, cfg) if _, ok := m.folderCfgs["default"]; !ok { t.Error("Folder should be running") @@ -2218,11 +2226,7 @@ func TestIssue4357(t *testing.T) { } // Should not panic when removing a running folder. - p, err = wrapper.Replace(config.Configuration{}) - p.Wait() - if err != nil { - t.Error(err) - } + replace(t, wrapper, config.Configuration{}) if _, ok := m.folderCfgs["default"]; ok { t.Error("Folder should not be running") @@ -2304,11 +2308,10 @@ func TestIndexesForUnknownDevicesDropped(t *testing.T) { } func TestSharedWithClearedOnDisconnect(t *testing.T) { - wcfg := createTmpWrapper(defaultCfg) - wcfg.SetDevice(config.NewDeviceConfiguration(device2, "device2")) - fcfg := wcfg.FolderList()[0] - fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{DeviceID: device2}) - wcfg.SetFolder(fcfg) + wcfg, cancel := createTmpWrapper(defaultCfg) + defer cancel() + addDevice2(t, wcfg, wcfg.FolderList()[0]) + defer os.Remove(wcfg.ConfigPath()) m := setupModel(t, wcfg) @@ -2500,11 +2503,9 @@ func TestIssue3829(t *testing.T) { func TestNoRequestsFromPausedDevices(t *testing.T) { t.Skip("broken, fails randomly, #3843") - wcfg := createTmpWrapper(defaultCfg) - wcfg.SetDevice(config.NewDeviceConfiguration(device2, "device2")) - fcfg := wcfg.FolderList()[0] - fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{DeviceID: device2}) - wcfg.SetFolder(fcfg) + wcfg, cancel := createTmpWrapper(defaultCfg) + defer cancel() + addDevice2(t, wcfg, wcfg.FolderList()[0]) m := setupModel(t, wcfg) defer cleanupModel(m) @@ -2579,7 +2580,8 @@ func TestIssue2571(t *testing.T) { t.Skip("Scanning symlinks isn't supported on windows") } - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() testFs := fcfg.Filesystem() defer os.RemoveAll(testFs.URI()) @@ -2617,7 +2619,8 @@ func TestIssue4573(t *testing.T) { t.Skip("Can't make the dir inaccessible on windows") } - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() testFs := fcfg.Filesystem() defer os.RemoveAll(testFs.URI()) @@ -2646,7 +2649,8 @@ func TestIssue4573(t *testing.T) { // TestInternalScan checks whether various fs operations are correctly represented // in the db after scanning. func TestInternalScan(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() testFs := fcfg.Filesystem() defer os.RemoveAll(testFs.URI()) @@ -2710,7 +2714,7 @@ func TestCustomMarkerName(t *testing.T) { fcfg.ID = "default" fcfg.RescanIntervalS = 1 fcfg.MarkerName = "myfile" - cfg := createTmpWrapper(config.Configuration{ + cfg, cancel := createTmpWrapper(config.Configuration{ Folders: []config.FolderConfiguration{fcfg}, Devices: []config.DeviceConfiguration{ { @@ -2718,6 +2722,7 @@ func TestCustomMarkerName(t *testing.T) { }, }, }) + defer cancel() testOs.RemoveAll(fcfg.Path) @@ -2803,7 +2808,8 @@ func TestRemoveDirWithContent(t *testing.T) { } func TestIssue4475(t *testing.T) { - m, conn, fcfg := setupModelWithConnection(t) + m, conn, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModel(m) testFs := fcfg.Filesystem() @@ -2873,7 +2879,8 @@ func TestVersionRestore(t *testing.T) { rawConfig := config.Configuration{ Folders: []config.FolderConfiguration{fcfg}, } - cfg := createTmpWrapper(rawConfig) + cfg, cancel := createTmpWrapper(rawConfig) + defer cancel() m := setupModel(t, cfg) defer cleanupModel(m) @@ -3052,7 +3059,8 @@ func TestVersionRestore(t *testing.T) { func TestPausedFolders(t *testing.T) { // Create a separate wrapper not to pollute other tests. - wrapper := createTmpWrapper(defaultCfgWrapper.RawCopy()) + wrapper, cancel := createTmpWrapper(defaultCfgWrapper.RawCopy()) + defer cancel() m := setupModel(t, wrapper) defer cleanupModel(m) @@ -3062,9 +3070,7 @@ func TestPausedFolders(t *testing.T) { pausedConfig := wrapper.RawCopy() pausedConfig.Folders[0].Paused = true - w, err := m.cfg.Replace(pausedConfig) - must(t, err) - w.Wait() + replace(t, wrapper, pausedConfig) if err := m.ScanFolder("default"); err != ErrFolderPaused { t.Errorf("Expected folder paused error, received: %v", err) @@ -3079,7 +3085,8 @@ func TestIssue4094(t *testing.T) { testOs := &fatalOs{t} // Create a separate wrapper not to pollute other tests. - wrapper := createTmpWrapper(config.Configuration{}) + wrapper, cancel := createTmpWrapper(config.Configuration{}) + defer cancel() m := newModel(t, wrapper, myID, "syncthing", "dev", nil) m.ServeBackground() defer cleanupModel(m) @@ -3097,9 +3104,7 @@ func TestIssue4094(t *testing.T) { }, } cfg.Folders = []config.FolderConfiguration{fcfg} - p, err := wrapper.Replace(cfg) - must(t, err) - p.Wait() + replace(t, wrapper, cfg) if err := m.SetIgnores(fcfg.ID, []string{"foo"}); err != nil { t.Fatalf("failed setting ignores: %v", err) @@ -3113,7 +3118,8 @@ func TestIssue4094(t *testing.T) { func TestIssue4903(t *testing.T) { testOs := &fatalOs{t} - wrapper := createTmpWrapper(config.Configuration{}) + wrapper, cancel := createTmpWrapper(config.Configuration{}) + defer cancel() m := setupModel(t, wrapper) defer cleanupModel(m) @@ -3130,9 +3136,7 @@ func TestIssue4903(t *testing.T) { }, } cfg.Folders = []config.FolderConfiguration{fcfg} - p, err := wrapper.Replace(cfg) - must(t, err) - p.Wait() + replace(t, wrapper, cfg) if err := fcfg.CheckPath(); err != config.ErrPathMissing { t.Fatalf("expected path missing error, got: %v", err) @@ -3165,8 +3169,9 @@ func TestIssue5002(t *testing.T) { } func TestParentOfUnignored(t *testing.T) { - m := newState(t, defaultCfg) + m, cancel := newState(t, defaultCfg) defer cleanupModel(m) + defer cancel() defer defaultFolderConfig.Filesystem().Remove(".stignore") m.SetIgnores("default", []string{"!quux", "*"}) @@ -3181,13 +3186,16 @@ func TestParentOfUnignored(t *testing.T) { // TestFolderRestartZombies reproduces issue 5233, where multiple concurrent folder // restarts would leave more than one folder runner alive. func TestFolderRestartZombies(t *testing.T) { - wrapper := createTmpWrapper(defaultCfg.Copy()) - opts := wrapper.Options() - opts.RawMaxFolderConcurrency = -1 - wrapper.SetOptions(opts) + wrapper, cancel := createTmpWrapper(defaultCfg.Copy()) + defer cancel() + waiter, err := wrapper.Modify(func(cfg *config.Configuration) { + cfg.Options.RawMaxFolderConcurrency = -1 + _, i, _ := cfg.Folder("default") + cfg.Folders[i].FilesystemType = fs.FilesystemTypeFake + }) + must(t, err) + waiter.Wait() folderCfg, _ := wrapper.Folder("default") - folderCfg.FilesystemType = fs.FilesystemTypeFake - wrapper.SetFolder(folderCfg) m := setupModel(t, wrapper) defer cleanupModel(m) @@ -3209,13 +3217,9 @@ func TestFolderRestartZombies(t *testing.T) { defer wg.Done() t0 := time.Now() for time.Since(t0) < time.Second { - cfg := folderCfg.Copy() - cfg.MaxConflicts = rand.Int() // safe change that should cause a folder restart - w, err := wrapper.SetFolder(cfg) - if err != nil { - panic(err) - } - w.Wait() + fcfg := folderCfg.Copy() + fcfg.MaxConflicts = rand.Int() // safe change that should cause a folder restart + setFolder(t, wrapper, fcfg) } }() } @@ -3231,10 +3235,14 @@ func TestFolderRestartZombies(t *testing.T) { } func TestRequestLimit(t *testing.T) { - wrapper := createTmpWrapper(defaultCfg.Copy()) - dev, _ := wrapper.Device(device1) - dev.MaxRequestKiB = 1 - wrapper.SetDevice(dev) + wrapper, cancel := createTmpWrapper(defaultCfg.Copy()) + defer cancel() + waiter, err := wrapper.Modify(func(cfg *config.Configuration) { + _, i, _ := cfg.Device(device1) + cfg.Devices[i].MaxRequestKiB = 1 + }) + must(t, err) + waiter.Wait() m, _ := setupModelWithConnectionFromWrapper(t, wrapper) defer cleanupModel(m) @@ -3278,7 +3286,8 @@ func TestConnCloseOnRestart(t *testing.T) { protocol.CloseTimeout = oldCloseTimeout }() - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() m := setupModel(t, w) defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) @@ -3314,10 +3323,11 @@ func TestConnCloseOnRestart(t *testing.T) { } func TestModTimeWindow(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() tfs := fcfg.Filesystem() fcfg.RawModTimeWindowS = 2 - w.SetFolder(fcfg) + setFolder(t, w, fcfg) m := setupModel(t, w) defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -3370,7 +3380,8 @@ func TestModTimeWindow(t *testing.T) { } func TestDevicePause(t *testing.T) { - m, _, fcfg := setupModelWithConnection(t) + m, _, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) sub := m.evLogger.Subscribe(events.DevicePaused) @@ -3380,9 +3391,7 @@ func TestDevicePause(t *testing.T) { closed := m.closed[device1] m.pmut.RUnlock() - dev := m.cfg.Devices()[device1] - dev.Paused = true - m.cfg.SetDevice(dev) + pauseDevice(t, m.cfg, device1, true) timeout := time.NewTimer(5 * time.Second) select { @@ -3398,7 +3407,8 @@ func TestDevicePause(t *testing.T) { } func TestDeviceWasSeen(t *testing.T) { - m, _, fcfg := setupModelWithConnection(t) + m, _, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) m.deviceWasSeen(device1) @@ -3442,9 +3452,9 @@ func TestNewLimitedRequestResponse(t *testing.T) { } func TestSummaryPausedNoError(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() - fcfg.Paused = true - wcfg.SetFolder(fcfg) + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() + pauseFolder(t, wcfg, fcfg.ID, true) m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3455,9 +3465,9 @@ func TestSummaryPausedNoError(t *testing.T) { } func TestFolderAPIErrors(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() - fcfg.Paused = true - wcfg.SetFolder(fcfg) + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() + pauseFolder(t, wcfg, fcfg.ID, true) m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3487,7 +3497,8 @@ func TestFolderAPIErrors(t *testing.T) { } func TestRenameSequenceOrder(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3557,7 +3568,8 @@ func TestRenameSequenceOrder(t *testing.T) { } func TestRenameSameFile(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3607,7 +3619,8 @@ func TestRenameSameFile(t *testing.T) { } func TestRenameEmptyFile(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3683,7 +3696,8 @@ func TestRenameEmptyFile(t *testing.T) { } func TestBlockListMap(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3750,7 +3764,8 @@ func TestBlockListMap(t *testing.T) { } func TestScanRenameCaseOnly(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModel(m) @@ -3801,7 +3816,7 @@ func TestScanRenameCaseOnly(t *testing.T) { } func TestClusterConfigOnFolderAdd(t *testing.T) { - testConfigChangeTriggersClusterConfigs(t, false, true, nil, func(cfg config.Wrapper) { + testConfigChangeTriggersClusterConfigs(t, false, true, nil, func(wrapper config.Wrapper) { fcfg := testFolderConfigTmp() fcfg.ID = "second" fcfg.Label = "second" @@ -3809,11 +3824,7 @@ func TestClusterConfigOnFolderAdd(t *testing.T) { DeviceID: device2, IntroducedBy: protocol.EmptyDeviceID, }} - if w, err := cfg.SetFolder(fcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + setFolder(t, wrapper, fcfg) }) } @@ -3824,11 +3835,7 @@ func TestClusterConfigOnFolderShare(t *testing.T) { DeviceID: device2, IntroducedBy: protocol.EmptyDeviceID, }} - if w, err := cfg.SetFolder(fcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + setFolder(t, cfg, fcfg) }) } @@ -3836,11 +3843,7 @@ func TestClusterConfigOnFolderUnshare(t *testing.T) { testConfigChangeTriggersClusterConfigs(t, true, false, nil, func(cfg config.Wrapper) { fcfg := cfg.FolderList()[0] fcfg.Devices = nil - if w, err := cfg.SetFolder(fcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + setFolder(t, cfg, fcfg) }) } @@ -3848,43 +3851,21 @@ func TestClusterConfigOnFolderRemove(t *testing.T) { testConfigChangeTriggersClusterConfigs(t, true, false, nil, func(cfg config.Wrapper) { rcfg := cfg.RawCopy() rcfg.Folders = nil - if w, err := cfg.Replace(rcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + replace(t, cfg, rcfg) }) } func TestClusterConfigOnFolderPause(t *testing.T) { testConfigChangeTriggersClusterConfigs(t, true, false, nil, func(cfg config.Wrapper) { - fcfg := cfg.FolderList()[0] - fcfg.Paused = true - if w, err := cfg.SetFolder(fcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + pauseFolder(t, cfg, cfg.FolderList()[0].ID, true) }) } func TestClusterConfigOnFolderUnpause(t *testing.T) { testConfigChangeTriggersClusterConfigs(t, true, false, func(cfg config.Wrapper) { - fcfg := cfg.FolderList()[0] - fcfg.Paused = true - if w, err := cfg.SetFolder(fcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + pauseFolder(t, cfg, cfg.FolderList()[0].ID, true) }, func(cfg config.Wrapper) { - fcfg := cfg.FolderList()[0] - fcfg.Paused = false - if w, err := cfg.SetFolder(fcfg); err != nil { - t.Fatal(err) - } else { - w.Wait() - } + pauseFolder(t, cfg, cfg.FolderList()[0].ID, false) }) } @@ -3905,10 +3886,10 @@ func TestAddFolderCompletion(t *testing.T) { } func TestScanDeletedROChangedOnSR(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() fcfg.Type = config.FolderTypeReceiveOnly - waiter, _ := w.SetFolder(fcfg) - waiter.Wait() + setFolder(t, w, fcfg) m := setupModel(t, w) defer cleanupModel(m) name := "foo" @@ -3933,8 +3914,7 @@ func TestScanDeletedROChangedOnSR(t *testing.T) { } fcfg.Type = config.FolderTypeSendReceive - waiter, _ = w.SetFolder(fcfg) - waiter.Wait() + setFolder(t, w, fcfg) m.ScanFolders() if receiveOnlyChangedSize(t, m, fcfg.ID).Deleted != 0 { @@ -3947,14 +3927,12 @@ func TestScanDeletedROChangedOnSR(t *testing.T) { func testConfigChangeTriggersClusterConfigs(t *testing.T, expectFirst, expectSecond bool, pre func(config.Wrapper), fn func(config.Wrapper)) { t.Helper() - wcfg, _ := tmpDefaultWrapper() + wcfg, _, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModel(m) - _, err := wcfg.SetDevice(config.NewDeviceConfiguration(device2, "device2")) - if err != nil { - t.Fatal(err) - } + setDevice(t, wcfg, config.NewDeviceConfiguration(device2, "device2")) if pre != nil { pre(wcfg) @@ -4017,16 +3995,20 @@ func testConfigChangeTriggersClusterConfigs(t *testing.T, expectFirst, expectSec // That then causes these files to be considered as needed, while they are not. // https://github.com/syncthing/syncthing/issues/6961 func TestIssue6961(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() tfs := fcfg.Filesystem() - wcfg.SetDevice(config.NewDeviceConfiguration(device2, "device2")) - fcfg.Type = config.FolderTypeReceiveOnly - fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{DeviceID: device2}) - wcfg.SetFolder(fcfg) + waiter, err := wcfg.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(config.NewDeviceConfiguration(device2, "device2")) + fcfg.Type = config.FolderTypeReceiveOnly + fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{DeviceID: device2}) + cfg.SetFolder(fcfg) + }) + must(t, err) + waiter.Wait() // Always recalc/repair when opening a fileset. m := newModel(t, wcfg, myID, "syncthing", "dev", nil) m.db.Close() - var err error m.db, err = db.NewLowlevel(backend.OpenMemory(), m.evLogger, db.WithRecheckInterval(time.Millisecond)) if err != nil { t.Fatal(err) @@ -4056,7 +4038,7 @@ func TestIssue6961(t *testing.T) { m.ScanFolders() // Get rid of valid global - waiter, err := wcfg.RemoveDevice(device1) + waiter, err = wcfg.RemoveDevice(device1) if err != nil { t.Fatal(err) } @@ -4070,18 +4052,8 @@ func TestIssue6961(t *testing.T) { m.Index(device2, fcfg.ID, []protocol.FileInfo{{Name: "bar", RawInvalid: true, Sequence: 1}}) // Pause and unpause folder to create new db.FileSet and thus recalculate everything - fcfg.Paused = true - waiter, err = wcfg.SetFolder(fcfg) - if err != nil { - t.Fatal(err) - } - waiter.Wait() - fcfg.Paused = false - waiter, err = wcfg.SetFolder(fcfg) - if err != nil { - t.Fatal(err) - } - waiter.Wait() + pauseFolder(t, wcfg, fcfg.ID, true) + pauseFolder(t, wcfg, fcfg.ID, false) if comp := m.Completion(device2, fcfg.ID); comp.NeedDeletes != 0 { t.Error("Expected 0 needed deletes, got", comp.NeedDeletes) @@ -4091,7 +4063,8 @@ func TestIssue6961(t *testing.T) { } func TestCompletionEmptyGlobal(t *testing.T) { - wcfg, fcfg := tmpDefaultWrapper() + wcfg, fcfg, wcfgCancel := tmpDefaultWrapper() + defer wcfgCancel() m := setupModel(t, wcfg) defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) files := []protocol.FileInfo{{Name: "foo", Version: protocol.Vector{}.Update(myID.Short()), Sequence: 1}} @@ -4108,12 +4081,9 @@ func TestCompletionEmptyGlobal(t *testing.T) { } func TestNeedMetaAfterIndexReset(t *testing.T) { - w, fcfg := tmpDefaultWrapper() - waiter, _ := w.SetDevice(config.NewDeviceConfiguration(device2, "device2")) - waiter.Wait() - fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{DeviceID: device2}) - waiter, _ = w.SetFolder(fcfg) - waiter.Wait() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() + addDevice2(t, w, fcfg) m := setupModel(t, w) defer cleanupModelAndRemoveDir(m, fcfg.Path) @@ -4148,7 +4118,8 @@ func TestNeedMetaAfterIndexReset(t *testing.T) { } func TestCcCheckEncryption(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() m := setupModel(t, w) m.cancel() defer cleanupModel(m) @@ -4288,7 +4259,8 @@ func TestCcCheckEncryption(t *testing.T) { func TestCCFolderNotRunning(t *testing.T) { // Create the folder, but don't start it. - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() tfs := fcfg.Filesystem() m := newModel(t, w, myID, "syncthing", "dev", nil) defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -4315,15 +4287,12 @@ func TestCCFolderNotRunning(t *testing.T) { } func TestPendingFolder(t *testing.T) { - w, _ := tmpDefaultWrapper() + w, _, wCancel := tmpDefaultWrapper() + defer wCancel() m := setupModel(t, w) defer cleanupModel(m) - waiter, err := w.SetDevice(config.DeviceConfiguration{DeviceID: device2}) - if err != nil { - t.Fatal(err) - } - waiter.Wait() + setDevice(t, w, config.DeviceConfiguration{DeviceID: device2}) pfolder := "default" if err := m.db.AddOrUpdatePendingFolder(pfolder, pfolder, device2); err != nil { t.Fatal(err) @@ -4340,11 +4309,7 @@ func TestPendingFolder(t *testing.T) { } device3, err := protocol.DeviceIDFromString("AIBAEAQ-CAIBAEC-AQCAIBA-EAQCAIA-BAEAQCA-IBAEAQC-CAIBAEA-QCAIBA7") - waiter, err = w.SetDevice(config.DeviceConfiguration{DeviceID: device3}) - if err != nil { - t.Fatal(err) - } - waiter.Wait() + setDevice(t, w, config.DeviceConfiguration{DeviceID: device3}) if err := m.db.AddOrUpdatePendingFolder(pfolder, pfolder, device3); err != nil { t.Fatal(err) } @@ -4359,7 +4324,7 @@ func TestPendingFolder(t *testing.T) { t.Errorf("folder %v pending for device %v, but not filtered out", pfolder, device3) } - waiter, err = w.RemoveDevice(device3) + waiter, err := w.RemoveDevice(device3) if err != nil { t.Fatal(err) } diff --git a/lib/model/progressemitter_test.go b/lib/model/progressemitter_test.go index c5c7ee5ba..77678c5c0 100644 --- a/lib/model/progressemitter_test.go +++ b/lib/model/progressemitter_test.go @@ -60,11 +60,16 @@ func TestProgressEmitter(t *testing.T) { w := evLogger.Subscribe(events.DownloadProgress) - c := createTmpWrapper(config.Configuration{}) + c, cfgCancel := createTmpWrapper(config.Configuration{}) defer os.Remove(c.ConfigPath()) - c.SetOptions(config.OptionsConfiguration{ - ProgressUpdateIntervalS: 60, // irrelevant, but must be positive + defer cfgCancel() + waiter, err := c.Modify(func(cfg *config.Configuration) { + cfg.Options.ProgressUpdateIntervalS = 60 // irrelevant, but must be positive }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() p := NewProgressEmitter(c, evLogger) go p.Serve(ctx) @@ -109,12 +114,17 @@ func TestProgressEmitter(t *testing.T) { } func TestSendDownloadProgressMessages(t *testing.T) { - c := createTmpWrapper(config.Configuration{}) + c, cfgCancel := createTmpWrapper(config.Configuration{}) defer os.Remove(c.ConfigPath()) - c.SetOptions(config.OptionsConfiguration{ - ProgressUpdateIntervalS: 60, // irrelevant, but must be positive - TempIndexMinBlocks: 10, + defer cfgCancel() + waiter, err := c.Modify(func(cfg *config.Configuration) { + cfg.Options.ProgressUpdateIntervalS = 60 // irrelevant, but must be positive + cfg.Options.TempIndexMinBlocks = 10 }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() fc := &fakeConnection{} diff --git a/lib/model/requests_test.go b/lib/model/requests_test.go index 6de463113..c096c3f32 100644 --- a/lib/model/requests_test.go +++ b/lib/model/requests_test.go @@ -29,7 +29,8 @@ func TestRequestSimple(t *testing.T) { // Verify that the model performs a request and creates a file based on // an incoming index update. - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -72,7 +73,8 @@ func TestSymlinkTraversalRead(t *testing.T) { return } - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) // We listen for incoming index updates and trigger when we see one for @@ -115,7 +117,8 @@ func TestSymlinkTraversalWrite(t *testing.T) { return } - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) // We listen for incoming index updates and trigger when we see one for @@ -174,7 +177,8 @@ func TestSymlinkTraversalWrite(t *testing.T) { func TestRequestCreateTmpSymlink(t *testing.T) { // Test that an update for a temporary file is invalidated - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) // We listen for incoming index updates and trigger when we see one for @@ -216,14 +220,15 @@ func TestRequestVersioningSymlinkAttack(t *testing.T) { // Sets up a folder with trashcan versioning and tries to use a // deleted symlink to escape - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() defer func() { os.RemoveAll(fcfg.Filesystem().URI()) os.Remove(w.ConfigPath()) }() fcfg.Versioning = config.VersioningConfiguration{Type: "trashcan"} - w.SetFolder(fcfg) + setFolder(t, w, fcfg) m, fc := setupModelWithConnectionFromWrapper(t, w) defer cleanupModel(m) @@ -293,11 +298,12 @@ func TestPullInvalidIgnoredSR(t *testing.T) { // This test checks that (un-)ignored/invalid/deleted files are treated as expected. func pullInvalidIgnored(t *testing.T, ft config.FolderType) { - w := createTmpWrapper(defaultCfgWrapper.RawCopy()) + w, wCancel := createTmpWrapper(defaultCfgWrapper.RawCopy()) + defer wCancel() fcfg := testFolderConfigTmp() fss := fcfg.Filesystem() fcfg.Type = ft - w.SetFolder(fcfg) + setFolder(t, w, fcfg) m := setupModel(t, w) defer cleanupModelAndRemoveDir(m, fss.URI()) @@ -420,7 +426,8 @@ func pullInvalidIgnored(t *testing.T, ft config.FolderType) { } func TestIssue4841(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) received := make(chan []protocol.FileInfo) @@ -464,7 +471,8 @@ func TestIssue4841(t *testing.T) { } func TestRescanIfHaveInvalidContent(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -530,7 +538,8 @@ func TestRescanIfHaveInvalidContent(t *testing.T) { } func TestParentDeletion(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() testFs := fcfg.Filesystem() defer cleanupModelAndRemoveDir(m, testFs.URI()) @@ -609,7 +618,8 @@ func TestRequestSymlinkWindows(t *testing.T) { t.Skip("windows specific test") } - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() defer cleanupModelAndRemoveDir(m, fcfg.Filesystem().URI()) received := make(chan []protocol.FileInfo) @@ -677,7 +687,8 @@ func equalContents(path string, contents []byte) error { } func TestRequestRemoteRenameChanged(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() tmpDir := tfs.URI() defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -812,7 +823,8 @@ func TestRequestRemoteRenameChanged(t *testing.T) { } func TestRequestRemoteRenameConflict(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() tmpDir := tfs.URI() defer cleanupModelAndRemoveDir(m, tmpDir) @@ -903,7 +915,8 @@ func TestRequestRemoteRenameConflict(t *testing.T) { } func TestRequestDeleteChanged(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -972,7 +985,8 @@ func TestRequestDeleteChanged(t *testing.T) { } func TestNeedFolderFiles(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() tmpDir := tfs.URI() defer cleanupModelAndRemoveDir(m, tmpDir) @@ -1020,7 +1034,8 @@ func TestNeedFolderFiles(t *testing.T) { // propagated upon un-ignoring. // https://github.com/syncthing/syncthing/issues/6038 func TestIgnoreDeleteUnignore(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() m := setupModel(t, w) fss := fcfg.Filesystem() tmpDir := fss.URI() @@ -1120,7 +1135,8 @@ func TestIgnoreDeleteUnignore(t *testing.T) { // TestRequestLastFileProgress checks that the last pulled file (here only) is registered // as in progress. func TestRequestLastFileProgress(t *testing.T) { - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -1156,7 +1172,8 @@ func TestRequestIndexSenderPause(t *testing.T) { done := make(chan struct{}) defer close(done) - m, fc, fcfg := setupModelWithConnection(t) + m, fc, fcfg, wcfgCancel := setupModelWithConnection(t) + defer wcfgCancel() tfs := fcfg.Filesystem() defer cleanupModelAndRemoveDir(m, tfs.URI()) @@ -1215,13 +1232,8 @@ func TestRequestIndexSenderPause(t *testing.T) { // Local paused and resume - fcfg.Paused = true - waiter, _ := m.cfg.SetFolder(fcfg) - waiter.Wait() - - fcfg.Paused = false - waiter, _ = m.cfg.SetFolder(fcfg) - waiter.Wait() + pauseFolder(t, m.cfg, fcfg.ID, true) + pauseFolder(t, m.cfg, fcfg.ID, false) seq++ files[0].Sequence = seq @@ -1238,16 +1250,12 @@ func TestRequestIndexSenderPause(t *testing.T) { cc.Folders[0].Paused = true m.ClusterConfig(device1, cc) - fcfg.Paused = true - waiter, _ = m.cfg.SetFolder(fcfg) - waiter.Wait() + pauseFolder(t, m.cfg, fcfg.ID, true) cc.Folders[0].Paused = false m.ClusterConfig(device1, cc) - fcfg.Paused = false - waiter, _ = m.cfg.SetFolder(fcfg) - waiter.Wait() + pauseFolder(t, m.cfg, fcfg.ID, false) seq++ files[0].Sequence = seq @@ -1277,7 +1285,8 @@ func TestRequestIndexSenderPause(t *testing.T) { } func TestRequestIndexSenderClusterConfigBeforeStart(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() tfs := fcfg.Filesystem() dir1 := "foo" dir2 := "bar" @@ -1340,12 +1349,11 @@ func TestRequestIndexSenderClusterConfigBeforeStart(t *testing.T) { } func TestRequestReceiveEncryptedLocalNoSend(t *testing.T) { - w, fcfg := tmpDefaultWrapper() + w, fcfg, wCancel := tmpDefaultWrapper() + defer wCancel() tfs := fcfg.Filesystem() fcfg.Type = config.FolderTypeReceiveEncrypted - waiter, err := w.SetFolder(fcfg) - must(t, err) - waiter.Wait() + setFolder(t, w, fcfg) encToken := protocol.PasswordToken(fcfg.ID, "pw") must(t, tfs.Mkdir(config.DefaultMarkerName, 0777)) diff --git a/lib/model/testutils_test.go b/lib/model/testutils_test.go index 2840ca9d8..b74caa81d 100644 --- a/lib/model/testutils_test.go +++ b/lib/model/testutils_test.go @@ -13,6 +13,8 @@ import ( "testing" "time" + "github.com/thejerf/suture/v4" + "github.com/syncthing/syncthing/lib/config" "github.com/syncthing/syncthing/lib/db" "github.com/syncthing/syncthing/lib/db/backend" @@ -24,12 +26,13 @@ import ( ) var ( - myID, device1, device2 protocol.DeviceID - defaultCfgWrapper config.Wrapper - defaultFolderConfig config.FolderConfiguration - defaultFs fs.Filesystem - defaultCfg config.Configuration - defaultAutoAcceptCfg config.Configuration + myID, device1, device2 protocol.DeviceID + defaultCfgWrapper config.Wrapper + defaultCfgWrapperCancel context.CancelFunc + defaultFolderConfig config.FolderConfiguration + defaultFs fs.Filesystem + defaultCfg config.Configuration + defaultAutoAcceptCfg config.Configuration ) func init() { @@ -40,12 +43,13 @@ func init() { defaultFolderConfig = testFolderConfig("testdata") defaultFs = defaultFolderConfig.Filesystem() - defaultCfgWrapper = createTmpWrapper(config.New(myID)) - _, _ = defaultCfgWrapper.SetDevice(config.NewDeviceConfiguration(device1, "device1")) - _, _ = defaultCfgWrapper.SetFolder(defaultFolderConfig) - opts := defaultCfgWrapper.Options() - opts.KeepTemporariesH = 1 - _, _ = defaultCfgWrapper.SetOptions(opts) + defaultCfgWrapper, defaultCfgWrapperCancel = createTmpWrapper(config.New(myID)) + waiter, _ := defaultCfgWrapper.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(config.NewDeviceConfiguration(device1, "device1")) + cfg.SetFolder(defaultFolderConfig) + cfg.Options.KeepTemporariesH = 1 + }) + waiter.Wait() defaultCfg = defaultCfgWrapper.RawCopy() @@ -70,11 +74,28 @@ func init() { } } -func tmpDefaultWrapper() (config.Wrapper, config.FolderConfiguration) { - w := createTmpWrapper(defaultCfgWrapper.RawCopy()) +func createTmpWrapper(cfg config.Configuration) (config.Wrapper, context.CancelFunc) { + tmpFile, err := ioutil.TempFile("", "syncthing-testConfig-") + if err != nil { + panic(err) + } + wrapper := config.Wrap(tmpFile.Name(), cfg, myID, events.NoopLogger) + tmpFile.Close() + if cfgService, ok := wrapper.(suture.Service); ok { + ctx, cancel := context.WithCancel(context.Background()) + go cfgService.Serve(ctx) + return wrapper, cancel + } + return wrapper, func() {} +} + +func tmpDefaultWrapper() (config.Wrapper, config.FolderConfiguration, context.CancelFunc) { + w, cancel := createTmpWrapper(defaultCfgWrapper.RawCopy()) fcfg := testFolderConfigTmp() - _, _ = w.SetFolder(fcfg) - return w, fcfg + _, _ = w.Modify(func(cfg *config.Configuration) { + cfg.SetFolder(fcfg) + }) + return w, fcfg, cancel } func testFolderConfigTmp() config.FolderConfiguration { @@ -96,11 +117,11 @@ func testFolderConfigFake() config.FolderConfiguration { return cfg } -func setupModelWithConnection(t testing.TB) (*testModel, *fakeConnection, config.FolderConfiguration) { +func setupModelWithConnection(t testing.TB) (*testModel, *fakeConnection, config.FolderConfiguration, context.CancelFunc) { t.Helper() - w, fcfg := tmpDefaultWrapper() + w, fcfg, cancel := tmpDefaultWrapper() m, fc := setupModelWithConnectionFromWrapper(t, w) - return m, fc, fcfg + return m, fc, fcfg, cancel } func setupModelWithConnectionFromWrapper(t testing.TB, w config.Wrapper) (*testModel, *fakeConnection) { @@ -313,3 +334,70 @@ func newFileSet(t testing.TB, folder string, fs fs.Filesystem, ldb *db.Lowlevel) } return fset } + +func replace(t testing.TB, w config.Wrapper, to config.Configuration) { + t.Helper() + waiter, err := w.Modify(func(cfg *config.Configuration) { + *cfg = to + }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() +} + +func pauseFolder(t testing.TB, w config.Wrapper, id string, paused bool) { + t.Helper() + waiter, err := w.Modify(func(cfg *config.Configuration) { + _, i, _ := cfg.Folder(id) + cfg.Folders[i].Paused = paused + }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() +} + +func setFolder(t testing.TB, w config.Wrapper, fcfg config.FolderConfiguration) { + t.Helper() + waiter, err := w.Modify(func(cfg *config.Configuration) { + cfg.SetFolder(fcfg) + }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() +} + +func pauseDevice(t testing.TB, w config.Wrapper, id protocol.DeviceID, paused bool) { + t.Helper() + waiter, err := w.Modify(func(cfg *config.Configuration) { + _, i, _ := cfg.Device(id) + cfg.Devices[i].Paused = paused + }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() +} + +func setDevice(t testing.TB, w config.Wrapper, device config.DeviceConfiguration) { + t.Helper() + waiter, err := w.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(device) + }) + if err != nil { + t.Fatal(err) + } + waiter.Wait() +} + +func addDevice2(t testing.TB, w config.Wrapper, fcfg config.FolderConfiguration) { + waiter, err := w.Modify(func(cfg *config.Configuration) { + cfg.SetDevice(config.NewDeviceConfiguration(device2, "device2")) + fcfg.Devices = append(fcfg.Devices, config.FolderDeviceConfiguration{DeviceID: device2}) + cfg.SetFolder(fcfg) + }) + must(t, err) + waiter.Wait() +} diff --git a/lib/syncthing/syncthing.go b/lib/syncthing/syncthing.go index c4ed0b5ef..578be2f9b 100644 --- a/lib/syncthing/syncthing.go +++ b/lib/syncthing/syncthing.go @@ -121,6 +121,10 @@ func (a *App) Start() error { } func (a *App) startup() error { + if cfgService, ok := a.cfg.(suture.Service); ok { + a.mainService.Add(cfgService) + } + a.mainService.Add(ur.NewFailureHandler(a.cfg, a.evLogger)) a.mainService.Add(a.ll) @@ -277,24 +281,21 @@ func (a *App) startup() error { a.mainService.Add(discoveryManager) a.mainService.Add(connectionsService) - // Candidate builds always run with usage reporting. - - if opts := a.cfg.Options(); build.IsCandidate { - l.Infoln("Anonymous usage reporting is always enabled for candidate releases.") - if opts.URAccepted != ur.Version { - opts.URAccepted = ur.Version - a.cfg.SetOptions(opts) - a.cfg.Save() - // Unique ID will be set and config saved below if necessary. + a.cfg.Modify(func(cfg *config.Configuration) { + // Candidate builds always run with usage reporting. + if build.IsCandidate { + l.Infoln("Anonymous usage reporting is always enabled for candidate releases.") + if cfg.Options.URAccepted != ur.Version { + cfg.Options.URAccepted = ur.Version + // Unique ID will be set and config saved below if necessary. + } } - } - // If we are going to do usage reporting, ensure we have a valid unique ID. - if opts := a.cfg.Options(); opts.URAccepted > 0 && opts.URUniqueID == "" { - opts.URUniqueID = rand.String(8) - a.cfg.SetOptions(opts) - a.cfg.Save() - } + // If we are going to do usage reporting, ensure we have a valid unique ID. + if cfg.Options.URAccepted > 0 && cfg.Options.URUniqueID == "" { + cfg.Options.URUniqueID = rand.String(8) + } + }) usageReportingSvc := ur.New(a.cfg, m, connectionsService, a.opts.NoUpgrade) a.mainService.Add(usageReportingSvc) diff --git a/lib/ur/failurereporting.go b/lib/ur/failurereporting.go index 4e28ffc9b..28ef2e4df 100644 --- a/lib/ur/failurereporting.go +++ b/lib/ur/failurereporting.go @@ -67,37 +67,17 @@ type failureStat struct { } func (h *failureHandler) Serve(ctx context.Context) error { - go func() { - select { - case h.optsChan <- h.cfg.Options(): - case <-ctx.Done(): - } - }() - h.cfg.Subscribe(h) + cfg := h.cfg.Subscribe(h) defer h.cfg.Unsubscribe(h) + url, sub, evChan := h.applyOpts(cfg.Options, nil) - var url string var err error - var sub events.Subscription - var evChan <-chan events.Event timer := time.NewTimer(minDelay) resetTimer := make(chan struct{}) -outer: - for { + for err == nil { select { case opts := <-h.optsChan: - // Sub nil checks just for safety - config updates can be racy. - if opts.URAccepted > 0 { - if sub == nil { - sub = h.evLogger.Subscribe(events.Failure) - evChan = sub.C() - } - } else if sub != nil { - sub.Unsubscribe() - sub = nil - evChan = nil - } - url = opts.CRURL + "/failure" + url, sub, evChan = h.applyOpts(opts, sub) case e, ok := <-evChan: if !ok { // Just to be safe - shouldn't ever happen, as @@ -137,7 +117,7 @@ outer: case <-resetTimer: timer.Reset(minDelay) case <-ctx.Done(): - break outer + err = ctx.Err() } } @@ -154,6 +134,21 @@ outer: return err } +func (h *failureHandler) applyOpts(opts config.OptionsConfiguration, sub events.Subscription) (string, events.Subscription, <-chan events.Event) { + // Sub nil checks just for safety - config updates can be racy. + url := opts.CRURL + "/failure" + if opts.URAccepted > 0 { + if sub == nil { + sub = h.evLogger.Subscribe(events.Failure) + } + return url, sub, sub.C() + } + if sub != nil { + sub.Unsubscribe() + } + return url, nil, nil +} + func (h *failureHandler) addReport(descr string, evTime time.Time) { if stat, ok := h.buf[descr]; ok { stat.last = evTime