MM-20865 - Demo Plugin: Enabling and disabling demo plugin generates "connection is shutdown" error (#13604)

Automatic Merge

app/plugin_hooks_test.go

@@ -959,3 +959,74 @@ func TestHookContext(t *testing.T) {
 	_, err := th.App.CreatePost(post, th.BasicChannel, false)
 	require.Nil(t, err)
 }
+
+func TestActiveHooks(t *testing.T) {
+	th := Setup(t).InitBasic()
+	defer th.TearDown()
+
+	t.Run("", func(t *testing.T) {
+		tearDown, pluginIds, _ := SetAppEnvironmentWithPlugins(t,
+			[]string{
+				`
+			package main
+
+			import (
+				"github.com/mattermost/mattermost-server/v5/model"
+				"github.com/mattermost/mattermost-server/v5/plugin"
+			)
+
+			type MyPlugin struct {
+				plugin.MattermostPlugin
+			}
+
+			func (p *MyPlugin) OnActivate() error {
+				return nil
+			}
+
+			func (p *MyPlugin) OnConfigurationChange() error {
+				return nil
+			}
+
+			func (p *MyPlugin) UserHasBeenCreated(c *plugin.Context, user *model.User) {
+				user.Nickname = "plugin-callback-success"
+				p.API.UpdateUser(user)
+			}
+
+			func main() {
+				plugin.ClientMain(&MyPlugin{})
+			}
+		`}, th.App, th.App.NewPluginAPI)
+		defer tearDown()
+
+		require.Len(t, pluginIds, 1)
+		pluginId := pluginIds[0]
+
+		require.True(t, th.App.GetPluginsEnvironment().IsActive(pluginId))
+		user1 := &model.User{
+			Email:       model.NewId() + "success+test@example.com",
+			Nickname:    "Darth Vader1",
+			Username:    "vader" + model.NewId(),
+			Password:    "passwd1",
+			AuthService: "",
+		}
+		_, appErr := th.App.CreateUser(user1)
+		require.Nil(t, appErr)
+		time.Sleep(1 * time.Second)
+		user1, appErr = th.App.GetUser(user1.Id)
+		require.Nil(t, appErr)
+		require.Equal(t, "plugin-callback-success", user1.Nickname)
+
+		// Disable plugin
+		require.True(t, th.App.GetPluginsEnvironment().Deactivate(pluginId))
+		require.False(t, th.App.GetPluginsEnvironment().IsActive(pluginId))
+
+		hooks, err := th.App.GetPluginsEnvironment().HooksForPlugin(pluginId)
+		require.Error(t, err)
+		require.Nil(t, hooks)
+
+		// Should fail to find pluginId as it was deleted when deactivated
+		path, err := th.App.GetPluginsEnvironment().PublicFilesPath(pluginId)
+		require.Error(t, err)
+		require.Empty(t, path)
+	})
+}

plugin/environment.go

@@ -271,6 +271,11 @@ func (env *Environment) Activate(id string) (manifest *model.Manifest, activated
 		if err != nil {
 			return nil, false, errors.Wrapf(err, "unable to start plugin: %v", id)
 		}
+
+		if err := sup.Hooks().OnActivate(); err != nil {
+			sup.Shutdown()
+			return nil, false, err
+		}
 		rp.supervisor = sup
 		env.registeredPlugins.Store(id, rp)
 
@@ -313,6 +318,8 @@ func (env *Environment) Deactivate(id string) bool {
 		rp.supervisor.Shutdown()
 	}
 
+	env.registeredPlugins.Delete(id)
+
 	return true
 }
 
@@ -333,7 +340,7 @@ func (env *Environment) Shutdown() {
 	env.registeredPlugins.Range(func(key, value interface{}) bool {
 		rp := value.(registeredPlugin)
 
-		if rp.supervisor == nil {
+		if rp.supervisor == nil || !env.IsActive(rp.BundleInfo.Manifest.Id) {
 			return true
 		}
 
@@ -434,7 +441,7 @@ func (env *Environment) UnpackWebappBundle(id string) (*model.Manifest, error) {
 func (env *Environment) HooksForPlugin(id string) (Hooks, error) {
 	if p, ok := env.registeredPlugins.Load(id); ok {
 		rp := p.(registeredPlugin)
-		if rp.supervisor != nil {
+		if rp.supervisor != nil && env.IsActive(id) {
 			return rp.supervisor.Hooks(), nil
 		}
 	}
@@ -442,7 +449,7 @@ func (env *Environment) HooksForPlugin(id string) (Hooks, error) {
 	return nil, fmt.Errorf("plugin not found: %v", id)
 }
 
-// RunMultiPluginHook invokes hookRunnerFunc for each plugin that implements the given hookId.
+// RunMultiPluginHook invokes hookRunnerFunc for each active plugin that implements the given hookId.
 //
 // If hookRunnerFunc returns false, iteration will not continue. The iteration order among active
 // plugins is not specified.
@@ -450,14 +457,11 @@ func (env *Environment) RunMultiPluginHook(hookRunnerFunc func(hooks Hooks) bool
 	env.registeredPlugins.Range(func(key, value interface{}) bool {
 		rp := value.(registeredPlugin)
 
-		if rp.supervisor == nil || !rp.supervisor.Implements(hookId) {
+		if rp.supervisor == nil || !rp.supervisor.Implements(hookId) || !env.IsActive(rp.BundleInfo.Manifest.Id) {
 			return true
 		}
-		if !hookRunnerFunc(rp.supervisor.Hooks()) {
-			return false
-		}
 
-		return true
+		return hookRunnerFunc(rp.supervisor.Hooks())
 	})
 }
 

plugin/supervisor.go

@@ -92,11 +92,6 @@ func newSupervisor(pluginInfo *model.BundleInfo, parentLogger *mlog.Logger, apiI
 		}
 	}
 
-	err = sup.Hooks().OnActivate()
-	if err != nil {
-		return nil, err
-	}
-
 	return &sup, nil
 }