Martchus
/
syncthing
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

lib/config, cmd/syncthing: Handle committing configuration better (fixes #3077) 

This slightly changes the interface used for committing configuration
changes. The two parts are now:

 - VerifyConfiguration, which runs synchronously and locked, and can
   abort the config change. These callbacks shouldn't *do* anything
   apart from looking at the config changes and saying yes or no. No
   change from previously.

 - CommitConfiguration, which runs asynchronously (one goroutine per
   call) *after* replacing the config and releasing any locks. Returning
   false from these methods sets the "requires restart" flag, which now
   lives in the config.Wrapper.

This should be deadlock free as the CommitConfiguration calls can take
as long as they like and can wait for locks to be released when they
need to tweak things. I think this should be safe compared to before as
the CommitConfiguration calls were always made from a random background
goroutine (typically one from the HTTP server), so it was always
concurrent with everything else anyway.

Hence the CommitResponse type is gone, instead you get an error back on
verification failure only, and need to explicitly check
w.RequiresRestart() afterwards if you care.

As an added bonus this fixes a bug where we would reset the "requires
restart" indicator if a config that did not require restart was saved,
even if we already were in the requires-restart state.

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/3386
This commit is contained in:
Jakob Borg 2016-07-04 20:32:34 +00:00 committed by Audrius Butkevicius
parent 7ff7b55732
commit 44d30c83bf
4 changed files with 79 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
cmd/syncthing/gui.go
View File

@ -41,8 +41,7 @@ import (
)
var (
configInSync = true
startTime = time.Now()
startTime = time.Now()
)
type apiService struct {
@ -100,12 +99,13 @@ type configIntf interface {
GUI() config.GUIConfiguration
Raw() config.Configuration
Options() config.OptionsConfiguration
Replace(cfg config.Configuration) config.CommitResponse
Replace(cfg config.Configuration) error
Subscribe(c config.Committer)
Folders() map[string]config.FolderConfiguration
Devices() map[protocol.DeviceID]config.DeviceConfiguration
Save() error
ListenAddresses() []string
RequiresRestart() bool
}
type connectionsIntf interface {
@ -722,13 +722,19 @@ func (s *apiService) postSystemConfig(w http.ResponseWriter, r *http.Request) {
// Activate and save
resp := s.cfg.Replace(to)
configInSync = !resp.RequiresRestart
s.cfg.Save()
if err := s.cfg.Replace(to); err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
if err := s.cfg.Save(); err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
}
func (s *apiService) getSystemConfigInsync(w http.ResponseWriter, r *http.Request) {
sendJSON(w, map[string]bool{"configInSync": configInSync})
sendJSON(w, map[string]bool{"configInSync": !s.cfg.RequiresRestart()})
}
func (s *apiService) postSystemRestart(w http.ResponseWriter, r *http.Request) {

8
cmd/syncthing/mocked_config_test.go
View File

@ -31,8 +31,8 @@ func (c *mockedConfig) Options() config.OptionsConfiguration {
return config.OptionsConfiguration{}
}
func (c *mockedConfig) Replace(cfg config.Configuration) config.CommitResponse {
return config.CommitResponse{}
func (c *mockedConfig) Replace(cfg config.Configuration) error {
return nil
}
func (c *mockedConfig) Subscribe(cm config.Committer) {}
@ -48,3 +48,7 @@ func (c *mockedConfig) Devices() map[protocol.DeviceID]config.DeviceConfiguratio
func (c *mockedConfig) Save() error {
return nil
}
func (c *mockedConfig) RequiresRestart() bool {
return false
}

41
lib/config/commit_test.go
View File

@ -11,12 +11,18 @@ import (
"testing"
)
type requiresRestart struct{}
type requiresRestart struct {
committed chan struct{}
}
func (requiresRestart) VerifyConfiguration(_, _ Configuration) error {
return nil
}
func (requiresRestart) CommitConfiguration(_, _ Configuration) bool {
func (c requiresRestart) CommitConfiguration(_, _ Configuration) bool {
select {
case c.committed <- struct{}{}:
default:
}
return false
}
func (requiresRestart) String() string {
@ -28,7 +34,7 @@ type validationError struct{}
func (validationError) VerifyConfiguration(_, _ Configuration) error {
return errors.New("some error")
}
func (validationError) CommitConfiguration(_, _ Configuration) bool {
func (c validationError) CommitConfiguration(_, _ Configuration) bool {
return true
}
func (validationError) String() string {
@ -44,11 +50,11 @@ func TestReplaceCommit(t *testing.T) {
// Replace config. We should get back a clean response and the config
// should change.
resp := w.Replace(Configuration{Version: 1})
if resp.ValidationError != nil {
t.Fatal("Should not have a validation error")
err := w.Replace(Configuration{Version: 1})
if err != nil {
t.Fatal("Should not have a validation error:", err)
}
if resp.RequiresRestart {
if w.RequiresRestart() {
t.Fatal("Should not require restart")
}
if w.Raw().Version != 1 {
@ -58,13 +64,16 @@ func TestReplaceCommit(t *testing.T) {
// Now with a subscriber requiring restart. We should get a clean response
// but with the restart flag set, and the config should change.
w.Subscribe(requiresRestart{})
sub0 := requiresRestart{committed: make(chan struct{}, 1)}
w.Subscribe(sub0)
resp = w.Replace(Configuration{Version: 2})
if resp.ValidationError != nil {
t.Fatal("Should not have a validation error")
err = w.Replace(Configuration{Version: 2})
if err != nil {
t.Fatal("Should not have a validation error:", err)
}
if !resp.RequiresRestart {
<-sub0.committed
if !w.RequiresRestart() {
t.Fatal("Should require restart")
}
if w.Raw().Version != 2 {
@ -76,12 +85,12 @@ func TestReplaceCommit(t *testing.T) {
w.Subscribe(validationError{})
resp = w.Replace(Configuration{Version: 3})
if resp.ValidationError == nil {
err = w.Replace(Configuration{Version: 3})
if err == nil {
t.Fatal("Should have a validation error")
}
if resp.RequiresRestart {
t.Fatal("Should not require restart")
if !w.RequiresRestart() {
t.Fatal("Should still require restart")
}
if w.Raw().Version != 2 {
t.Fatal("Config should not have changed")

61
lib/config/wrapper.go
View File

@ -8,6 +8,7 @@ package config
import (
"os"
"sync/atomic"
"github.com/syncthing/syncthing/lib/events"
"github.com/syncthing/syncthing/lib/osutil"
@ -41,11 +42,6 @@ type Committer interface {
String() string
}
type CommitResponse struct {
ValidationError error
RequiresRestart bool
}
// A wrapper around a Configuration that manages loads, saves and published
// notifications of changes to registered Handlers
@ -58,6 +54,8 @@ type Wrapper struct {
replaces chan Configuration
subs []Committer
mut sync.Mutex
requiresRestart uint32 // an atomic bool
}
// Wrap wraps an existing Configuration structure and ties it to a file on
@ -128,32 +126,21 @@ func (w *Wrapper) Raw() Configuration {
}
// Replace swaps the current configuration object for the given one.
func (w *Wrapper) Replace(cfg Configuration) CommitResponse {
func (w *Wrapper) Replace(cfg Configuration) error {
w.mut.Lock()
defer w.mut.Unlock()
return w.replaceLocked(cfg)
}
func (w *Wrapper) replaceLocked(to Configuration) CommitResponse {
func (w *Wrapper) replaceLocked(to Configuration) error {
from := w.cfg
for _, sub := range w.subs {
l.Debugln(sub, "verifying configuration")
if err := sub.VerifyConfiguration(from, to); err != nil {
l.Debugln(sub, "rejected config:", err)
return CommitResponse{
ValidationError: err,
}
}
}
allOk := true
for _, sub := range w.subs {
l.Debugln(sub, "committing configuration")
ok := sub.CommitConfiguration(from, to)
if !ok {
l.Debugln(sub, "requires restart")
allOk = false
return err
}
}
@ -161,8 +148,22 @@ func (w *Wrapper) replaceLocked(to Configuration) CommitResponse {
w.deviceMap = nil
w.folderMap = nil
return CommitResponse{
RequiresRestart: !allOk,
w.notifyListeners(from, to)
return nil
}
func (w *Wrapper) notifyListeners(from, to Configuration) {
for _, sub := range w.subs {
go w.notifyListener(sub, from, to)
}
}
func (w *Wrapper) notifyListener(sub Committer, from, to Configuration) {
l.Debugln(sub, "committing configuration")
if !sub.CommitConfiguration(from, to) {
l.Debugln(sub, "requires restart")
w.setRequiresRestart()
}
}
@ -182,7 +183,7 @@ func (w *Wrapper) Devices() map[protocol.DeviceID]DeviceConfiguration {
// SetDevice adds a new device to the configuration, or overwrites an existing
// device with the same ID.
func (w *Wrapper) SetDevice(dev DeviceConfiguration) CommitResponse {
func (w *Wrapper) SetDevice(dev DeviceConfiguration) error {
w.mut.Lock()
defer w.mut.Unlock()
@ -218,7 +219,7 @@ func (w *Wrapper) Folders() map[string]FolderConfiguration {
// SetFolder adds a new folder to the configuration, or overwrites an existing
// folder with the same ID.
func (w *Wrapper) SetFolder(fld FolderConfiguration) CommitResponse {
func (w *Wrapper) SetFolder(fld FolderConfiguration) error {
w.mut.Lock()
defer w.mut.Unlock()
@ -246,7 +247,7 @@ func (w *Wrapper) Options() OptionsConfiguration {
}
// SetOptions replaces the current options configuration object.
func (w *Wrapper) SetOptions(opts OptionsConfiguration) CommitResponse {
func (w *Wrapper) SetOptions(opts OptionsConfiguration) error {
w.mut.Lock()
defer w.mut.Unlock()
newCfg := w.cfg.Copy()
@ -262,7 +263,7 @@ func (w *Wrapper) GUI() GUIConfiguration {
}
// SetGUI replaces the current GUI configuration object.
func (w *Wrapper) SetGUI(gui GUIConfiguration) CommitResponse {
func (w *Wrapper) SetGUI(gui GUIConfiguration) error {
w.mut.Lock()
defer w.mut.Unlock()
newCfg := w.cfg.Copy()
@ -332,3 +333,11 @@ func (w *Wrapper) ListenAddresses() []string {
}
return util.UniqueStrings(addresses)
}
func (w *Wrapper) RequiresRestart() bool {
return atomic.LoadUint32(&w.requiresRestart) != 0
}
func (w *Wrapper) setRequiresRestart() {
atomic.StoreUint32(&w.requiresRestart, 1)
}