From e1e2ca59512bb888f7fd77af0e2888980a051bb2 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 1 Oct 2018 14:16:45 -0400 Subject: [PATCH 1/4] MM-12193: illustrate simplified, but safe configuration handling --- server/configuration.go | 69 +++++++++++++++++++++++++++++++++++++++++ server/plugin.go | 9 ++++++ 2 files changed, 78 insertions(+) create mode 100644 server/configuration.go diff --git a/server/configuration.go b/server/configuration.go new file mode 100644 index 0000000..24bb825 --- /dev/null +++ b/server/configuration.go @@ -0,0 +1,69 @@ +package main + +import ( + "github.com/pkg/errors" +) + +// configuration captures the plugin's external configuration as exposed in the Mattermost server +// configuration, as well as values computed from the configuration. Any public fields will be +// deserialized from the Mattermost server configuration in OnConfigurationChange, while any +// private fields will be ignored. +// +// As plugins are inherently concurrent (hooks being called asynchronously), and the plugin +// configuration can change at any time, access to the configuration must be synchronized. The +// strategy used in this plugin is to guard a pointer to the configuration, and clone the entire +// struct whenever it changes. You may replace this with whatever strategy you choose. +// +// If you add non-reference types to your configuration struct, be sure to rewrite Clone as a deep +// copy appropriate for your types. +type configuration struct { +} + +// Clone shallow copies the configuration. Your implementation may require a deep copy if +// your configuration has reference types. +func (c *configuration) Clone() *configuration { + if c == nil { + return &configuration{} + } + + var clone = *c + return &clone +} + +// getConfiguration retrieves the active configuration under lock, making it safe to use +// concurrently. The active configuration may change underneath the client of this method, but +// the struct returned by this API call is considered immutable. +func (p *Plugin) getConfiguration() *configuration { + p.configurationLock.RLock() + defer p.configurationLock.RUnlock() + + if p.configuration == nil { + return &configuration{} + } + + return p.configuration +} + +// setConfiguration replaces the active configuration under lock. +// +// Do not call setConfiguration while holding the configurationLock, as sync.Mutex is not +// reentrant. +func (p *Plugin) setConfiguration(configuration *configuration) { + p.configurationLock.Lock() + defer p.configurationLock.Unlock() + p.configuration = configuration +} + +// OnConfigurationChange is invoked when configuration changes may have been made. +func (p *Plugin) OnConfigurationChange() error { + var configuration = new(configuration) + + // Load the public configuration fields from the Mattermost server configuration. + if err := p.API.LoadPluginConfiguration(configuration); err != nil { + return errors.Wrap(err, "failed to load plugin configuration") + } + + p.setConfiguration(configuration) + + return nil +} diff --git a/server/plugin.go b/server/plugin.go index 61df7b8..73723c9 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -1,11 +1,20 @@ package main import ( + "sync" + "github.com/mattermost/mattermost-server/plugin" ) type Plugin struct { plugin.MattermostPlugin + + // configurationLock synchronizes access to the configuration. + configurationLock sync.RWMutex + + // configuration is the active plugin configuration. Consult getConfiguration and + // setConfiguration for usage. + configuration *configuration } // See https://developers.mattermost.com/extend/plugins/server/reference/ From 8adb629abd368dbafc86bcf6fc45c2e31e1d8dc6 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 1 Oct 2018 17:12:41 -0400 Subject: [PATCH 2/4] simplify configuration.Clone --- server/configuration.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/configuration.go b/server/configuration.go index 24bb825..5bc900f 100644 --- a/server/configuration.go +++ b/server/configuration.go @@ -22,10 +22,6 @@ type configuration struct { // Clone shallow copies the configuration. Your implementation may require a deep copy if // your configuration has reference types. func (c *configuration) Clone() *configuration { - if c == nil { - return &configuration{} - } - var clone = *c return &clone } From eb2678c6254454bc84016234686abf265f869440 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Mon, 1 Oct 2018 17:18:36 -0400 Subject: [PATCH 3/4] clarify avoiding API while holding configurationLock --- server/configuration.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/configuration.go b/server/configuration.go index 5bc900f..cc4c8dd 100644 --- a/server/configuration.go +++ b/server/configuration.go @@ -43,7 +43,8 @@ func (p *Plugin) getConfiguration() *configuration { // setConfiguration replaces the active configuration under lock. // // Do not call setConfiguration while holding the configurationLock, as sync.Mutex is not -// reentrant. +// reentrant. In particular, avoid using the plugin API entirely, as this may in turn trigger a +// hook back into the plugin. If that hook attempts to acquire this lock, a deadlock may occur. func (p *Plugin) setConfiguration(configuration *configuration) { p.configurationLock.Lock() defer p.configurationLock.Unlock() From 75eccc0628d46d913dc2a291baed01b65de3b1ae Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 2 Oct 2018 14:09:41 -0400 Subject: [PATCH 4/4] pull updates from demo plugin pr --- server/configuration.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/server/configuration.go b/server/configuration.go index cc4c8dd..15424b1 100644 --- a/server/configuration.go +++ b/server/configuration.go @@ -6,8 +6,7 @@ import ( // configuration captures the plugin's external configuration as exposed in the Mattermost server // configuration, as well as values computed from the configuration. Any public fields will be -// deserialized from the Mattermost server configuration in OnConfigurationChange, while any -// private fields will be ignored. +// deserialized from the Mattermost server configuration in OnConfigurationChange. // // As plugins are inherently concurrent (hooks being called asynchronously), and the plugin // configuration can change at any time, access to the configuration must be synchronized. The @@ -45,9 +44,18 @@ func (p *Plugin) getConfiguration() *configuration { // Do not call setConfiguration while holding the configurationLock, as sync.Mutex is not // reentrant. In particular, avoid using the plugin API entirely, as this may in turn trigger a // hook back into the plugin. If that hook attempts to acquire this lock, a deadlock may occur. +// +// This method panics if setConfiguration is called with the existing configuration. This almost +// certainly means that the configuration was modified without being cloned and may result in +// an unsafe access. func (p *Plugin) setConfiguration(configuration *configuration) { p.configurationLock.Lock() defer p.configurationLock.Unlock() + + if configuration != nil && p.configuration == configuration { + panic("setConfiguration called with the existing configuration") + } + p.configuration = configuration }