From 835c0871a01d9c0460ee34e22e9dfc53a69a2ac9 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 27 Jul 2018 11:37:17 -0400 Subject: MM-11431: handle plugin deadlocks (#9167) * ensure plugin is always shutdown Once we call `.client.Client()` the plugin has started, and must be shut down. `newSupervisor` sometimes returned with an error (and without a reference to the supervisor), leaving the client running indefinitely. * Clarify the documentation to explain that plugin hooks will not trigger until `OnActivate` returns successfully, and will stop triggering just before `OnDeactivate` is called. * test for plugin deadlock * plugin/environment.go: switch to sync.Map From: https://golang.org/pkg/sync/#Map > If a goroutine holds a RWMutex for reading and another goroutine might call Lock, no goroutine should expect to be able to acquire a read lock until the initial read lock is released. In particular, this prohibits recursive read locking. This is to ensure that the lock eventually becomes available; a blocked Lock call excludes new readers from acquiring the lock. The previous `RWMutex` was not safe given that we effectively acquired read locks recursively (hook -> api -> hook). This worked up until we activated or deactivated plugins, tried to acquire a write lock, and the plugin used the API to effectively trigger another hook. Switching to sync.Map avoids this by divesting the need to lock at all, avoiding the potential for a recursive lock in the first place. --- plugin/hooks.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'plugin/hooks.go') diff --git a/plugin/hooks.go b/plugin/hooks.go index 944909077..c191652e3 100644 --- a/plugin/hooks.go +++ b/plugin/hooks.go @@ -39,7 +39,9 @@ const ( // A plugin only need implement the hooks it cares about. The MattermostPlugin provides some // default implementations for convenience but may be overridden. type Hooks interface { - // OnActivate is invoked when the plugin is activated. + // OnActivate is invoked when the plugin is activated. If an error is returned, the plugin + // will be terminated. The plugin will not receive hooks until after OnActivate returns + // without error. OnActivate() error // Implemented returns a list of hooks that are implemented by the plugin. @@ -47,7 +49,8 @@ type Hooks interface { Implemented() ([]string, error) // OnDeactivate is invoked when the plugin is deactivated. This is the plugin's last chance to - // use the API, and the plugin will be terminated shortly after this invocation. + // use the API, and the plugin will be terminated shortly after this invocation. The plugin + // will stop receiving hooks just prior to this method being called. OnDeactivate() error // OnConfigurationChange is invoked when configuration changes may have been made. -- cgit v1.2.3-1-g7c22