config: make config file reads reload the config file if needed #4996

Before this change the config file needed to be explicitly reloaded.
This coupled the config file implementation with the backends
needlessly.

This change stats the config file to see if it needs to be reloaded on
every config file operation.

This allows us to remove calls to

- config.SaveConfig
- config.GetFresh

Which now makes the the only needed interface to the config file be
that provided by configmap.Map when rclone is not being configured.

This also adds tests for configfile

backend/onedrive/onedrive.go

@@ -302,7 +302,6 @@ func init() {
 
 			m.Set(configDriveID, finalDriveID)
 			m.Set(configDriveType, rootItem.ParentReference.DriveType)
-			config.SaveConfig()
 		},
 		Options: append(oauthutil.SharedOptions, []fs.Option{{
 			Name:    "region",

backend/seafile/seafile.go

@@ -372,7 +372,6 @@ func Config(ctx context.Context, name string, m configmap.Mapper) {
 			m.Set(configAuthToken, token)
 			// And delete any previous entry for password
 			m.Set(configPassword, "")
-			config.SaveConfig()
 			// And we're done here
 			break
 		}

fs/config/config.go

@@ -230,7 +230,7 @@ func LoadConfig(ctx context.Context) {
 var ErrorConfigFileNotFound = errors.New("config file not found")
 
 // SaveConfig calling function which saves configuration file.
-// if saveConfig returns error trying again after sleep.
+// if SaveConfig returns error trying again after sleep.
 func SaveConfig() {
 	ctx := context.Background()
 	ci := fs.GetConfig(ctx)
@@ -253,19 +253,6 @@ func SaveConfig() {
 func SetValueAndSave(name, key, value string) error {
 	// Set the value in config in case we fail to reload it
 	Data.SetValue(name, key, value)
-
-	// Reload the config file
-	err := Data.Load()
-	if err == ErrorConfigFileNotFound {
-		// Config file not written yet so ignore reload
-		return nil
-	} else if err != nil {
-		return err
-	}
-	if !Data.HasSection(name) {
-		// Section doesn't exist yet so ignore reload
-		return nil
-	}
 	// Save it again
 	SaveConfig()
 	return nil
@@ -281,20 +268,6 @@ func getWithDefault(name, key, defaultValue string) string {
 	return value
 }
 
-// FileGetFresh reads the config key under section return the value or
-// an error if the config file was not found or that value couldn't be
-// read.
-func FileGetFresh(section, key string) (value string, err error) {
-	if err := Data.Load(); err != nil {
-		return "", err
-	}
-	value, found := Data.GetValue(section, key)
-	if !found {
-		return "", errors.New("value not found")
-	}
-	return value, nil
-}
-
 // UpdateRemote adds the keyValues passed in to the remote of name.
 // keyValues should be key, value pairs.
 func UpdateRemote(ctx context.Context, name string, keyValues rc.Params, doObscure, noObscure bool) error {
@@ -446,11 +419,6 @@ func FileDeleteKey(section, key string) bool {
 
 var matchEnv = regexp.MustCompile(`^RCLONE_CONFIG_(.*?)_TYPE=.*$`)
 
-// FileRefresh ensures the latest configFile is loaded from disk
-func FileRefresh() error {
-	return Data.Load()
-}
-
 // FileSections returns the sections in the config file
 // including any defined by environment variables.
 func FileSections() []string {

fs/config/config_test.go

@@ -3,16 +3,12 @@
 package config_test
 
 import (
-	"bytes"
 	"context"
-	"io/ioutil"
 	"testing"
 
 	"github.com/rclone/rclone/fs/config"
 	"github.com/rclone/rclone/fs/config/configfile"
-	"github.com/rclone/rclone/fs/rc"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestConfigLoad(t *testing.T) {
@@ -31,22 +27,3 @@ func TestConfigLoad(t *testing.T) {
 	expect = []string{"type", "nounc"}
 	assert.Equal(t, expect, keys)
 }
-
-func TestFileRefresh(t *testing.T) {
-	ctx := context.Background()
-	defer testConfigFile(t, "refresh.conf")()
-	require.NoError(t, config.CreateRemote(ctx, "refresh_test", "config_test_remote", rc.Params{
-		"bool": true,
-	}, false, false))
-	b, err := ioutil.ReadFile(config.ConfigPath)
-	assert.NoError(t, err)
-
-	b = bytes.Replace(b, []byte("refresh_test"), []byte("refreshed_test"), 1)
-	err = ioutil.WriteFile(config.ConfigPath, b, 0644)
-	assert.NoError(t, err)
-
-	assert.NotEqual(t, []string{"refreshed_test"}, config.Data.GetSectionList())
-	err = config.FileRefresh()
-	assert.NoError(t, err)
-	assert.Equal(t, []string{"refreshed_test"}, config.Data.GetSectionList())
-}

fs/config/configfile/configfile.go

@@ -1,3 +1,4 @@
+// Package configfile implements a config file loader and saver
 package configfile
 
 import (
@@ -6,6 +7,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sync"
 
 	"github.com/Unknwon/goconfig"
 	"github.com/pkg/errors"
@@ -22,28 +24,54 @@ func LoadConfig(ctx context.Context) {
 // Storage implements config.Storage for saving and loading config
 // data in a simple INI based file.
 type Storage struct {
-	gc *goconfig.ConfigFile
+	gc *goconfig.ConfigFile // config file loaded - thread safe
+	mu sync.Mutex           // to protect the following variables
+	fi os.FileInfo          // stat of the file when last loaded
 }
 
-// Load the config from permanent storage, decrypting if necessary
-func (s *Storage) Load() (err error) {
+// Check to see if we need to reload the config
+func (s *Storage) check() {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	// Check to see if config file has changed since it was last loaded
+	fi, err := os.Stat(config.ConfigPath)
+	if err == nil {
+		// check to see if config file has changed and if it has, reload it
+		if s.fi == nil || !fi.ModTime().Equal(s.fi.ModTime()) || fi.Size() != s.fi.Size() {
+			fs.Debugf(nil, "Config file has changed externaly - reloading")
+			err := s._load()
+			if err != nil {
+				fs.Errorf(nil, "Failed to read config file - using previous config: %v", err)
+			}
+		}
+	}
+}
+
+// _load the config from permanent storage, decrypting if necessary
+//
+// mu must be held when calling this
+func (s *Storage) _load() (err error) {
 	// Make sure we have a sensible default even when we error
 	defer func() {
-		if err != nil {
+		if s.gc == nil {
 			s.gc, _ = goconfig.LoadFromReader(bytes.NewReader([]byte{}))
 		}
 	}()
 
-	b, err := os.Open(config.ConfigPath)
+	fd, err := os.Open(config.ConfigPath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return config.ErrorConfigFileNotFound
 		}
 		return err
 	}
-	defer fs.CheckClose(b, &err)
+	defer fs.CheckClose(fd, &err)
 
-	cryptReader, err := config.Decrypt(b)
+	// Update s.fi with the current file info
+	s.fi, _ = os.Stat(config.ConfigPath)
+
+	cryptReader, err := config.Decrypt(fd)
 	if err != nil {
 		return err
 	}
@@ -57,8 +85,18 @@ func (s *Storage) Load() (err error) {
 	return nil
 }
 
+// Load the config from permanent storage, decrypting if necessary
+func (s *Storage) Load() (err error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s._load()
+}
+
 // Save the config to permanent storage, encrypting if necessary
 func (s *Storage) Save() error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
 	dir, name := filepath.Split(config.ConfigPath)
 	err := os.MkdirAll(dir, os.ModePerm)
 	if err != nil {
@@ -116,11 +154,15 @@ func (s *Storage) Save() error {
 		fs.Errorf(nil, "Failed to remove backup config file: %v", err)
 	}
 
+	// Update s.fi with the newly written file
+	s.fi, _ = os.Stat(config.ConfigPath)
+
 	return nil
 }
 
 // Serialize the config into a string
 func (s *Storage) Serialize() (string, error) {
+	s.check()
 	var buf bytes.Buffer
 	if err := goconfig.SaveConfigData(s.gc, &buf); err != nil {
 		return "", errors.Errorf("Failed to save config file: %v", err)
@@ -131,6 +173,7 @@ func (s *Storage) Serialize() (string, error) {
 
 // HasSection returns true if section exists in the config file
 func (s *Storage) HasSection(section string) bool {
+	s.check()
 	_, err := s.gc.GetSection(section)
 	if err != nil {
 		return false
@@ -141,22 +184,26 @@ func (s *Storage) HasSection(section string) bool {
 // DeleteSection removes the named section and all config from the
 // config file
 func (s *Storage) DeleteSection(section string) {
+	s.check()
 	s.gc.DeleteSection(section)
 }
 
 // GetSectionList returns a slice of strings with names for all the
 // sections
 func (s *Storage) GetSectionList() []string {
+	s.check()
 	return s.gc.GetSectionList()
 }
 
 // GetKeyList returns the keys in this section
 func (s *Storage) GetKeyList(section string) []string {
+	s.check()
 	return s.gc.GetKeyList(section)
 }
 
 // GetValue returns the key in section with a found flag
 func (s *Storage) GetValue(section string, key string) (value string, found bool) {
+	s.check()
 	value, err := s.gc.GetValue(section, key)
 	if err != nil {
 		return "", false
@@ -166,11 +213,13 @@ func (s *Storage) GetValue(section string, key string) (value string, found bool
 
 // SetValue sets the value under key in section
 func (s *Storage) SetValue(section string, key string, value string) {
+	s.check()
 	s.gc.SetValue(section, key, value)
 }
 
 // DeleteKey removes the key under section
 func (s *Storage) DeleteKey(section string, key string) bool {
+	s.check()
 	return s.gc.DeleteKey(section, key)
 }
 

fs/config/configfile/configfile_test.go

@@ -0,0 +1,215 @@
+package configfile
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/rclone/rclone/fs/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+var configData = `[one]
+type = number1
+fruit = potato
+
+[two]
+type = number2
+fruit = apple
+topping = nuts
+
+[three]
+type = number3
+fruit = banana
+
+`
+
+// Fill up a temporary config file with the testdata filename passed in
+func setConfigFile(t *testing.T, data string) func() {
+	out, err := ioutil.TempFile("", "rclone-configfile-test")
+	require.NoError(t, err)
+	filePath := out.Name()
+
+	_, err = out.Write([]byte(data))
+	require.NoError(t, err)
+
+	require.NoError(t, out.Close())
+
+	old := config.ConfigPath
+	config.ConfigPath = filePath
+	return func() {
+		config.ConfigPath = old
+		_ = os.Remove(filePath)
+	}
+}
+
+// toUnix converts \r\n to \n in buf
+func toUnix(buf string) string {
+	if runtime.GOOS == "windows" {
+		return strings.Replace(buf, "\r\n", "\n", -1)
+	}
+	return buf
+}
+
+func TestConfigFile(t *testing.T) {
+	defer setConfigFile(t, configData)()
+	data := &Storage{}
+
+	require.NoError(t, data.Load())
+
+	t.Run("Read", func(t *testing.T) {
+		t.Run("Serialize", func(t *testing.T) {
+			buf, err := data.Serialize()
+			require.NoError(t, err)
+			assert.Equal(t, configData, toUnix(buf))
+		})
+		t.Run("HasSection", func(t *testing.T) {
+			assert.True(t, data.HasSection("one"))
+			assert.False(t, data.HasSection("missing"))
+		})
+		t.Run("GetSectionList", func(t *testing.T) {
+			assert.Equal(t, []string{
+				"one",
+				"two",
+				"three",
+			}, data.GetSectionList())
+		})
+		t.Run("GetKeyList", func(t *testing.T) {
+			assert.Equal(t, []string{
+				"type",
+				"fruit",
+				"topping",
+			}, data.GetKeyList("two"))
+			assert.Equal(t, []string(nil), data.GetKeyList("unicorn"))
+		})
+		t.Run("GetValue", func(t *testing.T) {
+			value, ok := data.GetValue("one", "type")
+			assert.True(t, ok)
+			assert.Equal(t, "number1", value)
+			value, ok = data.GetValue("three", "fruit")
+			assert.True(t, ok)
+			assert.Equal(t, "banana", value)
+			value, ok = data.GetValue("one", "typeX")
+			assert.False(t, ok)
+			assert.Equal(t, "", value)
+			value, ok = data.GetValue("threeX", "fruit")
+			assert.False(t, ok)
+			assert.Equal(t, "", value)
+		})
+	})
+
+	//defer setConfigFile(configData)()
+
+	t.Run("Write", func(t *testing.T) {
+		t.Run("SetValue", func(t *testing.T) {
+			data.SetValue("one", "extra", "42")
+			data.SetValue("two", "fruit", "acorn")
+
+			buf, err := data.Serialize()
+			require.NoError(t, err)
+			assert.Equal(t, `[one]
+type = number1
+fruit = potato
+extra = 42
+
+[two]
+type = number2
+fruit = acorn
+topping = nuts
+
+[three]
+type = number3
+fruit = banana
+
+`, toUnix(buf))
+			t.Run("DeleteKey", func(t *testing.T) {
+				data.DeleteKey("one", "type")
+				data.DeleteKey("two", "missing")
+				data.DeleteKey("three", "fruit")
+				buf, err := data.Serialize()
+				require.NoError(t, err)
+				assert.Equal(t, `[one]
+fruit = potato
+extra = 42
+
+[two]
+type = number2
+fruit = acorn
+topping = nuts
+
+[three]
+type = number3
+
+`, toUnix(buf))
+				t.Run("DeleteSection", func(t *testing.T) {
+					data.DeleteSection("two")
+					data.DeleteSection("missing")
+					buf, err := data.Serialize()
+					require.NoError(t, err)
+					assert.Equal(t, `[one]
+fruit = potato
+extra = 42
+
+[three]
+type = number3
+
+`, toUnix(buf))
+					t.Run("Save", func(t *testing.T) {
+						require.NoError(t, data.Save())
+						buf, err := ioutil.ReadFile(config.ConfigPath)
+						require.NoError(t, err)
+						assert.Equal(t, `[one]
+fruit = potato
+extra = 42
+
+[three]
+type = number3
+
+`, toUnix(string(buf)))
+					})
+				})
+			})
+		})
+	})
+}
+
+func TestConfigFileReload(t *testing.T) {
+	defer setConfigFile(t, configData)()
+	data := &Storage{}
+
+	require.NoError(t, data.Load())
+
+	value, ok := data.GetValue("three", "appended")
+	assert.False(t, ok)
+	assert.Equal(t, "", value)
+
+	// Now write a new value on the end
+	out, err := os.OpenFile(config.ConfigPath, os.O_APPEND|os.O_WRONLY, 0777)
+	require.NoError(t, err)
+	fmt.Fprintln(out, "appended = what magic")
+	require.NoError(t, out.Close())
+
+	// And check we magically reloaded it
+	value, ok = data.GetValue("three", "appended")
+	assert.True(t, ok)
+	assert.Equal(t, "what magic", value)
+}
+
+func TestConfigFileDoesNotExist(t *testing.T) {
+	defer setConfigFile(t, configData)()
+	data := &Storage{}
+
+	require.NoError(t, os.Remove(config.ConfigPath))
+
+	err := data.Load()
+	require.Equal(t, config.ErrorConfigFileNotFound, err)
+
+	// check that using data doesn't crash
+	value, ok := data.GetValue("three", "appended")
+	assert.False(t, ok)
+	assert.Equal(t, "", value)
+}

lib/oauthutil/oauthutil.go

@@ -148,14 +148,8 @@ func PutToken(name string, m configmap.Mapper, token *oauth2.Token, newSection b
 	tokenString := string(tokenBytes)
 	old, ok := m.Get(config.ConfigToken)
 	if !ok || tokenString != old {
-		err = config.SetValueAndSave(name, config.ConfigToken, tokenString)
-		if newSection && err != nil {
-			fs.Debugf(name, "Added new token to config, still needs to be saved")
-		} else if err != nil {
-			fs.Errorf(nil, "Failed to save new token in config file: %v", err)
-		} else {
-			fs.Debugf(name, "Saved new token in config file")
-		}
+		m.Set(config.ConfigToken, tokenString)
+		fs.Debugf(name, "Saved new token in config file")
 	}
 	return nil
 }
@@ -175,13 +169,13 @@ type TokenSource struct {
 // If token has expired then first try re-reading it from the config
 // file in case a concurrently running rclone has updated it already
 func (ts *TokenSource) reReadToken() bool {
-	tokenString, err := config.FileGetFresh(ts.name, config.ConfigToken)
-	if err != nil {
-		fs.Debugf(ts.name, "Failed to read token out of config file: %v", err)
+	tokenString, found := ts.m.Get(config.ConfigToken)
+	if !found || tokenString == "" {
+		fs.Debugf(ts.name, "Failed to read token out of config file")
 		return false
 	}
 	newToken := new(oauth2.Token)
-	err = json.Unmarshal([]byte(tokenString), newToken)
+	err := json.Unmarshal([]byte(tokenString), newToken)
 	if err != nil {
 		fs.Debugf(ts.name, "Failed to parse token out of config file: %v", err)
 		return false