From 2f47cf59948a95e2c28e6d4fbb718ed957b50c0d Mon Sep 17 00:00:00 2001 From: Ashim Sedhain <38435962+asimsedhain@users.noreply.github.com> Date: Fri, 2 Oct 2020 03:02:58 -0500 Subject: [PATCH] [MM-24753] Improves error message when plugin crashes during slash command (#15334) Co-authored-by: Ali Farooq Co-authored-by: Ben Schumacher --- app/plugin_commands.go | 12 +++++ app/plugin_commands_test.go | 90 +++++++++++++++++++++++++++++++++++++ i18n/en.json | 8 ++++ plugin/environment.go | 4 +- plugin/health_check.go | 2 +- 5 files changed, 113 insertions(+), 3 deletions(-) diff --git a/app/plugin_commands.go b/app/plugin_commands.go index 0153c8004c..cbf6069001 100644 --- a/app/plugin_commands.go +++ b/app/plugin_commands.go @@ -4,6 +4,7 @@ package app import ( + "fmt" "net/http" "net/url" "strings" @@ -136,6 +137,11 @@ func (a *App) tryExecutePluginCommand(args *model.CommandArgs) (*model.Command, return nil, nil, nil } + // Checking if plugin is working or not + if err := pluginsEnvironment.PerformHealthCheck(matched.PluginId); err != nil { + return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command_error.error.app_error", map[string]interface{}{"Command": trigger}, "err= Plugin has recently crashed: "+matched.PluginId, http.StatusInternalServerError) + } + 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) @@ -150,5 +156,11 @@ func (a *App) tryExecutePluginCommand(args *model.CommandArgs) (*model.Command, } response, appErr := pluginHooks.ExecuteCommand(a.PluginContext(), args) + + // Checking if plugin crashed after running the command + if err := pluginsEnvironment.PerformHealthCheck(matched.PluginId); err != nil { + errMessage := fmt.Sprintf("err= Plugin %s crashed due to /%s command", matched.PluginId, trigger) + return matched.Command, nil, model.NewAppError("ExecutePluginCommand", "model.plugin_command_crash.error.app_error", map[string]interface{}{"Command": trigger, "PluginId": matched.PluginId}, errMessage, http.StatusInternalServerError) + } return matched.Command, response, appErr } diff --git a/app/plugin_commands_test.go b/app/plugin_commands_test.go index 26c0c8339a..635c316f8a 100644 --- a/app/plugin_commands_test.go +++ b/app/plugin_commands_test.go @@ -286,4 +286,94 @@ func TestPluginCommand(t *testing.T) { th.App.RemovePlugin(pluginIds[0]) }) + t.Run("plugin has crashed before execution of command", func(t *testing.T) { + tearDown, pluginIds, activationErrors := SetAppEnvironmentWithPlugins(t, []string{` + package main + + import ( + "github.com/mattermost/mattermost-server/v5/plugin" + "github.com/mattermost/mattermost-server/v5/model" + ) + + type MyPlugin struct { + plugin.MattermostPlugin + + } + + func (p *MyPlugin) OnActivate() error { + err := p.API.RegisterCommand(&model.Command{ + Trigger: "code", + }) + if err != nil { + p.API.LogError("error", "err", err) + } + panic("Uncaught Error") + + return err + } + + func (p *MyPlugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) { + return &model.CommandResponse{}, nil + } + + func main() { + plugin.ClientMain(&MyPlugin{}) + } + `}, th.App, th.App.NewPluginAPI) + defer tearDown() + require.Len(t, activationErrors, 1) + require.Nil(t, nil, activationErrors[0]) + args.Command = "/code" + resp, err := th.App.ExecuteCommand(args) + require.Nil(t, resp) + require.NotNil(t, err) + require.Equal(t, err.Id, "model.plugin_command_error.error.app_error") + th.App.RemovePlugin(pluginIds[0]) + }) + + t.Run("plugin has crashed due to the execution of the command", func(t *testing.T) { + tearDown, pluginIds, activationErrors := SetAppEnvironmentWithPlugins(t, []string{` + package main + + import ( + "github.com/mattermost/mattermost-server/v5/plugin" + "github.com/mattermost/mattermost-server/v5/model" + ) + + type MyPlugin struct { + plugin.MattermostPlugin + + } + + func (p *MyPlugin) OnActivate() error { + err := p.API.RegisterCommand(&model.Command{ + Trigger: "code", + }) + if err != nil { + p.API.LogError("error", "err", err) + } + + return err + } + + func (p *MyPlugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) { + panic("Uncaught Error") + return &model.CommandResponse{}, nil + } + + func main() { + plugin.ClientMain(&MyPlugin{}) + } + `}, th.App, th.App.NewPluginAPI) + defer tearDown() + require.Len(t, activationErrors, 1) + require.Nil(t, nil, activationErrors[0]) + args.Command = "/code" + resp, err := th.App.ExecuteCommand(args) + require.Nil(t, resp) + require.NotNil(t, err) + require.Equal(t, err.Id, "model.plugin_command_crash.error.app_error") + th.App.RemovePlugin(pluginIds[0]) + }) + } diff --git a/i18n/en.json b/i18n/en.json index 6a15967ce6..64f249419a 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -7302,6 +7302,14 @@ "id": "model.plugin_command.error.app_error", "translation": "An error occurred while trying to execute this command." }, + { + "id": "model.plugin_command_crash.error.app_error", + "translation": "/{{.Command}} command crashed the {{.PluginId}} plugin. Please contact your system administrator" + }, + { + "id": "model.plugin_command_error.error.app_error", + "translation": "Plugin for /{{.Command}} is not working. Please contact your system administrator" + }, { "id": "model.plugin_key_value.is_valid.key.app_error", "translation": "Invalid key, must be more than {{.Min}} and a of maximum {{.Max}} characters long." diff --git a/plugin/environment.go b/plugin/environment.go index 284d63f117..e0f968f9cf 100644 --- a/plugin/environment.go +++ b/plugin/environment.go @@ -472,8 +472,8 @@ func (env *Environment) RunMultiPluginHook(hookRunnerFunc func(hooks Hooks) bool } } -// performHealthCheck uses the active plugin's supervisor to verify if the plugin has crashed. -func (env *Environment) performHealthCheck(id string) error { +// PerformHealthCheck uses the active plugin's supervisor to verify if the plugin has crashed. +func (env *Environment) PerformHealthCheck(id string) error { p, ok := env.registeredPlugins.Load(id) if !ok { return nil diff --git a/plugin/health_check.go b/plugin/health_check.go index 91978b781d..a620cd1842 100644 --- a/plugin/health_check.go +++ b/plugin/health_check.go @@ -51,7 +51,7 @@ func (job *PluginHealthCheckJob) run() { // If the plugin passes the health check, do nothing. // If the plugin fails the health check, the function either restarts or deactivates the plugin, based on the quantity and frequency of its failures. func (job *PluginHealthCheckJob) CheckPlugin(id string) { - err := job.env.performHealthCheck(id) + err := job.env.PerformHealthCheck(id) if err == nil { return }