From 835c0871a01d9c0460ee34e22e9dfc53a69a2ac9 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 27 Jul 2018 11:37:17 -0400 Subject: MM-11431: handle plugin deadlocks (#9167) * ensure plugin is always shutdown Once we call `.client.Client()` the plugin has started, and must be shut down. `newSupervisor` sometimes returned with an error (and without a reference to the supervisor), leaving the client running indefinitely. * Clarify the documentation to explain that plugin hooks will not trigger until `OnActivate` returns successfully, and will stop triggering just before `OnDeactivate` is called. * test for plugin deadlock * plugin/environment.go: switch to sync.Map From: https://golang.org/pkg/sync/#Map > If a goroutine holds a RWMutex for reading and another goroutine might call Lock, no goroutine should expect to be able to acquire a read lock until the initial read lock is released. In particular, this prohibits recursive read locking. This is to ensure that the lock eventually becomes available; a blocked Lock call excludes new readers from acquiring the lock. The previous `RWMutex` was not safe given that we effectively acquired read locks recursively (hook -> api -> hook). This worked up until we activated or deactivated plugins, tried to acquire a write lock, and the plugin used the API to effectively trigger another hook. Switching to sync.Map avoids this by divesting the need to lock at all, avoiding the potential for a recursive lock in the first place. --- app/plugin_deadlock_test.go | 211 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 211 insertions(+) create mode 100644 app/plugin_deadlock_test.go (limited to 'app/plugin_deadlock_test.go') diff --git a/app/plugin_deadlock_test.go b/app/plugin_deadlock_test.go new file mode 100644 index 000000000..381206943 --- /dev/null +++ b/app/plugin_deadlock_test.go @@ -0,0 +1,211 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package app + +import ( + "os" + "strings" + "testing" + "text/template" + "time" + + "github.com/mattermost/mattermost-server/model" +) + +func TestPluginDeadlock(t *testing.T) { + t.Run("Single Plugin", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + 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) OnActivate() error { + _, err := p.API.CreatePost(&model.Post{ + UserId: "{{.User.Id}}", + ChannelId: "{{.Channel.Id}}", + Message: "message", + }) + if err != nil { + panic(err.Error()) + } + + return nil + } + + func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) { + if _, from_plugin := post.Props["from_plugin"]; from_plugin { + return post, "" + } + + p.API.CreatePost(&model.Post{ + UserId: "{{.User.Id}}", + ChannelId: "{{.Channel.Id}}", + Message: "message", + Props: map[string]interface{}{ + "from_plugin": true, + }, + }) + + return post, "" + } + + 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() { + SetAppEnvironmentWithPlugins(t, plugins, th.App, th.App.NewPluginAPI) + close(done) + }() + + select { + case <-done: + case <-time.After(30 * time.Second): + t.Fatal("plugin failed to activate: likely deadlocked") + go func() { + time.Sleep(5 * time.Second) + os.Exit(1) + }() + } + }) + + t.Run("Multiple Plugins", func(t *testing.T) { + th := Setup().InitBasic() + defer th.TearDown() + + pluginPostOnHasBeenPosted := template.Must(template.New("pluginPostOnHasBeenPosted").Parse(` + package main + + import ( + "github.com/mattermost/mattermost-server/plugin" + "github.com/mattermost/mattermost-server/model" + ) + + type MyPlugin struct { + plugin.MattermostPlugin + } + + func (p *MyPlugin) MessageWillBePosted(c *plugin.Context, post *model.Post) (*model.Post, string) { + if _, from_plugin := post.Props["from_plugin"]; from_plugin { + return post, "" + } + + p.API.CreatePost(&model.Post{ + UserId: "{{.User.Id}}", + ChannelId: "{{.Channel.Id}}", + Message: "message", + Props: map[string]interface{}{ + "from_plugin": true, + }, + }) + + return post, "" + } + + func main() { + plugin.ClientMain(&MyPlugin{}) + } +`, + )) + + 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) OnActivate() error { + _, err := p.API.CreatePost(&model.Post{ + UserId: "{{.User.Id}}", + ChannelId: "{{.Channel.Id}}", + Message: "message", + }) + if err != nil { + panic(err.Error()) + } + + return nil + } + + func main() { + plugin.ClientMain(&MyPlugin{}) + } +`, + )) + + templateData := struct { + User *model.User + Channel *model.Channel + }{ + th.BasicUser, + th.BasicChannel, + } + + plugins := []string{} + pluginTemplates := []*template.Template{ + pluginPostOnHasBeenPosted, + pluginPostOnActivate, + } + for _, pluginTemplate := range pluginTemplates { + b := &strings.Builder{} + pluginTemplate.Execute(b, templateData) + + plugins = append(plugins, b.String()) + } + + done := make(chan bool) + go func() { + SetAppEnvironmentWithPlugins(t, plugins, th.App, th.App.NewPluginAPI) + close(done) + }() + + select { + case <-done: + case <-time.After(30 * time.Second): + t.Fatal("plugin failed to activate: likely deadlocked") + go func() { + time.Sleep(5 * time.Second) + os.Exit(1) + }() + } + }) +} -- cgit v1.2.3-1-g7c22