MM-18540 - Demo plugin crashes on server shutdown (#12371)

* MM-18540 - Demo plugin crashes on server shutdown

* MM-18540 - Removed unncessary lock statements

* Added comments, logging a warning if PluginsEnv is updated while shutting down plugins

* Don't clean up PluginsEnv if another Env is detected

* Changing warn to debug

* Revert "Changing warn to debug"

This reverts commit 46f20ab21eeabb01d07f53e02d850cd6d9b2b837.

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.GetPluginsEnvironment()
-	pluginsEnvironment := a.Srv.PluginsEnvironment
-	defer a.Srv.PluginsLock.Unlock()
 	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) {

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