diff --git a/app/plugin.go b/app/plugin.go index c1d67a44fc..e543a7de51 100644 --- a/app/plugin.go +++ b/app/plugin.go @@ -43,6 +43,7 @@ func (a *App) SetPluginsEnvironment(pluginsEnvironment *plugin.Environment) { } func (a *App) SyncPluginsActiveState() { + // Acquiring lock manually, as plugins might be disabled. See GetPluginsEnvironment. a.Srv.PluginsLock.RLock() pluginsEnvironment := a.Srv.PluginsEnvironment a.Srv.PluginsLock.RUnlock() @@ -124,6 +125,7 @@ func (a *App) NewPluginAPI(manifest *model.Manifest) plugin.API { } func (a *App) InitPlugins(pluginDir, webappPluginDir string) { + // Acquiring lock manually, as plugins might be disabled. See GetPluginsEnvironment. a.Srv.PluginsLock.RLock() pluginsEnvironment := a.Srv.PluginsEnvironment a.Srv.PluginsLock.RUnlock() @@ -257,9 +259,7 @@ func (a *App) SyncPlugins() *model.AppError { } func (a *App) ShutDownPlugins() { - a.Srv.PluginsLock.Lock() - pluginsEnvironment := a.Srv.PluginsEnvironment - defer a.Srv.PluginsLock.Unlock() + pluginsEnvironment := a.GetPluginsEnvironment() if pluginsEnvironment == nil { return } @@ -270,7 +270,15 @@ func (a *App) ShutDownPlugins() { a.RemoveConfigListener(a.Srv.PluginConfigListenerId) a.Srv.PluginConfigListenerId = "" - a.Srv.PluginsEnvironment = nil + + // Acquiring lock manually before cleaning up PluginsEnvironment. + a.Srv.PluginsLock.Lock() + defer a.Srv.PluginsLock.Unlock() + if a.Srv.PluginsEnvironment == pluginsEnvironment { + a.Srv.PluginsEnvironment = nil + } else { + mlog.Warn("Another PluginsEnvironment detected while shutting down plugins.") + } } func (a *App) GetActivePluginManifests() ([]*model.Manifest, *model.AppError) { diff --git a/app/plugin_deadlock_test.go b/app/plugin_deadlock_test.go index ae777e34f9..0eb28c5e16 100644 --- a/app/plugin_deadlock_test.go +++ b/app/plugin_deadlock_test.go @@ -4,13 +4,14 @@ package app import ( - "github.com/stretchr/testify/require" "os" "strings" "testing" "text/template" "time" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/model" ) @@ -209,4 +210,112 @@ func TestPluginDeadlock(t *testing.T) { }() } }) + + t.Run("CreatePost on OnDeactivate Plugin", func(t *testing.T) { + th := Setup(t).InitBasic() + + pluginPostOnActivate := template.Must(template.New("pluginPostOnActivate").Parse(` + package main + + import ( + "github.com/mattermost/mattermost-server/plugin" + "github.com/mattermost/mattermost-server/model" + ) + + type MyPlugin struct { + plugin.MattermostPlugin + } + + func (p *MyPlugin) OnDeactivate() error { + _, err := p.API.CreatePost(&model.Post{ + UserId: "{{.User.Id}}", + ChannelId: "{{.Channel.Id}}", + Message: "OnDeactivate", + }) + if err != nil { + panic(err.Error()) + } + + return nil + } + + func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) { + updatedPost := &model.Post{ + UserId: "{{.User.Id}}", + ChannelId: "{{.Channel.Id}}", + Message: "messageUpdated", + Props: map[string]interface{}{ + "from_plugin": true, + }, + } + + return updatedPost, "" + } + + func main() { + plugin.ClientMain(&MyPlugin{}) + } +`, + )) + + templateData := struct { + User *model.User + Channel *model.Channel + }{ + th.BasicUser, + th.BasicChannel, + } + + plugins := []string{} + pluginTemplates := []*template.Template{ + pluginPostOnActivate, + } + for _, pluginTemplate := range pluginTemplates { + b := &strings.Builder{} + pluginTemplate.Execute(b, templateData) + + plugins = append(plugins, b.String()) + } + + done := make(chan bool) + go func() { + posts, appErr := th.App.GetPosts(th.BasicChannel.Id, 0, 2) + require.Nil(t, appErr) + require.NotNil(t, posts) + + messageWillBePostedCalled := false + for _, p := range posts.Posts { + if p.Message == "messageUpdated" { + messageWillBePostedCalled = true + } + } + require.False(t, messageWillBePostedCalled, "MessageWillBePosted should not have been called") + + SetAppEnvironmentWithPlugins(t, plugins, th.App, th.App.NewPluginAPI) + th.TearDown() + + posts, appErr = th.App.GetPosts(th.BasicChannel.Id, 0, 2) + require.Nil(t, appErr) + require.NotNil(t, posts) + + messageWillBePostedCalled = false + for _, p := range posts.Posts { + if p.Message == "messageUpdated" { + messageWillBePostedCalled = true + } + } + require.True(t, messageWillBePostedCalled, "MessageWillBePosted was not called on deactivate") + close(done) + }() + + select { + case <-done: + case <-time.After(30 * time.Second): + require.Fail(t, "plugin failed to activate: likely deadlocked") + go func() { + time.Sleep(5 * time.Second) + os.Exit(1) + }() + } + }) }