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.

1 files changed, 211 insertions, 0 deletions

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)
+			}()
+		}
+	})
+}