call OnActivate after plugin crash, update example (#7940)

6 files changed, 51 insertions, 40 deletions

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
 }