diff --git a/app/command.go b/app/command.go index a69d8a3637..0d44b5dd1c 100644 --- a/app/command.go +++ b/app/command.go @@ -187,7 +187,7 @@ func (a *App) ExecuteCommand(args *model.CommandArgs) (*model.CommandResponse, * return nil, model.NewAppError("command", "api.command.execute_command.not_found.app_error", map[string]interface{}{"Trigger": trigger}, "", http.StatusNotFound) } -// tryExecutePluginCommand attempts to run a built in command based on the given arguments. If no such command can be +// tryExecuteBuiltInCommand attempts to run a built in command based on the given arguments. If no such command can be // found, returns nil for all arguments. func (a *App) tryExecuteBuiltInCommand(args *model.CommandArgs, trigger string, message string) (*model.Command, *model.CommandResponse) { provider := GetCommandProvider(trigger) diff --git a/app/plugin_commands.go b/app/plugin_commands.go index 46842451d9..8e1df5fa42 100644 --- a/app/plugin_commands.go +++ b/app/plugin_commands.go @@ -97,20 +97,29 @@ func (a *App) tryExecutePluginCommand(args *model.CommandArgs) (*model.Command, trigger := parts[0][1:] trigger = strings.ToLower(trigger) + var matched *PluginCommand a.Srv.pluginCommandsLock.RLock() - defer a.Srv.pluginCommandsLock.RUnlock() - for _, pc := range a.Srv.pluginCommands { if (pc.Command.TeamId == "" || pc.Command.TeamId == args.TeamId) && pc.Command.Trigger == trigger { - if pluginsEnvironment := a.GetPluginsEnvironment(); pluginsEnvironment != nil { - pluginHooks, err := pluginsEnvironment.HooksForPlugin(pc.PluginId) - if err != nil { - return pc.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command.error.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) - } - response, appErr := pluginHooks.ExecuteCommand(a.PluginContext(), args) - return pc.Command, response, appErr - } + matched = pc + break } } - return nil, nil, nil + a.Srv.pluginCommandsLock.RUnlock() + if matched == nil { + return nil, nil, nil + } + + pluginsEnvironment := a.GetPluginsEnvironment() + if pluginsEnvironment == nil { + return nil, nil, nil + } + + pluginHooks, err := pluginsEnvironment.HooksForPlugin(matched.PluginId) + if err != nil { + return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command.error.app_error", nil, "err="+err.Error(), http.StatusInternalServerError) + } + + response, appErr := pluginHooks.ExecuteCommand(a.PluginContext(), args) + return matched.Command, response, appErr } diff --git a/app/plugin_commands_test.go b/app/plugin_commands_test.go index 830ef48980..06af46d1cd 100644 --- a/app/plugin_commands_test.go +++ b/app/plugin_commands_test.go @@ -5,6 +5,7 @@ package app import ( "testing" + "time" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" @@ -46,7 +47,7 @@ func TestPluginCommand(t *testing.T) { } type MyPlugin struct { - plugin.MattermostPlugin + plugin.MattermostPlugin configuration configuration } @@ -110,6 +111,110 @@ func TestPluginCommand(t *testing.T) { th.App.RemovePlugin(pluginIds[0]) }) + t.Run("re-entrant command registration on config change", func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { + cfg.PluginSettings.Plugins["testloadpluginconfig"] = map[string]interface{}{ + "TeamId": args.TeamId, + } + }) + + tearDown, pluginIds, activationErrors := SetAppEnvironmentWithPlugins(t, []string{` + package main + + import ( + "github.com/mattermost/mattermost-server/plugin" + "github.com/mattermost/mattermost-server/model" + ) + + type configuration struct { + TeamId string + } + + type MyPlugin struct { + plugin.MattermostPlugin + + configuration configuration + } + + func (p *MyPlugin) OnConfigurationChange() error { + p.API.LogInfo("OnConfigurationChange") + err := p.API.LoadPluginConfiguration(&p.configuration); + if err != nil { + return err + } + + p.API.LogInfo("About to register") + err = p.API.RegisterCommand(&model.Command{ + TeamId: p.configuration.TeamId, + Trigger: "plugin", + DisplayName: "Plugin Command", + AutoComplete: true, + AutoCompleteDesc: "autocomplete", + }) + if err != nil { + p.API.LogInfo("Registered, with error", err, err.Error()) + return err + } + p.API.LogInfo("Registered, without error") + return nil + } + + func (p *MyPlugin) ExecuteCommand(c *plugin.Context, commandArgs *model.CommandArgs) (*model.CommandResponse, *model.AppError) { + p.API.LogInfo("ExecuteCommand") + // Saving the plugin config eventually results in a call to + // OnConfigurationChange. This used to deadlock on account of + // effectively acquiring a RWLock reentrantly. + err := p.API.SavePluginConfig(map[string]interface{}{ + "TeamId": p.configuration.TeamId, + }) + if err != nil { + p.API.LogError("Failed to save plugin config", err, err.Error()) + return nil, err + } + p.API.LogInfo("ExecuteCommand, saved plugin config") + + return &model.CommandResponse{ + ResponseType: model.COMMAND_RESPONSE_TYPE_EPHEMERAL, + Text: "text", + }, nil + } + + func main() { + plugin.ClientMain(&MyPlugin{}) + } + `}, th.App, th.App.NewPluginAPI) + defer tearDown() + + require.Len(t, activationErrors, 1) + require.Nil(t, nil, activationErrors[0]) + + wait := make(chan bool) + killed := false + go func() { + defer close(wait) + + resp, err := th.App.ExecuteCommand(args) + + // Ignore if we kill below. + if !killed { + require.Nil(t, err) + require.Equal(t, model.COMMAND_RESPONSE_TYPE_EPHEMERAL, resp.ResponseType) + require.Equal(t, "text", resp.Text) + } + }() + + select { + case <-wait: + case <-time.After(10 * time.Second): + killed = true + } + + th.App.RemovePlugin(pluginIds[0]) + if killed { + t.Fatal("execute command appears to have deadlocked") + } + }) + t.Run("error after plugin command unregistered", func(t *testing.T) { _, err := th.App.ExecuteCommand(args) require.NotNil(t, err) diff --git a/app/plugin_hooks_test.go b/app/plugin_hooks_test.go index 87398ed409..df88b99b9b 100644 --- a/app/plugin_hooks_test.go +++ b/app/plugin_hooks_test.go @@ -46,6 +46,12 @@ func SetAppEnvironmentWithPlugins(t *testing.T, pluginCode []string, app *App, a _, _, activationErr := env.Activate(pluginId) pluginIds = append(pluginIds, pluginId) activationErrors = append(activationErrors, activationErr) + + app.UpdateConfig(func(cfg *model.Config) { + cfg.PluginSettings.PluginStates[pluginId] = &model.PluginState{ + Enable: true, + } + }) } return func() {