From 3c7b40063dbba777d27efd2fa4c7093d5507dca6 Mon Sep 17 00:00:00 2001 From: Chris Date: Tue, 5 Dec 2017 08:19:32 -0600 Subject: call OnActivate after plugin crash, update example (#7940) --- plugin/example_hello_user_test.go | 3 ++- plugin/pluginenv/environment.go | 6 +----- plugin/pluginenv/environment_test.go | 26 ++++++------------------ plugin/rpcplugin/supervisor.go | 15 +++++++++----- plugin/rpcplugin/supervisor_test.go | 39 ++++++++++++++++++++++++++++-------- plugin/supervisor.go | 2 +- 6 files changed, 51 insertions(+), 40 deletions(-) (limited to 'plugin') diff --git a/plugin/example_hello_user_test.go b/plugin/example_hello_user_test.go index 4aefbc5f5..989cca0f2 100644 --- a/plugin/example_hello_user_test.go +++ b/plugin/example_hello_user_test.go @@ -12,9 +12,10 @@ type HelloUserPlugin struct { api plugin.API } -func (p *HelloUserPlugin) OnActivate(api plugin.API) { +func (p *HelloUserPlugin) OnActivate(api plugin.API) error { // Just save api for later when we need to look up users. p.api = api + return nil } func (p *HelloUserPlugin) ServeHTTP(w http.ResponseWriter, r *http.Request) { diff --git a/plugin/pluginenv/environment.go b/plugin/pluginenv/environment.go index 62eb20e9d..26511c651 100644 --- a/plugin/pluginenv/environment.go +++ b/plugin/pluginenv/environment.go @@ -148,13 +148,9 @@ func (env *Environment) ActivatePlugin(id string) error { if err != nil { return errors.Wrapf(err, "unable to get api for plugin: %v", id) } - if err := supervisor.Start(); err != nil { + if err := supervisor.Start(api); err != nil { return errors.Wrapf(err, "unable to start plugin: %v", id) } - if err := supervisor.Hooks().OnActivate(api); err != nil { - supervisor.Stop() - return errors.Wrapf(err, "unable to activate plugin: %v", id) - } activePlugin.Supervisor = supervisor } diff --git a/plugin/pluginenv/environment_test.go b/plugin/pluginenv/environment_test.go index e4f63cf70..988e5b08f 100644 --- a/plugin/pluginenv/environment_test.go +++ b/plugin/pluginenv/environment_test.go @@ -44,8 +44,8 @@ type MockSupervisor struct { mock.Mock } -func (m *MockSupervisor) Start() error { - return m.Called().Error(0) +func (m *MockSupervisor) Start(api plugin.API) error { + return m.Called(api).Error(0) } func (m *MockSupervisor) Stop() error { @@ -141,12 +141,10 @@ func TestEnvironment(t *testing.T) { provider.On("API").Return(&api, nil) provider.On("Supervisor").Return(&supervisor, nil) - supervisor.On("Start").Return(nil) + supervisor.On("Start", &api).Return(nil) supervisor.On("Stop").Return(nil) supervisor.On("Hooks").Return(&hooks) - hooks.On("OnActivate", &api).Return(nil) - assert.NoError(t, env.ActivatePlugin("foo")) assert.Equal(t, env.ActivePluginIds(), []string{"foo"}) activePlugins = env.ActivePlugins() @@ -238,17 +236,7 @@ func TestEnvironment_ActivatePluginErrors(t *testing.T) { provider.On("API").Return(&api, nil) provider.On("Supervisor").Return(&supervisor, nil) - supervisor.On("Start").Return(fmt.Errorf("test error")) - }, - "HooksError": func() { - provider.On("API").Return(&api, nil) - provider.On("Supervisor").Return(&supervisor, nil) - - supervisor.On("Start").Return(nil) - supervisor.On("Stop").Return(nil) - supervisor.On("Hooks").Return(&hooks) - - hooks.On("OnActivate", &api).Return(fmt.Errorf("test error")) + supervisor.On("Start", &api).Return(fmt.Errorf("test error")) }, } { t.Run(name, func(t *testing.T) { @@ -291,11 +279,10 @@ func TestEnvironment_ShutdownError(t *testing.T) { provider.On("API").Return(&api, nil) provider.On("Supervisor").Return(&supervisor, nil) - supervisor.On("Start").Return(nil) + supervisor.On("Start", &api).Return(nil) supervisor.On("Stop").Return(fmt.Errorf("test error")) supervisor.On("Hooks").Return(&hooks) - hooks.On("OnActivate", &api).Return(nil) hooks.On("OnDeactivate").Return(fmt.Errorf("test error")) assert.NoError(t, env.ActivatePlugin("foo")) @@ -329,13 +316,12 @@ func TestEnvironment_ConcurrentHookInvocations(t *testing.T) { provider.On("API").Return(&api, nil) provider.On("Supervisor").Return(&supervisor, nil) - supervisor.On("Start").Return(nil) + supervisor.On("Start", &api).Return(nil) supervisor.On("Stop").Return(nil) supervisor.On("Hooks").Return(&hooks) ch := make(chan bool) - hooks.On("OnActivate", &api).Return(nil) hooks.On("OnDeactivate").Return(nil) hooks.On("ServeHTTP", mock.AnythingOfType("*httptest.ResponseRecorder"), mock.AnythingOfType("*http.Request")).Run(func(args mock.Arguments) { r := args.Get(1).(*http.Request) diff --git a/plugin/rpcplugin/supervisor.go b/plugin/rpcplugin/supervisor.go index 7b036d726..ad3c8401d 100644 --- a/plugin/rpcplugin/supervisor.go +++ b/plugin/rpcplugin/supervisor.go @@ -30,11 +30,11 @@ var _ plugin.Supervisor = (*Supervisor)(nil) // Starts the plugin. This method will block until the plugin is successfully launched for the first // time and will return an error if the plugin cannot be launched at all. -func (s *Supervisor) Start() error { +func (s *Supervisor) Start(api plugin.API) error { ctx, cancel := context.WithCancel(context.Background()) s.done = make(chan bool, 1) start := make(chan error, 1) - go s.run(ctx, start) + go s.run(ctx, start, api) select { case <-time.After(time.Second * 3): @@ -60,13 +60,13 @@ func (s *Supervisor) Hooks() plugin.Hooks { return s.hooks.Load().(plugin.Hooks) } -func (s *Supervisor) run(ctx context.Context, start chan<- error) { +func (s *Supervisor) run(ctx context.Context, start chan<- error, api plugin.API) { defer func() { s.done <- true }() done := ctx.Done() for { - s.runPlugin(ctx, start) + s.runPlugin(ctx, start, api) select { case <-done: return @@ -77,7 +77,7 @@ func (s *Supervisor) run(ctx context.Context, start chan<- error) { } } -func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error) error { +func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error, api plugin.API) error { p, ipc, err := NewProcess(ctx, s.executable) if err != nil { if start != nil { @@ -100,6 +100,10 @@ func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error) error { }() hooks, err := ConnectMain(muxer) + if err == nil { + err = hooks.OnActivate(api) + } + if err != nil { if start != nil { start <- err @@ -111,6 +115,7 @@ func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error) error { } s.hooks.Store(hooks) + if start != nil { start <- nil } diff --git a/plugin/rpcplugin/supervisor_test.go b/plugin/rpcplugin/supervisor_test.go index 6a2b5ec5d..bd20158b5 100644 --- a/plugin/rpcplugin/supervisor_test.go +++ b/plugin/rpcplugin/supervisor_test.go @@ -1,6 +1,8 @@ package rpcplugin import ( + "encoding/json" + "fmt" "io/ioutil" "os" "path/filepath" @@ -8,9 +10,11 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/model" + "github.com/mattermost/mattermost-server/plugin/plugintest" ) func TestSupervisor(t *testing.T) { @@ -38,8 +42,7 @@ func TestSupervisor(t *testing.T) { bundle := model.BundleInfoForPath(dir) supervisor, err := SupervisorProvider(bundle) require.NoError(t, err) - require.NoError(t, supervisor.Start()) - require.NoError(t, supervisor.Hooks().OnActivate(nil)) + require.NoError(t, supervisor.Start(nil)) require.NoError(t, supervisor.Stop()) } @@ -68,7 +71,7 @@ func TestSupervisor_NonExistentExecutablePath(t *testing.T) { require.NotNil(t, supervisor) require.NoError(t, err) - require.Error(t, supervisor.Start()) + require.Error(t, supervisor.Start(nil)) } // If plugin development goes really wrong, let's make sure plugin activation won't block forever. @@ -92,7 +95,7 @@ func TestSupervisor_StartTimeout(t *testing.T) { bundle := model.BundleInfoForPath(dir) supervisor, err := SupervisorProvider(bundle) require.NoError(t, err) - require.Error(t, supervisor.Start()) + require.Error(t, supervisor.Start(nil)) } // Crashed plugins should be relaunched. @@ -112,14 +115,23 @@ func TestSupervisor_PluginCrash(t *testing.T) { "github.com/mattermost/mattermost-server/plugin/rpcplugin" ) - type MyPlugin struct {} + type Configuration struct { + ShouldExit bool + } + + type MyPlugin struct { + config Configuration + } func (p *MyPlugin) OnActivate(api plugin.API) error { - os.Exit(1) + api.LoadPluginConfiguration(&p.config) return nil } func (p *MyPlugin) OnDeactivate() error { + if p.config.ShouldExit { + os.Exit(1) + } return nil } @@ -130,17 +142,28 @@ func TestSupervisor_PluginCrash(t *testing.T) { ioutil.WriteFile(filepath.Join(dir, "plugin.json"), []byte(`{"id": "foo", "backend": {"executable": "backend.exe"}}`), 0600) + var api plugintest.API + shouldExit := true + api.On("LoadPluginConfiguration", mock.MatchedBy(func(x interface{}) bool { return true })).Return(func(dest interface{}) error { + err := json.Unmarshal([]byte(fmt.Sprintf(`{"ShouldExit": %v}`, shouldExit)), dest) + shouldExit = false + return err + }) + bundle := model.BundleInfoForPath(dir) supervisor, err := SupervisorProvider(bundle) require.NoError(t, err) - require.NoError(t, supervisor.Start()) - require.Error(t, supervisor.Hooks().OnActivate(nil)) + require.NoError(t, supervisor.Start(&api)) + failed := false recovered := false for i := 0; i < 30; i++ { if supervisor.Hooks().OnDeactivate() == nil { + require.True(t, failed) recovered = true break + } else { + failed = true } time.Sleep(time.Millisecond * 100) } diff --git a/plugin/supervisor.go b/plugin/supervisor.go index 76f20eaee..6cb7445f7 100644 --- a/plugin/supervisor.go +++ b/plugin/supervisor.go @@ -6,7 +6,7 @@ package plugin // Supervisor provides the interface for an object that controls the execution of a plugin. This // type is only relevant to the server, and isn't used by the plugins themselves. type Supervisor interface { - Start() error + Start(API) error Stop() error Hooks() Hooks } -- cgit v1.2.3-1-g7c22