diff options
author | Jesse Hallam <jesse.hallam@gmail.com> | 2018-05-01 10:34:12 -0400 |
---|---|---|
committer | Christopher Speller <crspeller@gmail.com> | 2018-05-01 07:34:12 -0700 |
commit | 1e6553704d372f82e937094721d3b83ccb3fa6de (patch) | |
tree | 89afa44e8bfa9fee71b517ec901cf5dfaa1ee57d /plugin/rpcplugin/supervisor.go | |
parent | 3b57422c5fb6f6631a96c1c6a5d730b1db6e4b2a (diff) | |
download | chat-1e6553704d372f82e937094721d3b83ccb3fa6de.tar.gz chat-1e6553704d372f82e937094721d3b83ccb3fa6de.tar.bz2 chat-1e6553704d372f82e937094721d3b83ccb3fa6de.zip |
MM-8622: improved plugin error handling (#8692)
* don't report an error on plugin activation if already active
* improved plugin logging events
Log an error when a plugin's ServeHTTP fails, or when it unexpectedly
terminates.
Restart a plugin at most three times, allowing its failure to later
bubble up under the "failed to stay running" status.
* clarified plugin activation/deactivation
Avoid repeatedly activating when any configuration bit changes. Improved
logging.
* constrain plugin ids to ^[a-zA-Z0-9-_\.]+$ and enforce minimum length
Previously, the plugin id was used unsanitized to relocate the plugin
bundle, which allowed writing outside the `plugins/` directory by using
an `id` containing `../`.
Similarly, an empty string was accepted as an id and led to unexpected
error messages.
* remove plugins by manifest path, not id
If the id within the manifest ever diverges from the actual plugin
location, it becomes impossible to remove via the API. Instead, if the
plugin is found by id, remove the path containing the manifest.
* ignore plugins with nil manifests
If a plugin was detected, but had a manifest that couldn't be parsed, it
will be left nil but still be listed among the packages. Skip over these
in most cases to avoid segfaults.
* leverage mlog more effectively for plugins
* build issues
Diffstat (limited to 'plugin/rpcplugin/supervisor.go')
-rw-r--r-- | plugin/rpcplugin/supervisor.go | 26 |
1 files changed, 21 insertions, 5 deletions
diff --git a/plugin/rpcplugin/supervisor.go b/plugin/rpcplugin/supervisor.go index 6a48cb5e8..6e26d5682 100644 --- a/plugin/rpcplugin/supervisor.go +++ b/plugin/rpcplugin/supervisor.go @@ -12,19 +12,26 @@ import ( "sync/atomic" "time" + "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" ) +const ( + MaxProcessRestarts = 3 +) + // Supervisor implements a plugin.Supervisor that launches the plugin in a separate process and // communicates via RPC. // -// If the plugin unexpectedly exists, the supervisor will relaunch it after a short delay. +// If the plugin unexpectedly exits, the supervisor will relaunch it after a short delay, but will +// only restart a plugin at most three times. type Supervisor struct { hooks atomic.Value done chan bool cancel context.CancelFunc newProcess func(context.Context) (Process, io.ReadWriteCloser, error) + pluginId string } var _ plugin.Supervisor = (*Supervisor)(nil) @@ -66,19 +73,28 @@ func (s *Supervisor) run(ctx context.Context, start chan<- error, api plugin.API s.done <- true }() done := ctx.Done() - for { + for i := 0; i <= MaxProcessRestarts; i++ { s.runPlugin(ctx, start, api) select { case <-done: return default: start = nil - time.Sleep(time.Second) + if i < MaxProcessRestarts { + mlog.Debug("Plugin terminated unexpectedly", mlog.String("plugin_id", s.pluginId)) + time.Sleep(time.Duration((1 + i*i)) * time.Second) + } else { + mlog.Debug("Plugin terminated unexpectedly too many times", mlog.String("plugin_id", s.pluginId), mlog.Int("max_process_restarts", MaxProcessRestarts)) + } } } } func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error, api plugin.API) error { + if start == nil { + mlog.Debug("Restarting plugin", mlog.String("plugin_id", s.pluginId)) + } + p, ipc, err := s.newProcess(ctx) if err != nil { if start != nil { @@ -100,7 +116,7 @@ func (s *Supervisor) runPlugin(ctx context.Context, start chan<- error, api plug muxerClosed <- muxer.Close() }() - hooks, err := ConnectMain(muxer) + hooks, err := ConnectMain(muxer, s.pluginId) if err == nil { err = hooks.OnActivate(api) } @@ -147,5 +163,5 @@ func SupervisorWithNewProcessFunc(bundle *model.BundleInfo, newProcess func(cont if strings.HasPrefix(executable, "..") { return nil, fmt.Errorf("invalid backend executable") } - return &Supervisor{newProcess: newProcess}, nil + return &Supervisor{pluginId: bundle.Manifest.Id, newProcess: newProcess}, nil } |