Refactor config commit stuff to support restartless updates better

Includes restartless updates of the GUI settings (listening port etc) as
a proof of concept.

internal/config/commit_test.go

@@ -0,0 +1,83 @@
+package config
+
+import (
+	"errors"
+	"testing"
+)
+
+type requiresRestart struct{}
+
+func (requiresRestart) VerifyConfiguration(_, _ Configuration) error {
+	return nil
+}
+func (requiresRestart) CommitConfiguration(_, _ Configuration) bool {
+	return false
+}
+func (requiresRestart) String() string {
+	return "requiresRestart"
+}
+
+type validationError struct{}
+
+func (validationError) VerifyConfiguration(_, _ Configuration) error {
+	return errors.New("some error")
+}
+func (validationError) CommitConfiguration(_, _ Configuration) bool {
+	return true
+}
+func (validationError) String() string {
+	return "validationError"
+}
+
+func TestReplaceCommit(t *testing.T) {
+	w := Wrap("/dev/null", Configuration{Version: 0})
+	if w.Raw().Version != 0 {
+		t.Fatal("Config incorrect")
+	}
+
+	// Replace config. We should get back a clean response and the config
+	// should change.
+
+	resp := w.Replace(Configuration{Version: 1})
+	if resp.ValidationError != nil {
+		t.Fatal("Should not have a validation error")
+	}
+	if resp.RequiresRestart {
+		t.Fatal("Should not require restart")
+	}
+	if w.Raw().Version != 1 {
+		t.Fatal("Config should have changed")
+	}
+
+	// Now with a subscriber requiring restart. We should get a clean response
+	// but with the restart flag set, and the config should change.
+
+	w.Subscribe(requiresRestart{})
+
+	resp = w.Replace(Configuration{Version: 2})
+	if resp.ValidationError != nil {
+		t.Fatal("Should not have a validation error")
+	}
+	if !resp.RequiresRestart {
+		t.Fatal("Should require restart")
+	}
+	if w.Raw().Version != 2 {
+		t.Fatal("Config should have changed")
+	}
+
+	// Now with a subscriber that throws a validation error. The config should
+	// not change.
+
+	w.Subscribe(validationError{})
+
+	resp = w.Replace(Configuration{Version: 3})
+	if resp.ValidationError == nil {
+		t.Fatal("Should have a validation error")
+	}
+	if resp.RequiresRestart {
+		t.Fatal("Should not require restart")
+	}
+	if w.Raw().Version != 2 {
+		t.Fatal("Config should not have changed")
+	}
+}

internal/config/config.go

@@ -20,14 +20,11 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/calmh/logger"
 	"github.com/syncthing/protocol"
 	"github.com/syncthing/syncthing/internal/osutil"
 	"golang.org/x/crypto/bcrypt"
 )
 
-var l = logger.DefaultLogger
-
 const (
 	OldestHandledVersion = 5
 	CurrentVersion       = 10

internal/config/debug.go

@@ -0,0 +1,19 @@
+// Copyright (C) 2015 The Syncthing Authors.
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this file,
+// You can obtain one at http://mozilla.org/MPL/2.0/.
+
+package config
+
+import (
+	"os"
+	"strings"
+
+	"github.com/calmh/logger"
+)
+
+var (
+	debug = strings.Contains(os.Getenv("STTRACE"), "config") || os.Getenv("STTRACE") == "all"
+	l     = logger.DefaultLogger
+)

internal/config/wrapper.go

@@ -17,17 +17,39 @@ import (
 	"github.com/syncthing/syncthing/internal/sync"
 )
 
-// An interface to handle configuration changes, and a wrapper type á la
-// http.Handler
-
-type Handler interface {
-	Changed(Configuration) error
+// The Committer interface is implemented by objects that need to know about
+// or have a say in configuration changes.
+//
+// When the configuration is about to be changed, VerifyConfiguration() is
+// called for each subscribing object, with the old and new configuration. A
+// nil error is returned if the new configuration is acceptable (i.e. does not
+// contain any errors that would prevent it from being a valid config).
+// Otherwise an error describing the problem is returned.
+//
+// If any subscriber returns an error from VerifyConfiguration(), the
+// configuration change is not committed and an error is returned to whoever
+// tried to commit the broken config.
+//
+// If all verification calls returns nil, CommitConfiguration() is called for
+// each subscribing object. The callee returns true if the new configuration
+// has been successfully applied, otherwise false. Any Commit() call returning
+// false will result in a "restart needed" respone to the API/user. Note that
+// the new configuration will still have been applied by those who were
+// capable of doing so.
+type Committer interface {
+	VerifyConfiguration(from, to Configuration) error
+	CommitConfiguration(from, to Configuration) (handled bool)
+	String() string
 }
 
-type HandlerFunc func(Configuration) error
+type CommitResponse struct {
+	ValidationError error
+	RequiresRestart bool
+}
 
-func (fn HandlerFunc) Changed(cfg Configuration) error {
-	return fn(cfg)
+var ResponseNoRestart = CommitResponse{
+	ValidationError: nil,
+	RequiresRestart: false,
 }
 
 // A wrapper around a Configuration that manages loads, saves and published
@@ -42,7 +64,7 @@ type Wrapper struct {
 	replaces  chan Configuration
 	mut       sync.Mutex
 
-	subs []Handler
+	subs []Committer
 	sMut sync.Mutex
 }
 
@@ -56,7 +78,6 @@ func Wrap(path string, cfg Configuration) *Wrapper {
 		sMut: sync.NewMutex(),
 	}
 	w.replaces = make(chan Configuration)
-	go w.Serve()
 	return w
 }
 
@@ -77,21 +98,6 @@ func Load(path string, myID protocol.DeviceID) (*Wrapper, error) {
 	return Wrap(path, cfg), nil
 }
 
-// Serve handles configuration replace events and calls any interested
-// handlers. It is started automatically by Wrap() and Load() and should not
-// be run manually.
-func (w *Wrapper) Serve() {
-	for cfg := range w.replaces {
-		w.sMut.Lock()
-		subs := w.subs
-		w.sMut.Unlock()
-
-		for _, h := range subs {
-			h.Changed(cfg)
-		}
-	}
-}
-
 // Stop stops the Serve() loop. Set and Replace operations will panic after a
 // Stop.
 func (w *Wrapper) Stop() {
@@ -100,9 +106,9 @@ func (w *Wrapper) Stop() {
 
 // Subscribe registers the given handler to be called on any future
 // configuration changes.
-func (w *Wrapper) Subscribe(h Handler) {
+func (w *Wrapper) Subscribe(c Committer) {
 	w.sMut.Lock()
-	w.subs = append(w.subs, h)
+	w.subs = append(w.subs, c)
 	w.sMut.Unlock()
 }
 
@@ -112,14 +118,50 @@ func (w *Wrapper) Raw() Configuration {
 }
 
 // Replace swaps the current configuration object for the given one.
-func (w *Wrapper) Replace(cfg Configuration) {
+func (w *Wrapper) Replace(cfg Configuration) CommitResponse {
 	w.mut.Lock()
 	defer w.mut.Unlock()
+	return w.replaceLocked(cfg)
+}
 
-	w.cfg = cfg
+func (w *Wrapper) replaceLocked(to Configuration) CommitResponse {
+	from := w.cfg
+
+	for _, sub := range w.subs {
+		if debug {
+			l.Debugln(sub, "verifying configuration")
+		}
+		if err := sub.VerifyConfiguration(from, to); err != nil {
+			if debug {
+				l.Debugln(sub, "rejected config:", err)
+			}
+			return CommitResponse{
+				ValidationError: err,
+			}
+		}
+	}
+
+	allOk := true
+	for _, sub := range w.subs {
+		if debug {
+			l.Debugln(sub, "committing configuration")
+		}
+		ok := sub.CommitConfiguration(from, to)
+		if !ok {
+			if debug {
+				l.Debugln(sub, "requires restart")
+			}
+			allOk = false
+		}
+	}
+
+	w.cfg = to
 	w.deviceMap = nil
 	w.folderMap = nil
-	w.replaces <- cfg.Copy()
+
+	return CommitResponse{
+		RequiresRestart: !allOk,
+	}
 }
 
 // Devices returns a map of devices. Device structures should not be changed,
@@ -138,22 +180,24 @@ func (w *Wrapper) Devices() map[protocol.DeviceID]DeviceConfiguration {
 
 // SetDevice adds a new device to the configuration, or overwrites an existing
 // device with the same ID.
-func (w *Wrapper) SetDevice(dev DeviceConfiguration) {
+func (w *Wrapper) SetDevice(dev DeviceConfiguration) CommitResponse {
 	w.mut.Lock()
 	defer w.mut.Unlock()
 
-	w.deviceMap = nil
-
-	for i := range w.cfg.Devices {
-		if w.cfg.Devices[i].DeviceID == dev.DeviceID {
-			w.cfg.Devices[i] = dev
-			w.replaces <- w.cfg.Copy()
-			return
+	newCfg := w.cfg.Copy()
+	replaced := false
+	for i := range newCfg.Devices {
+		if newCfg.Devices[i].DeviceID == dev.DeviceID {
+			newCfg.Devices[i] = dev
+			replaced = true
+			break
 		}
 	}
+	if !replaced {
+		newCfg.Devices = append(w.cfg.Devices, dev)
+	}
 
-	w.cfg.Devices = append(w.cfg.Devices, dev)
-	w.replaces <- w.cfg.Copy()
+	return w.replaceLocked(newCfg)
 }
 
 // Folders returns a map of folders. Folder structures should not be changed,
@@ -172,22 +216,24 @@ func (w *Wrapper) Folders() map[string]FolderConfiguration {
 
 // SetFolder adds a new folder to the configuration, or overwrites an existing
 // folder with the same ID.
-func (w *Wrapper) SetFolder(fld FolderConfiguration) {
+func (w *Wrapper) SetFolder(fld FolderConfiguration) CommitResponse {
 	w.mut.Lock()
 	defer w.mut.Unlock()
 
-	w.folderMap = nil
-
-	for i := range w.cfg.Folders {
-		if w.cfg.Folders[i].ID == fld.ID {
-			w.cfg.Folders[i] = fld
-			w.replaces <- w.cfg.Copy()
-			return
+	newCfg := w.cfg.Copy()
+	replaced := false
+	for i := range newCfg.Folders {
+		if newCfg.Folders[i].ID == fld.ID {
+			newCfg.Folders[i] = fld
+			replaced = true
+			break
 		}
 	}
+	if !replaced {
+		newCfg.Folders = append(w.cfg.Folders, fld)
+	}
 
-	w.cfg.Folders = append(w.cfg.Folders, fld)
-	w.replaces <- w.cfg.Copy()
+	return w.replaceLocked(newCfg)
 }
 
 // Options returns the current options configuration object.
@@ -198,11 +244,12 @@ func (w *Wrapper) Options() OptionsConfiguration {
 }
 
 // SetOptions replaces the current options configuration object.
-func (w *Wrapper) SetOptions(opts OptionsConfiguration) {
+func (w *Wrapper) SetOptions(opts OptionsConfiguration) CommitResponse {
 	w.mut.Lock()
 	defer w.mut.Unlock()
-	w.cfg.Options = opts
-	w.replaces <- w.cfg.Copy()
+	newCfg := w.cfg.Copy()
+	newCfg.Options = opts
+	return w.replaceLocked(newCfg)
 }
 
 // GUI returns the current GUI configuration object.
@@ -213,11 +260,12 @@ func (w *Wrapper) GUI() GUIConfiguration {
 }
 
 // SetGUI replaces the current GUI configuration object.
-func (w *Wrapper) SetGUI(gui GUIConfiguration) {
+func (w *Wrapper) SetGUI(gui GUIConfiguration) CommitResponse {
 	w.mut.Lock()
 	defer w.mut.Unlock()
-	w.cfg.GUI = gui
-	w.replaces <- w.cfg.Copy()
+	newCfg := w.cfg.Copy()
+	newCfg.GUI = gui
+	return w.replaceLocked(newCfg)
 }
 
 // IgnoredDevice returns whether or not connection attempts from the given

internal/db/blockmap.go

@@ -15,6 +15,7 @@ package db
 import (
 	"bytes"
 	"encoding/binary"
+	"fmt"
 	"sort"
 
 	"github.com/syncthing/protocol"
@@ -125,15 +126,22 @@ func NewBlockFinder(db *leveldb.DB, cfg *config.Wrapper) *BlockFinder {
 		db:  db,
 		mut: sync.NewRWMutex(),
 	}
-	f.Changed(cfg.Raw())
+
+	f.CommitConfiguration(config.Configuration{}, cfg.Raw())
 	cfg.Subscribe(f)
+
 	return f
 }
 
-// Changed implements config.Handler interface
-func (f *BlockFinder) Changed(cfg config.Configuration) error {
-	folders := make([]string, len(cfg.Folders))
-	for i, folder := range cfg.Folders {
+// VerifyConfiguration implementes the config.Committer interface
+func (f *BlockFinder) VerifyConfiguration(from, to config.Configuration) error {
+	return nil
+}
+
+// CommitConfiguration implementes the config.Committer interface
+func (f *BlockFinder) CommitConfiguration(from, to config.Configuration) bool {
+	folders := make([]string, len(to.Folders))
+	for i, folder := range to.Folders {
 		folders[i] = folder.ID
 	}
 
@@ -143,7 +151,11 @@ func (f *BlockFinder) Changed(cfg config.Configuration) error {
 	f.folders = folders
 	f.mut.Unlock()
 
-	return nil
+	return true
+}
+
+func (f *BlockFinder) String() string {
+	return fmt.Sprintf("BlockFinder@%p", f)
 }
 
 // Iterate takes an iterator function which iterates over all matching blocks

internal/model/model.go

@@ -17,6 +17,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"strings"
 	stdsync "sync"
@@ -1718,6 +1719,37 @@ func (m *Model) String() string {
 	return fmt.Sprintf("model@%p", m)
 }
 
+func (m *Model) VerifyConfiguration(from, to config.Configuration) error {
+	return nil
+}
+
+func (m *Model) CommitConfiguration(from, to config.Configuration) bool {
+	// TODO: This should not use reflect, and should take more care to try to handle stuff without restart.
+
+	// Adding, removing or changing folders requires restart
+	if !reflect.DeepEqual(from.Folders, to.Folders) {
+		return false
+	}
+
+	// Removing a device requres restart
+	toDevs := make(map[protocol.DeviceID]bool, len(from.Devices))
+	for _, dev := range to.Devices {
+		toDevs[dev.DeviceID] = true
+	}
+	for _, dev := range from.Devices {
+		if _, ok := toDevs[dev.DeviceID]; !ok {
+			return false
+		}
+	}
+
+	// All of the generic options require restart
+	if !reflect.DeepEqual(from.Options, to.Options) {
+		return false
+	}
+
+	return true
+}
+
 func symlinkInvalid(isLink bool) bool {
 	if !symlinks.Supported && isLink {
 		SymlinkWarning.Do(func() {

internal/model/model_test.go

@@ -317,21 +317,22 @@ func TestDeviceRename(t *testing.T) {
 
 	defer os.Remove("tmpconfig.xml")
 
-	cfg := config.New(device1)
-	cfg.Devices = []config.DeviceConfiguration{
+	rawCfg := config.New(device1)
+	rawCfg.Devices = []config.DeviceConfiguration{
 		{
 			DeviceID: device1,
 		},
 	}
+	cfg := config.Wrap("tmpconfig.xml", rawCfg)
 
 	db, _ := leveldb.Open(storage.NewMemStorage(), nil)
-	m := NewModel(config.Wrap("tmpconfig.xml", cfg), protocol.LocalDeviceID, "device", "syncthing", "dev", db)
-	if cfg.Devices[0].Name != "" {
+	m := NewModel(cfg, protocol.LocalDeviceID, "device", "syncthing", "dev", db)
+	if cfg.Devices()[device1].Name != "" {
 		t.Errorf("Device already has a name")
 	}
 
 	m.ClusterConfig(device1, ccm)
-	if cfg.Devices[0].Name != "" {
+	if cfg.Devices()[device1].Name != "" {
 		t.Errorf("Device already has a name")
 	}
 
@@ -342,13 +343,13 @@ func TestDeviceRename(t *testing.T) {
 		},
 	}
 	m.ClusterConfig(device1, ccm)
-	if cfg.Devices[0].Name != "tester" {
+	if cfg.Devices()[device1].Name != "tester" {
 		t.Errorf("Device did not get a name")
 	}
 
 	ccm.Options[0].Value = "tester2"
 	m.ClusterConfig(device1, ccm)
-	if cfg.Devices[0].Name != "tester" {
+	if cfg.Devices()[device1].Name != "tester" {
 		t.Errorf("Device name got overwritten")
 	}
 

internal/model/progressemitter.go

@@ -7,6 +7,7 @@
 package model
 
 import (
+	"fmt"
 	"path/filepath"
 	"reflect"
 	"time"
@@ -37,8 +38,10 @@ func NewProgressEmitter(cfg *config.Wrapper) *ProgressEmitter {
 		timer:    time.NewTimer(time.Millisecond),
 		mut:      sync.NewMutex(),
 	}
-	t.Changed(cfg.Raw())
+
+	t.CommitConfiguration(config.Configuration{}, cfg.Raw())
 	cfg.Subscribe(t)
+
 	return t
 }
 
@@ -81,17 +84,22 @@ func (t *ProgressEmitter) Serve() {
 	}
 }
 
-// Changed implements the config.Handler Interface to handle configuration
-// changes
-func (t *ProgressEmitter) Changed(cfg config.Configuration) error {
+// VerifyConfiguration implements the config.Committer interface
+func (t *ProgressEmitter) VerifyConfiguration(from, to config.Configuration) error {
+	return nil
+}
+
+// CommitConfiguration implements the config.Committer interface
+func (t *ProgressEmitter) CommitConfiguration(from, to config.Configuration) bool {
 	t.mut.Lock()
 	defer t.mut.Unlock()
 
-	t.interval = time.Duration(cfg.Options.ProgressUpdateIntervalS) * time.Second
+	t.interval = time.Duration(to.Options.ProgressUpdateIntervalS) * time.Second
 	if debug {
 		l.Debugln("progress emitter: updated interval", t.interval)
 	}
-	return nil
+
+	return true
 }
 
 // Stop stops the emitter.
@@ -138,3 +146,7 @@ func (t *ProgressEmitter) BytesCompleted(folder string) (bytes int64) {
 	}
 	return
 }
+
+func (t *ProgressEmitter) String() string {
+	return fmt.Sprintf("ProgressEmitter@%p", t)
+}