Remove global config watcher (#8080)

* remove global config watcher

* keep config watcher disabled for tests

* compile fix

* fix resource leak

api/apitestlib.go

@@ -64,13 +64,18 @@ func StopTestStore() {
 }
 
 func setupTestHelper(enterprise bool) *TestHelper {
-	var options []app.Option
+	options := []app.Option{app.DisableConfigWatch}
 	if testStore != nil {
 		options = append(options, app.StoreOverride(testStore))
 	}
 
+	a, err := app.New(options...)
+	if err != nil {
+		panic(err)
+	}
+
 	th := &TestHelper{
-		App: app.New(options...),
+		App: a,
 	}
 	th.originalConfig = th.App.Config().Clone()
 

api4/apitestlib.go

@@ -73,13 +73,18 @@ func StopTestStore() {
 }
 
 func setupTestHelper(enterprise bool) *TestHelper {
-	var options []app.Option
+	options := []app.Option{app.DisableConfigWatch}
 	if testStore != nil {
 		options = append(options, app.StoreOverride(testStore))
 	}
 
+	a, err := app.New(options...)
+	if err != nil {
+		panic(err)
+	}
+
 	th := &TestHelper{
-		App: app.New(options...),
+		App: a,
 	}
 	th.originalConfig = th.App.Config().Clone()
 

app/admin.go

@@ -173,13 +173,13 @@ func (a *App) SaveConfig(cfg *model.Config, sendConfigChangeClusterMessage bool)
 		return model.NewAppError("saveConfig", "ent.cluster.save_config.error", nil, "", http.StatusForbidden)
 	}
 
-	utils.DisableConfigWatch()
+	a.DisableConfigWatch()
 	a.UpdateConfig(func(update *model.Config) {
 		*update = *cfg
 	})
 	a.PersistConfig()
 	a.ReloadConfig()
-	utils.EnableConfigWatch()
+	a.EnableConfigWatch()
 
 	if a.Metrics != nil {
 		if *a.Config().MetricsSettings.Enable {

app/app.go

@@ -4,19 +4,16 @@
 package app
 
 import (
-	"crypto/md5"
-	"encoding/json"
-	"fmt"
 	"html/template"
 	"net"
 	"net/http"
-	"runtime/debug"
 	"strings"
 	"sync"
 	"sync/atomic"
 
 	l4g "github.com/alecthomas/log4go"
 	"github.com/gorilla/mux"
+	"github.com/pkg/errors"
 
 	"github.com/mattermost/mattermost-server/einterfaces"
 	ejobs "github.com/mattermost/mattermost-server/einterfaces/jobs"
@@ -65,6 +62,8 @@ type App struct {
 	roles               map[string]*model.Role
 	configListenerId    string
 	licenseListenerId   string
+	disableConfigWatch  bool
+	configWatcher       *utils.ConfigWatcher
 
 	pluginCommands     []*PluginCommand
 	pluginCommandsLock sync.RWMutex
@@ -78,7 +77,7 @@ var appCount = 0
 
 // New creates a new App. You must call Shutdown when you're done with it.
 // XXX: For now, only one at a time is allowed as some resources are still shared.
-func New(options ...Option) *App {
+func New(options ...Option) (*App, error) {
 	appCount++
 	if appCount > 1 {
 		panic("Only one App should exist at a time. Did you forget to call Shutdown()?")
@@ -99,11 +98,16 @@ func New(options ...Option) *App {
 	}
 
 	if utils.T == nil {
-		utils.TranslationsPreInit()
+		if err := utils.TranslationsPreInit(); err != nil {
+			return nil, errors.Wrapf(err, "unable to load Mattermost translation files")
+		}
 	}
 	model.AppErrorInit(utils.T)
 	utils.LoadGlobalConfig(app.configFile)
-	utils.InitTranslations(utils.Cfg.LocalizationSettings)
+	app.EnableConfigWatch()
+	if err := utils.InitTranslations(utils.Cfg.LocalizationSettings); err != nil {
+		return nil, errors.Wrapf(err, "unable to load Mattermost translation files")
+	}
 
 	app.configListenerId = utils.AddConfigListener(func(_, _ *model.Config) {
 		app.configOrLicenseListener()
@@ -142,7 +146,7 @@ func New(options ...Option) *App {
 		handlers: make(map[string]webSocketHandler),
 	}
 
-	return app
+	return app, nil
 }
 
 func (a *App) configOrLicenseListener() {
@@ -171,6 +175,8 @@ func (a *App) Shutdown() {
 	utils.RemoveConfigListener(a.configListenerId)
 	utils.RemoveLicenseListener(a.licenseListenerId)
 	l4g.Info(utils.T("api.server.stop_server.stopped.info"))
+
+	a.DisableConfigWatch()
 }
 
 var accountMigrationInterface func(*App) einterfaces.AccountMigrationInterface
@@ -341,46 +347,6 @@ func (a *App) initJobs() {
 	}
 }
 
-func (a *App) Config() *model.Config {
-	return utils.Cfg
-}
-
-func (a *App) UpdateConfig(f func(*model.Config)) {
-	old := utils.Cfg.Clone()
-	f(utils.Cfg)
-	utils.InvokeGlobalConfigListeners(old, utils.Cfg)
-}
-
-func (a *App) PersistConfig() {
-	utils.SaveConfig(a.ConfigFileName(), a.Config())
-}
-
-func (a *App) ReloadConfig() {
-	debug.FreeOSMemory()
-	utils.LoadGlobalConfig(a.ConfigFileName())
-
-	// start/restart email batching job if necessary
-	a.InitEmailBatching()
-}
-
-func (a *App) ConfigFileName() string {
-	return utils.CfgFileName
-}
-
-func (a *App) ClientConfig() map[string]string {
-	return a.clientConfig
-}
-
-func (a *App) ClientConfigHash() string {
-	return a.clientConfigHash
-}
-
-func (a *App) regenerateClientConfig() {
-	a.clientConfig = utils.GenerateClientConfig(a.Config(), a.DiagnosticId())
-	clientConfigJSON, _ := json.Marshal(a.clientConfig)
-	a.clientConfigHash = fmt.Sprintf("%x", md5.Sum(clientConfigJSON))
-}
-
 func (a *App) DiagnosticId() string {
 	return a.diagnosticId
 }

app/app_test.go

@@ -11,6 +11,7 @@ import (
 	l4g "github.com/alecthomas/log4go"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/store/storetest"
@@ -47,7 +48,8 @@ func TestMain(m *testing.M) {
 
 func TestAppRace(t *testing.T) {
 	for i := 0; i < 10; i++ {
-		a := New()
+		a, err := New()
+		require.NoError(t, err)
 		a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
 		a.StartServer()
 		a.Shutdown()

app/apptestlib.go

@@ -57,13 +57,18 @@ func StopTestStore() {
 }
 
 func setupTestHelper(enterprise bool) *TestHelper {
-	var options []Option
+	options := []Option{DisableConfigWatch}
 	if testStore != nil {
 		options = append(options, StoreOverride(testStore))
 	}
 
+	a, err := New(options...)
+	if err != nil {
+		panic(err)
+	}
+
 	th := &TestHelper{
-		App:         New(options...),
+		App:         a,
 		pluginHooks: make(map[string]plugin.Hooks),
 	}
 

app/config.go

@@ -0,0 +1,73 @@
+// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved.
+// See License.txt for license information.
+
+package app
+
+import (
+	"crypto/md5"
+	"encoding/json"
+	"fmt"
+	"runtime/debug"
+
+	l4g "github.com/alecthomas/log4go"
+
+	"github.com/mattermost/mattermost-server/model"
+	"github.com/mattermost/mattermost-server/utils"
+)
+
+func (a *App) Config() *model.Config {
+	return utils.Cfg
+}
+
+func (a *App) UpdateConfig(f func(*model.Config)) {
+	old := utils.Cfg.Clone()
+	f(utils.Cfg)
+	utils.InvokeGlobalConfigListeners(old, utils.Cfg)
+}
+
+func (a *App) PersistConfig() {
+	utils.SaveConfig(a.ConfigFileName(), a.Config())
+}
+
+func (a *App) ReloadConfig() {
+	debug.FreeOSMemory()
+	utils.LoadGlobalConfig(a.ConfigFileName())
+
+	// start/restart email batching job if necessary
+	a.InitEmailBatching()
+}
+
+func (a *App) ConfigFileName() string {
+	return utils.CfgFileName
+}
+
+func (a *App) ClientConfig() map[string]string {
+	return a.clientConfig
+}
+
+func (a *App) ClientConfigHash() string {
+	return a.clientConfigHash
+}
+
+func (a *App) EnableConfigWatch() {
+	if a.configWatcher == nil && !a.disableConfigWatch {
+		configWatcher, err := utils.NewConfigWatcher(utils.CfgFileName)
+		if err != nil {
+			l4g.Error(err)
+		}
+		a.configWatcher = configWatcher
+	}
+}
+
+func (a *App) DisableConfigWatch() {
+	if a.configWatcher != nil {
+		a.configWatcher.Close()
+		a.configWatcher = nil
+	}
+}
+
+func (a *App) regenerateClientConfig() {
+	a.clientConfig = utils.GenerateClientConfig(a.Config(), a.DiagnosticId())
+	clientConfigJSON, _ := json.Marshal(a.clientConfig)
+	a.clientConfigHash = fmt.Sprintf("%x", md5.Sum(clientConfigJSON))
+}

app/options.go

@@ -35,3 +35,7 @@ func ConfigFile(file string) Option {
 		a.configFile = file
 	}
 }
+
+func DisableConfigWatch(a *App) {
+	a.disableConfigWatch = true
+}

cmd/platform/init.go

@@ -31,13 +31,13 @@ func initDBCommandContext(configFileLocation string) (*app.App, error) {
 	}
 	model.AppErrorInit(utils.T)
 
-	if err := utils.InitAndLoadConfig(configFileLocation); err != nil {
+	utils.ConfigureCmdLineLog()
+
+	a, err := app.New(app.ConfigFile(configFileLocation))
+	if err != nil {
 		return nil, err
 	}
 
-	utils.ConfigureCmdLineLog()
-
-	a := app.New(app.ConfigFile(configFileLocation))
 	if model.BuildEnterpriseReady == "true" {
 		a.LoadLicense()
 	}

cmd/platform/server.go

@@ -35,30 +35,26 @@ func runServerCmd(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	utils.CfgDisableConfigWatch, _ = cmd.Flags().GetBool("disableconfigwatch")
+	disableConfigWatch, _ := cmd.Flags().GetBool("disableconfigwatch")
 
-	runServer(config)
+	runServer(config, disableConfigWatch)
 	return nil
 }
 
-func runServer(configFileLocation string) {
+func runServer(configFileLocation string, disableConfigWatch bool) {
-	if err := utils.TranslationsPreInit(); err != nil {
+	options := []app.Option{app.ConfigFile(configFileLocation)}
-		l4g.Exit("Unable to load Mattermost configuration file: ", err)
+	if disableConfigWatch {
-		return
+		options = append(options, app.DisableConfigWatch)
-	}
-	model.AppErrorInit(utils.T)
-
-	if err := utils.InitAndLoadConfig(configFileLocation); err != nil {
-		l4g.Exit("Unable to load Mattermost configuration file: ", err)
-		return
 	}
 
-	if err := utils.InitTranslations(utils.Cfg.LocalizationSettings); err != nil {
+	a, err := app.New(options...)
-		l4g.Exit("Unable to load Mattermost translation files: %v", err)
+	if err != nil {
+		l4g.Error(err.Error())
 		return
 	}
+	defer a.Shutdown()
 
-	utils.TestConnection(utils.Cfg)
+	utils.TestConnection(a.Config())
 
 	pwd, _ := os.Getwd()
 	l4g.Info(utils.T("mattermost.current_version"), model.CurrentVersion, model.BuildNumber, model.BuildDate, model.BuildHash, model.BuildHashEnterprise)
@@ -66,15 +62,12 @@ func runServer(configFileLocation string) {
 	l4g.Info(utils.T("mattermost.working_dir"), pwd)
 	l4g.Info(utils.T("mattermost.config_file"), utils.FindConfigFile(configFileLocation))
 
-	a := app.New(app.ConfigFile(configFileLocation))
+	backend, appErr := a.FileBackend()
-	defer a.Shutdown()
+	if appErr == nil {
-
+		appErr = backend.TestConnection()
-	backend, err := a.FileBackend()
-	if err == nil {
-		err = backend.TestConnection()
 	}
-	if err != nil {
+	if appErr != nil {
-		l4g.Error("Problem with file storage settings: " + err.Error())
+		l4g.Error("Problem with file storage settings: " + appErr.Error())
 	}
 
 	if model.BuildEnterpriseReady == "true" {

utils/config.go

@@ -18,6 +18,7 @@ import (
 
 	l4g "github.com/alecthomas/log4go"
 	"github.com/fsnotify/fsnotify"
+	"github.com/pkg/errors"
 	"github.com/spf13/viper"
 
 	"net/http"
@@ -35,11 +36,9 @@ const (
 )
 
 var cfgMutex = &sync.Mutex{}
-var watcher *fsnotify.Watcher
 var Cfg *model.Config = &model.Config{}
 var CfgHash = ""
 var CfgFileName string = ""
-var CfgDisableConfigWatch = false
 var originalDisableDebugLvl l4g.Level = l4g.DEBUG
 var siteURL = ""
 
@@ -201,78 +200,61 @@ func SaveConfig(fileName string, config *model.Config) *model.AppError {
 	return nil
 }
 
-func InitializeConfigWatch() {
+type ConfigWatcher struct {
-	cfgMutex.Lock()
+	watcher *fsnotify.Watcher
-	defer cfgMutex.Unlock()
+	close   chan struct{}
+	closed  chan struct{}
+}
 
-	if CfgDisableConfigWatch {
+func NewConfigWatcher(cfgFileName string) (*ConfigWatcher, error) {
-		return
+	watcher, err := fsnotify.NewWatcher()
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to create config watcher for file: "+cfgFileName)
 	}
 
-	if watcher == nil {
+	configFile := filepath.Clean(cfgFileName)
-		var err error
+	configDir, _ := filepath.Split(configFile)
-		watcher, err = fsnotify.NewWatcher()
+	watcher.Add(configDir)
-		if err != nil {
-			l4g.Error(fmt.Sprintf("Failed to watch config file at %v with err=%v", CfgFileName, err.Error()))
-		}
 
-		go func() {
+	ret := &ConfigWatcher{
-			configFile := filepath.Clean(CfgFileName)
+		watcher: watcher,
+		close:   make(chan struct{}),
+		closed:  make(chan struct{}),
+	}
 
-			for {
+	go func() {
-				select {
+		defer close(ret.closed)
-				case event := <-watcher.Events:
+		defer watcher.Close()
-					// we only care about the config file
-					if filepath.Clean(event.Name) == configFile {
-						if event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create {
-							l4g.Info(fmt.Sprintf("Config file watcher detected a change reloading %v", CfgFileName))
 
-							if _, configReadErr := ReadConfigFile(CfgFileName, true); configReadErr == nil {
+		for {
-								LoadGlobalConfig(CfgFileName)
+			select {
-							} else {
+			case event := <-watcher.Events:
-								l4g.Error(fmt.Sprintf("Failed to read while watching config file at %v with err=%v", CfgFileName, configReadErr.Error()))
+				// we only care about the config file
-							}
+				if filepath.Clean(event.Name) == configFile {
+					if event.Op&fsnotify.Write == fsnotify.Write || event.Op&fsnotify.Create == fsnotify.Create {
+						l4g.Info(fmt.Sprintf("Config file watcher detected a change reloading %v", cfgFileName))
+
+						if _, configReadErr := ReadConfigFile(cfgFileName, true); configReadErr == nil {
+							LoadGlobalConfig(cfgFileName)
+						} else {
+							l4g.Error(fmt.Sprintf("Failed to read while watching config file at %v with err=%v", cfgFileName, configReadErr.Error()))
 						}
 					}
-				case err := <-watcher.Errors:
-					l4g.Error(fmt.Sprintf("Failed while watching config file at %v with err=%v", CfgFileName, err.Error()))
 				}
+			case err := <-watcher.Errors:
+				l4g.Error(fmt.Sprintf("Failed while watching config file at %v with err=%v", cfgFileName, err.Error()))
+			case <-ret.close:
+				return
 			}
-		}()
-	}
-}
-
-func EnableConfigWatch() {
-	cfgMutex.Lock()
-	defer cfgMutex.Unlock()
-
-	if watcher != nil {
-		configFile := filepath.Clean(CfgFileName)
-		configDir, _ := filepath.Split(configFile)
-
-		if watcher != nil {
-			watcher.Add(configDir)
 		}
-	}
+	}()
+
+	return ret, nil
 }
 
-func DisableConfigWatch() {
+func (w *ConfigWatcher) Close() {
-	cfgMutex.Lock()
+	close(w.close)
-	defer cfgMutex.Unlock()
+	<-w.closed
-
-	if watcher != nil {
-		configFile := filepath.Clean(CfgFileName)
-		configDir, _ := filepath.Split(configFile)
-		watcher.Remove(configDir)
-	}
-}
-
-func InitAndLoadConfig(filename string) error {
-	LoadGlobalConfig(filename)
-	InitializeConfigWatch()
-	EnableConfigWatch()
-
-	return nil
 }
 
 // ReadConfig reads and parses the given configuration.
@@ -337,26 +319,16 @@ func EnsureConfigFile(fileName string) (string, error) {
 	return "", fmt.Errorf("no config file found")
 }
 
-// LoadGlobalConfig will try to search around for the corresponding config file.  It will search
+// LoadConfig will try to search around for the corresponding config file.  It will search
 // /tmp/fileName then attempt ./config/fileName, then ../config/fileName and last it will look at
-// fileName
+// fileName.
-//
+func LoadConfig(fileName string) (configPath string, config *model.Config, appErr *model.AppError) {
-// XXX: This is deprecated.
-func LoadGlobalConfig(fileName string) *model.Config {
-	cfgMutex.Lock()
-	defer cfgMutex.Unlock()
-
-	// Cfg should never be null
-	oldConfig := *Cfg
-
-	var configPath string
 	if fileName != filepath.Base(fileName) {
 		configPath = fileName
 	} else {
 		if path, err := EnsureConfigFile(fileName); err != nil {
-			errMsg := T("utils.config.load_config.opening.panic", map[string]interface{}{"Filename": fileName, "Error": err.Error()})
+			appErr = model.NewAppError("LoadConfig", "utils.config.load_config.opening.panic", map[string]interface{}{"Filename": fileName, "Error": err.Error()}, "", 0)
-			fmt.Fprintln(os.Stderr, errMsg)
+			return
-			os.Exit(1)
 		} else {
 			configPath = path
 		}
@@ -364,40 +336,31 @@ func LoadGlobalConfig(fileName string) *model.Config {
 
 	config, err := ReadConfigFile(configPath, true)
 	if err != nil {
-		errMsg := T("utils.config.load_config.decoding.panic", map[string]interface{}{"Filename": fileName, "Error": err.Error()})
+		appErr = model.NewAppError("LoadConfig", "utils.config.load_config.decoding.panic", map[string]interface{}{"Filename": fileName, "Error": err.Error()}, "", 0)
-		fmt.Fprintln(os.Stderr, errMsg)
+		return
-		os.Exit(1)
 	}
 
-	CfgFileName = configPath
-
 	needSave := len(config.SqlSettings.AtRestEncryptKey) == 0 || len(*config.FileSettings.PublicLinkSalt) == 0 ||
 		len(config.EmailSettings.InviteSalt) == 0
 
 	config.SetDefaults()
 
 	if err := config.IsValid(); err != nil {
-		panic(err.Message)
+		return "", nil, err
 	}
 
 	if needSave {
-		cfgMutex.Unlock()
+		if err := SaveConfig(configPath, config); err != nil {
-		if err := SaveConfig(CfgFileName, config); err != nil {
 			l4g.Warn(err.Error())
 		}
-		cfgMutex.Lock()
 	}
 
 	if err := ValidateLocales(config); err != nil {
-		cfgMutex.Unlock()
+		if err := SaveConfig(configPath, config); err != nil {
-		if err := SaveConfig(CfgFileName, config); err != nil {
 			l4g.Warn(err.Error())
 		}
-		cfgMutex.Lock()
 	}
 
-	configureLog(&config.LogSettings)
-
 	if *config.FileSettings.DriverName == model.IMAGE_DRIVER_LOCAL {
 		dir := config.FileSettings.Directory
 		if len(dir) > 0 && dir[len(dir)-1:] != "/" {
@@ -405,6 +368,27 @@ func LoadGlobalConfig(fileName string) *model.Config {
 		}
 	}
 
+	return configPath, config, nil
+}
+
+// XXX: This is deprecated. Use LoadConfig instead if possible.
+func LoadGlobalConfig(fileName string) *model.Config {
+	configPath, config, err := LoadConfig(fileName)
+	if err != nil {
+		fmt.Fprintln(os.Stderr, err.SystemMessage(T))
+		os.Exit(1)
+	}
+
+	cfgMutex.Lock()
+	defer cfgMutex.Unlock()
+
+	CfgFileName = configPath
+
+	configureLog(&config.LogSettings)
+
+	// Cfg should never be null
+	oldConfig := *Cfg
+
 	Cfg = config
 	CfgHash = fmt.Sprintf("%x", md5.Sum([]byte(Cfg.ToJson())))
 

web/web_test.go

@@ -38,7 +38,10 @@ func StopTestStore() {
 }
 
 func Setup() *app.App {
-	a := app.New(app.StoreOverride(testStore))
+	a, err := app.New(app.StoreOverride(testStore), app.DisableConfigWatch)
+	if err != nil {
+		panic(err)
+	}
 	prevListenAddress := *a.Config().ServiceSettings.ListenAddress
 	a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" })
 	a.StartServer()