From 2938dce794be7559ba73b4e9630015020a7fa937 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 27 Dec 2016 11:07:22 -0500 Subject: [PATCH] Fix race/deadlock in v1 plugin handlers When a plugin is activated, and then `plugins.Handle` is called to register a new handler for a given plugin type, a deadlock occurs when for anything which calls `waitActive`, including `Get`, and `GetAll`. This happens because `Handle()` is setting `activated` to `false` to ensure that plugin handlers are run on next activation. Maybe these handlers should be called immediately for any plugins which are already registered... but to preserve the existing behavior while fixing the deadlock, track if handlers have been run on plugins and reset when a new handler is registered. The simplest way to reproduce the deadlock with Docker is to add a `-v /foo` to the test container created for the external graphdriver tests. Signed-off-by: Brian Goff --- ...cker_cli_external_graphdriver_unix_test.go | 4 + pkg/plugins/plugin_test.go | 37 ++++++++++ pkg/plugins/plugins.go | 74 +++++++++++++------ 3 files changed, 92 insertions(+), 23 deletions(-) create mode 100644 pkg/plugins/plugin_test.go diff --git a/integration-cli/docker_cli_external_graphdriver_unix_test.go b/integration-cli/docker_cli_external_graphdriver_unix_test.go index a700f82a8a..152bc6aa63 100644 --- a/integration-cli/docker_cli_external_graphdriver_unix_test.go +++ b/integration-cli/docker_cli_external_graphdriver_unix_test.go @@ -57,6 +57,10 @@ func (s *DockerExternalGraphdriverSuite) SetUpTest(c *check.C) { }) } +func (s *DockerExternalGraphdriverSuite) OnTimeout(c *check.C) { + s.d.DumpStackAndQuit() +} + func (s *DockerExternalGraphdriverSuite) TearDownTest(c *check.C) { if s.d != nil { s.d.Stop(c) diff --git a/pkg/plugins/plugin_test.go b/pkg/plugins/plugin_test.go new file mode 100644 index 0000000000..ac810565f7 --- /dev/null +++ b/pkg/plugins/plugin_test.go @@ -0,0 +1,37 @@ +package plugins + +import ( + "path/filepath" + "runtime" + "sync" + "testing" + "time" +) + +// regression test for deadlock in handlers +func TestPluginAddHandler(t *testing.T) { + // make a plugin which is pre-activated + p := &Plugin{activateWait: sync.NewCond(&sync.Mutex{})} + p.Manifest = &Manifest{Implements: []string{"bananas"}} + storage.plugins["qwerty"] = p + + testActive(t, p) + Handle("bananas", func(_ string, _ *Client) {}) + testActive(t, p) +} + +func testActive(t *testing.T, p *Plugin) { + done := make(chan struct{}) + go func() { + p.waitActive() + close(done) + }() + + select { + case <-time.After(100 * time.Millisecond): + _, f, l, _ := runtime.Caller(1) + t.Fatalf("%s:%d: deadlock in waitActive", filepath.Base(f), l) + case <-done: + } + +} diff --git a/pkg/plugins/plugins.go b/pkg/plugins/plugins.go index acfb209992..125e6c7d66 100644 --- a/pkg/plugins/plugins.go +++ b/pkg/plugins/plugins.go @@ -70,12 +70,12 @@ type Plugin struct { // Manifest of the plugin (see above) Manifest *Manifest `json:"-"` - // error produced by activation - activateErr error - // specifies if the activation sequence is completed (not if it is successful or not) - activated bool // wait for activation to finish activateWait *sync.Cond + // error produced by activation + activateErr error + // keeps track of callback handlers run against this plugin + handlersRun bool } // BasePath returns the path to which all paths returned by the plugin are relative to. @@ -112,19 +112,51 @@ func NewLocalPlugin(name, addr string) *Plugin { func (p *Plugin) activate() error { p.activateWait.L.Lock() - if p.activated { + + if p.activated() { + p.runHandlers() p.activateWait.L.Unlock() return p.activateErr } p.activateErr = p.activateWithLock() - p.activated = true + p.runHandlers() p.activateWait.L.Unlock() p.activateWait.Broadcast() return p.activateErr } +// runHandlers runs the registered handlers for the implemented plugin types +// This should only be run after activation, and while the activation lock is held. +func (p *Plugin) runHandlers() { + if !p.activated() { + return + } + + handlers.RLock() + if !p.handlersRun { + for _, iface := range p.Manifest.Implements { + hdlrs, handled := handlers.extpointHandlers[iface] + if !handled { + continue + } + for _, handler := range hdlrs { + handler(p.name, p.client) + } + } + p.handlersRun = true + } + handlers.RUnlock() + +} + +// activated returns if the plugin has already been activated. +// This should only be called with the activation lock held +func (p *Plugin) activated() bool { + return p.Manifest != nil +} + func (p *Plugin) activateWithLock() error { c, err := NewClient(p.Addr, p.TLSConfig) if err != nil { @@ -138,24 +170,12 @@ func (p *Plugin) activateWithLock() error { } p.Manifest = m - - handlers.RLock() - for _, iface := range m.Implements { - hdlrs, handled := handlers.extpointHandlers[iface] - if !handled { - continue - } - for _, handler := range hdlrs { - handler(p.name, p.client) - } - } - handlers.RUnlock() return nil } func (p *Plugin) waitActive() error { p.activateWait.L.Lock() - for !p.activated { + for !p.activated() { p.activateWait.Wait() } p.activateWait.L.Unlock() @@ -163,7 +183,7 @@ func (p *Plugin) waitActive() error { } func (p *Plugin) implements(kind string) bool { - if err := p.waitActive(); err != nil { + if p.Manifest == nil { return false } for _, driver := range p.Manifest.Implements { @@ -232,7 +252,7 @@ func Get(name, imp string) (*Plugin, error) { if err != nil { return nil, err } - if pl.implements(imp) { + if err := pl.waitActive(); err == nil && pl.implements(imp) { logrus.Debugf("%s implements: %s", name, imp) return pl, nil } @@ -249,9 +269,17 @@ func Handle(iface string, fn func(string, *Client)) { hdlrs = append(hdlrs, fn) handlers.extpointHandlers[iface] = hdlrs + + storage.Lock() for _, p := range storage.plugins { - p.activated = false + p.activateWait.L.Lock() + if p.activated() && p.implements(iface) { + p.handlersRun = false + } + p.activateWait.L.Unlock() } + storage.Unlock() + handlers.Unlock() } @@ -292,7 +320,7 @@ func GetAll(imp string) ([]*Plugin, error) { logrus.Error(pl.err) continue } - if pl.pl.implements(imp) { + if err := pl.pl.waitActive(); err == nil && pl.pl.implements(imp) { out = append(out, pl.pl) } }