Merge pull request #6643 from axw/lp1645729-watcher-dispose

apiserver: dispose facade object on watcher.Stop

When we stop a watcher, we will now dispose of the
facade object that is cached upon creation. Unless
we do this, the cache grows every time a watcher
is started, and only cleared when the API connection
is severed.

Fixes the issue noted in https://bugs.launchpad.net/juju-core/+bug/1645729, but for the develop branch

apiserver/controller/controller_test.go

@@ -301,17 +301,20 @@ func (s *controllerSuite) TestWatchAllModels(c *gc.C) {
 	watcherId, err := s.controller.WatchAllModels()
 	c.Assert(err, jc.ErrorIsNil)
 
+	var disposed bool
 	watcherAPI_, err := apiserver.NewAllWatcher(facadetest.Context{
 		State_:     s.State,
 		Resources_: s.resources,
 		Auth_:      s.authorizer,
 		ID_:        watcherId.AllWatcherId,
+		Dispose_:   func() { disposed = true },
 	})
 	c.Assert(err, jc.ErrorIsNil)
 	watcherAPI := watcherAPI_.(*apiserver.SrvAllWatcher)
 	defer func() {
 		err := watcherAPI.Stop()
 		c.Assert(err, jc.ErrorIsNil)
+		c.Assert(disposed, jc.IsTrue)
 	}()
 
 	resultC := make(chan params.AllWatcherNextResults)

apiserver/facade/facadetest/context.go

@@ -12,6 +12,7 @@ import (
 type Context struct {
 	Abort_     <-chan struct{}
 	Auth_      facade.Authorizer
+	Dispose_   func()
 	Resources_ facade.Resources
 	State_     *state.State
 	ID_        string
@@ -27,6 +28,11 @@ func (context Context) Auth() facade.Authorizer {
 	return context.Auth_
 }
 
+// Dispose is part of the facade.Context interface.
+func (context Context) Dispose() {
+	context.Dispose_()
+}
+
 // Resources is part of the facade.Context interface.
 func (context Context) Resources() facade.Resources {
 	return context.Resources_

apiserver/facade/interface.go

@@ -33,6 +33,15 @@ type Context interface {
 	// not *known* to have a responsibility or requirement.
 	Auth() Authorizer
 
+	// Dispose disposes the context and any resources related to
+	// the API server facade object. Normally the context will not
+	// be disposed until the API connection is closed. This is OK
+	// except when contexts are dynamically generated, such as in
+	// the case of watchers. When a facade context is no longer
+	// needed, e.g. when a watcher is closed, then the context may
+	// be disposed by calling this method.
+	Dispose()
+
 	// Resources exposes per-connection capabilities. By adding a
 	// resource, you make it accessible by (returned) id to all
 	// other facades used by this connection. It's mostly used to

apiserver/root.go

@@ -196,7 +196,7 @@ func (r *apiRoot) FindMethod(rootName string, version int, methodName string) (r
 			// check.
 			return reflect.Value{}, err
 		}
-		obj, err := factory(r.facadeContext(id))
+		obj, err := factory(r.facadeContext(objKey))
 		if err != nil {
 			return reflect.Value{}, err
 		}
@@ -225,37 +225,53 @@ func (r *apiRoot) FindMethod(rootName string, version int, methodName string) (r
 	}, nil
 }
 
-func (r *apiRoot) facadeContext(id string) *facadeContext {
+func (r *apiRoot) dispose(key objectKey) {
+	r.objectMutex.Lock()
+	defer r.objectMutex.Unlock()
+	delete(r.objectCache, key)
+}
+
+func (r *apiRoot) facadeContext(key objectKey) *facadeContext {
 	return &facadeContext{
-		r:  r,
-		id: id,
+		r:   r,
+		key: key,
 	}
 }
 
 // facadeContext implements facade.Context
 type facadeContext struct {
-	r  *apiRoot
-	id string
+	r   *apiRoot
+	key objectKey
 }
 
+// Abort is part of of the facade.Context interface.
 func (ctx *facadeContext) Abort() <-chan struct{} {
 	return nil
 }
 
+// Auth is part of of the facade.Context interface.
 func (ctx *facadeContext) Auth() facade.Authorizer {
 	return ctx.r.authorizer
 }
 
+// Dispose is part of of the facade.Context interface.
+func (ctx *facadeContext) Dispose() {
+	ctx.r.dispose(ctx.key)
+}
+
+// Resources is part of of the facade.Context interface.
 func (ctx *facadeContext) Resources() facade.Resources {
 	return ctx.r.resources
 }
 
+// State is part of of the facade.Context interface.
 func (ctx *facadeContext) State() *state.State {
 	return ctx.r.state
 }
 
+// ID is part of of the facade.Context interface.
 func (ctx *facadeContext) ID() string {
-	return ctx.id
+	return ctx.key.objId
 }
 
 func lookupMethod(rootName string, version int, methodName string) (reflect.Type, rpcreflect.ObjMethod, error) {