From 1d9efd0e39a9663bb2fbf52b3353fe21ac3b6954 Mon Sep 17 00:00:00 2001 From: Chris Date: Thu, 11 Jan 2018 15:23:41 -0600 Subject: Remove global config watcher (#8080) * remove global config watcher * keep config watcher disabled for tests * compile fix * fix resource leak --- api/apitestlib.go | 9 ++- api4/apitestlib.go | 9 ++- app/admin.go | 4 +- app/app.go | 62 +++++------------- app/app_test.go | 4 +- app/apptestlib.go | 9 ++- app/config.go | 73 ++++++++++++++++++++++ app/options.go | 4 ++ cmd/platform/init.go | 8 +-- cmd/platform/server.go | 39 +++++------- utils/config.go | 166 ++++++++++++++++++++++--------------------------- web/web_test.go | 5 +- 12 files changed, 216 insertions(+), 176 deletions(-) create mode 100644 app/config.go diff --git a/api/apitestlib.go b/api/apitestlib.go index 547071e93..7e3c10efc 100644 --- a/api/apitestlib.go +++ b/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() diff --git a/api4/apitestlib.go b/api4/apitestlib.go index eab27173f..4c0fbf509 100644 --- a/api4/apitestlib.go +++ b/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() diff --git a/app/admin.go b/app/admin.go index e46d9073c..e7d1eb9ed 100644 --- a/app/admin.go +++ b/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 { diff --git a/app/app.go b/app/app.go index 8aa894b9b..561942019 100644 --- a/app/app.go +++ b/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 } diff --git a/app/app_test.go b/app/app_test.go index 2058ddd79..fd24bdfd7 100644 --- a/app/app_test.go +++ b/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() diff --git a/app/apptestlib.go b/app/apptestlib.go index 912433290..1ec45f0fa 100644 --- a/app/apptestlib.go +++ b/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), } diff --git a/app/config.go b/app/config.go new file mode 100644 index 000000000..483804c99 --- /dev/null +++ b/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)) +} diff --git a/app/options.go b/app/options.go index 9b40806f3..464566024 100644 --- a/app/options.go +++ b/app/options.go @@ -35,3 +35,7 @@ func ConfigFile(file string) Option { a.configFile = file } } + +func DisableConfigWatch(a *App) { + a.disableConfigWatch = true +} diff --git a/cmd/platform/init.go b/cmd/platform/init.go index 7d5593821..ef3d78692 100644 --- a/cmd/platform/init.go +++ b/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() } diff --git a/cmd/platform/server.go b/cmd/platform/server.go index e240c6583..cdf12cf71 100644 --- a/cmd/platform/server.go +++ b/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) { - if err := utils.TranslationsPreInit(); err != nil { - l4g.Exit("Unable to load Mattermost configuration file: ", err) - return +func runServer(configFileLocation string, disableConfigWatch bool) { + options := []app.Option{app.ConfigFile(configFileLocation)} + if disableConfigWatch { + 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 { - l4g.Exit("Unable to load Mattermost translation files: %v", err) + a, err := app.New(options...) + 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)) - defer a.Shutdown() - - backend, err := a.FileBackend() - if err == nil { - err = backend.TestConnection() + backend, appErr := a.FileBackend() + if appErr == nil { + appErr = backend.TestConnection() } - if err != nil { - l4g.Error("Problem with file storage settings: " + err.Error()) + if appErr != nil { + l4g.Error("Problem with file storage settings: " + appErr.Error()) } if model.BuildEnterpriseReady == "true" { diff --git a/utils/config.go b/utils/config.go index d1522538d..a692d82d0 100644 --- a/utils/config.go +++ b/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() { - cfgMutex.Lock() - defer cfgMutex.Unlock() +type ConfigWatcher struct { + watcher *fsnotify.Watcher + close chan struct{} + closed chan struct{} +} - if CfgDisableConfigWatch { - return +func NewConfigWatcher(cfgFileName string) (*ConfigWatcher, error) { + watcher, err := fsnotify.NewWatcher() + if err != nil { + return nil, errors.Wrapf(err, "failed to create config watcher for file: "+cfgFileName) } - if watcher == nil { - var err error - watcher, err = fsnotify.NewWatcher() - if err != nil { - l4g.Error(fmt.Sprintf("Failed to watch config file at %v with err=%v", CfgFileName, err.Error())) - } + configFile := filepath.Clean(cfgFileName) + configDir, _ := filepath.Split(configFile) + watcher.Add(configDir) - go func() { - configFile := filepath.Clean(CfgFileName) - - for { - select { - case event := <-watcher.Events: - // 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())) - } - } - }() + ret := &ConfigWatcher{ + watcher: watcher, + close: make(chan struct{}), + closed: make(chan struct{}), } -} -func EnableConfigWatch() { - cfgMutex.Lock() - defer cfgMutex.Unlock() + go func() { + defer close(ret.closed) + defer watcher.Close() - if watcher != nil { - configFile := filepath.Clean(CfgFileName) - configDir, _ := filepath.Split(configFile) + for { + select { + case event := <-watcher.Events: + // 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 watcher != nil { - watcher.Add(configDir) + 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 <-ret.close: + return + } } - } -} - -func DisableConfigWatch() { - cfgMutex.Lock() - defer cfgMutex.Unlock() + }() - if watcher != nil { - configFile := filepath.Clean(CfgFileName) - configDir, _ := filepath.Split(configFile) - watcher.Remove(configDir) - } + return ret, nil } -func InitAndLoadConfig(filename string) error { - LoadGlobalConfig(filename) - InitializeConfigWatch() - EnableConfigWatch() - - return nil +func (w *ConfigWatcher) Close() { + close(w.close) + <-w.closed } // 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 -// -// 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 +// fileName. +func LoadConfig(fileName string) (configPath string, config *model.Config, appErr *model.AppError) { 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()}) - fmt.Fprintln(os.Stderr, errMsg) - os.Exit(1) + appErr = model.NewAppError("LoadConfig", "utils.config.load_config.opening.panic", map[string]interface{}{"Filename": fileName, "Error": err.Error()}, "", 0) + return } 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()}) - fmt.Fprintln(os.Stderr, errMsg) - os.Exit(1) + appErr = model.NewAppError("LoadConfig", "utils.config.load_config.decoding.panic", map[string]interface{}{"Filename": fileName, "Error": err.Error()}, "", 0) + return } - 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(CfgFileName, config); err != nil { + if err := SaveConfig(configPath, config); err != nil { l4g.Warn(err.Error()) } - cfgMutex.Lock() } if err := ValidateLocales(config); err != nil { - cfgMutex.Unlock() - if err := SaveConfig(CfgFileName, config); err != nil { + if err := SaveConfig(configPath, 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()))) diff --git a/web/web_test.go b/web/web_test.go index 54f2aad9b..21a7968b3 100644 --- a/web/web_test.go +++ b/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() -- cgit v1.2.3-1-g7c22