From 83b3090eb967da1076de360ab2330e5e9655b810 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 4 Nov 2021 08:54:03 +0530 Subject: [PATCH] Fix PluginHealthCheckJob (#18927) * Fix PluginHealthCheckJob We were incorrectly calling to start PluginHealthCheckJob without initializing the plugins environment. To fix this, we move the code inside initPlugins right after the environment is initialized. To respond to config changes, we call it again from pluginsEnvironment.Shutdown which gets called from ConfigListener when plugins are disabled. And initPlugins is anyways called again from ConfigListener which plugins are enabled, so we can avoid checking for PluginSettings.Enable again in the call. We also rename the method to better indicate its nature. During this, we also uncover and fix another bug where disabling plugins would not shut down plugins at all because we were calling s.GetPluginsEnvironment() directly which returns nil if plugins were disabled. The approach we follow is to manually acquire the lock whenever we need access to the struct ignoring config setting. We fix that as well. https://community-daily.mattermost.com/boards/workspace/zyoahc9uapdn3xdptac6jb69ic/285b80a3-257d-41f6-8cf4-ed80ca9d92e5/495cdb4d-c13a-4992-8eb9-80cfee2819a4?c=6ef6178c-3512-4e57-8edd-1d2b66a09c9e ```release-note NONE ``` * Fix test ```release-note NONE ``` --- app/plugin.go | 8 +++++++- app/server.go | 14 -------------- plugin/environment.go | 8 +++----- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/app/plugin.go b/app/plugin.go index e9d70a5d77..789ebe2364 100644 --- a/app/plugin.go +++ b/app/plugin.go @@ -180,6 +180,7 @@ func (s *Server) initPlugins(c *request.Context, pluginDir, webappPluginDir stri s.PluginsLock.RUnlock() if pluginsEnvironment != nil || !*s.Config().PluginSettings.Enable { s.syncPluginsActiveState() + pluginsEnvironment.TogglePluginHealthCheckJob(*s.Config().PluginSettings.EnableHealthCheck) return } @@ -208,6 +209,8 @@ func (s *Server) initPlugins(c *request.Context, pluginDir, webappPluginDir stri s.PluginsEnvironment = env s.PluginsLock.Unlock() + s.PluginsEnvironment.TogglePluginHealthCheckJob(*s.Config().PluginSettings.EnableHealthCheck) + if err := s.syncPlugins(); err != nil { mlog.Error("Failed to sync plugins from the file store", mlog.Err(err)) } @@ -327,7 +330,10 @@ func (s *Server) syncPlugins() *model.AppError { } func (s *Server) ShutDownPlugins() { - pluginsEnvironment := s.GetPluginsEnvironment() + // Acquiring lock manually, as plugins might be disabled. See GetPluginsEnvironment. + s.PluginsLock.RLock() + pluginsEnvironment := s.PluginsEnvironment + s.PluginsLock.RUnlock() if pluginsEnvironment == nil { return } diff --git a/app/server.go b/app/server.go index 2e961188e5..b973e0cc42 100644 --- a/app/server.go +++ b/app/server.go @@ -571,20 +571,6 @@ func NewServer(options ...Option) (*Server, error) { s.EmailService.InitEmailBatching() }) - // Start plugin health check job - pluginsEnvironment := s.PluginsEnvironment - if pluginsEnvironment != nil { - pluginsEnvironment.InitPluginHealthCheckJob(*s.Config().PluginSettings.Enable && *s.Config().PluginSettings.EnableHealthCheck) - } - s.AddConfigListener(func(_, c *model.Config) { - s.PluginsLock.RLock() - pluginsEnvironment := s.PluginsEnvironment - s.PluginsLock.RUnlock() - if pluginsEnvironment != nil { - pluginsEnvironment.InitPluginHealthCheckJob(*s.Config().PluginSettings.Enable && *c.PluginSettings.EnableHealthCheck) - } - }) - logCurrentVersion := fmt.Sprintf("Current version is %v (%v/%v/%v/%v)", model.CurrentVersion, model.BuildNumber, model.BuildDate, model.BuildHash, model.BuildHashEnterprise) mlog.Info( logCurrentVersion, diff --git a/plugin/environment.go b/plugin/environment.go index 1071e735af..70c738733a 100644 --- a/plugin/environment.go +++ b/plugin/environment.go @@ -332,9 +332,7 @@ func (env *Environment) RestartPlugin(id string) error { // Shutdown deactivates all plugins and gracefully shuts down the environment. func (env *Environment) Shutdown() { - if env.pluginHealthCheckJob != nil { - env.pluginHealthCheckJob.Cancel() - } + env.TogglePluginHealthCheckJob(false) var wg sync.WaitGroup env.registeredPlugins.Range(func(key, value interface{}) bool { @@ -507,8 +505,8 @@ func newRegisteredPlugin(bundle *model.BundleInfo) registeredPlugin { return registeredPlugin{State: state, BundleInfo: bundle} } -// InitPluginHealthCheckJob starts a new job if one is not running and is set to enabled, or kills an existing one if set to disabled. -func (env *Environment) InitPluginHealthCheckJob(enable bool) { +// TogglePluginHealthCheckJob starts a new job if one is not running and is set to enabled, or kills an existing one if set to disabled. +func (env *Environment) TogglePluginHealthCheckJob(enable bool) { // Config is set to enable. No job exists, start a new job. if enable && env.pluginHealthCheckJob == nil { mlog.Debug("Enabling plugin health check job", mlog.Duration("interval_s", HealthCheckInterval))