Merge lp:~jameinel/juju-core/api-named-resources into lp:~go-bot/juju-core/trunk

[John A Meinel]

state/apiserver/common: Resources.RegisterNamed

Two small changes here.

1) Change common.Resources to allow you to RegisterNamed rather than
   only allowing you to register and get back the Id. This was necessary
   for the second step, but generally allows us to declare Resources
   that Facades can know about that aren't reported to clients.

2) Change the pingTimeout object from being an attribute of srvRoot into
   being just another resource like the Watchers at a specific resource
   name.

While working on the changes for pingTimeout, I noticed that no tests
actually failed if I started returning nullTimeout. So I added a test
that ensures calling Ping() actually does delay the connection from
being closed. (We only had a test that if you never called Ping in time,
it would die, but not that Ping can delay that death.)

This is a step towards making all of the Facades follow a common
pattern, as this allows Pinger to also just take Resources rather than
needing to know about details of the srvRoot object.

https://codereview.appspot.com/93500044/

state/apiserver/common: Resources.RegisterNamed

Two small changes here.

1) Change common.Resources to allow you to RegisterNamed rather than
   only allowing you to register and get back the Id. This was necessary
   for the second step, but generally allows us to declare Resources
   that Facades can know about that aren't reported to clients.

2) Change the pingTimeout object from being an attribute of srvRoot into
   being just another resource like the Watchers at a specific resource
   name.

While working on the changes for pingTimeout, I noticed that no tests
actually failed if I started returning nullTimeout. So I added a test
that ensures calling Ping() actually does delay the connection from
being closed. (We only had a test that if you never called Ping in time,
it would die, but not that Ping can delay that death.)

This is a step towards making all of the Facades follow a common
pattern, as this allows Pinger to also just take Resources rather than
needing to know about details of the srvRoot object.

https://codereview.appspot.com/93500044/

[John A Meinel (jameinel)]

Reviewers: mp+220208_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/common: Resources.RegisterNamed

Two small changes here.

1) Change common.Resources to allow you to RegisterNamed rather than
    only allowing you to register and get back the Id. This was necessary
    for the second step, but generally allows us to declare Resources
    that Facades can know about that aren't reported to clients.

2) Change the pingTimeout object from being an attribute of srvRoot into
    being just another resource like the Watchers at a specific resource
    name.

While working on the changes for pingTimeout, I noticed that no tests
actually failed if I started returning nullTimeout. So I added a test
that ensures calling Ping() actually does delay the connection from
being closed. (We only had a test that if you never called Ping in time,
it would die, but not that Ping can delay that death.)

This is a step towards making all of the Facades follow a common
pattern, as this allows Pinger to also just take Resources rather than
needing to know about details of the srvRoot object.

https://code.launchpad.net/~jameinel/juju-core/api-named-resources/+merge/220208

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/93500044/

Affected files (+118, -13 lines):
   A [revision details]
   M state/apiserver/admin.go
   M state/apiserver/common/resource.go
   M state/apiserver/common/resource_test.go
   M state/apiserver/pinger_test.go
   M state/apiserver/root.go
   M state/apiserver/root_test.go

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/93500044/

=== modified file 'state/apiserver/admin.go'
--- state/apiserver/admin.go	2014-05-13 04:30:48 +0000
+++ state/apiserver/admin.go	2014-05-20 08:25:48 +0000
@@ -181,6 +181,7 @@
 			logger.Errorf("error closing the RPC connection: %v", err)
 		}
 	}
-	newRoot.pingTimeout = newPingTimeout(action, maxClientPingInterval)
+	pingTimeout := newPingTimeout(action, maxClientPingInterval)
+	newRoot.resources.RegisterNamed("pingTimeout", pingTimeout)
 	return nil
 }

=== modified file 'state/apiserver/common/resource.go'
--- state/apiserver/common/resource.go	2014-04-17 01:26:34 +0000
+++ state/apiserver/common/resource.go	2014-05-20 08:25:48 +0000
@@ -4,6 +4,7 @@
 package common
 
 import (
+	"fmt"
 	"strconv"
 	"sync"
 )
@@ -50,6 +51,26 @@
 	return id
 }
 
+// RegisterNamed registers the given resource. Callers must supply a unique
+// name for the given resource. It is an error to try to register another
+// resource with the same name as an already registered name. (This could be
+// softened that you can overwrite an existing one and it will be Stopped and
+// replaced, but we don't have a need for that yet.)
+// It is also an error to supply a name that is an integer string, since that
+// collides with the auto-naming from Register.
+func (rs *Resources) RegisterNamed(name string, r Resource) error {
+	rs.mu.Lock()
+	defer rs.mu.Unlock()
+	if _, err := strconv.Atoi(name); err == nil {
+		return fmt.Errorf("RegisterNamed does not allow integer names: %q", name)
+	}
+	if _, ok := rs.resources[name]; ok {
+		return fmt.Errorf("resource %q already registered", name)
+	}
+	rs.resources[name] = r
+	return nil
+}
+
 // Stop stops the resource with the given id and unregisters it.
 // It returns any error from the underlying Stop call.
 // It does not return an error if the resource has already

=== modified file 'state/apiserver/common/resource_test.go'
--- state/apiserver/common/resource_test.go	2013-08-05 10:19:47 +0000
+++ state/apiserver/common/resource_test.go	2014-05-20 08:25:48 +0000
@@ -36,6 +36,50 @@
 	c.Assert(rs.Count(), gc.Equals, 2)
 }
 
+func (resourceSuite) TestRegisterNamedGetCount(c *gc.C) {
+	rs := common.NewResources()
+	defer rs.StopAll()
+	r1 := &fakeResource{}
+	err := rs.RegisterNamed("fake1", r1)
+	c.Assert(err, gc.IsNil)
+	c.Check(rs.Count(), gc.Equals, 1)
+	c.Check(rs.Get("fake1"), gc.Equals, r1)
+}
+
+func (resourceSuite) TestRegisterNamedRepeatedName(c *gc.C) {
+	rs := common.NewResources()
+	defer rs.StopAll()
+	r1 := &fakeResource{}
+	r2 := &fakeResource{}
+	err := rs.RegisterNamed("fake1", r1)
+	c.Assert(err, gc.IsNil)
+	c.Check(rs.Count(), gc.Equals, 1)
+	err = rs.RegisterNamed("fake1", r2)
+	c.Check(err, gc.ErrorMatches, `resource "fake1" already registered`)
+	c.Check(rs.Count(), gc.Equals, 1)
+	c.Check(rs.Get("fake1"), gc.Equals, r1)
+}
+
+func (resourceSuite) TestRegisterNamedIntegerName(c *gc.C) {
+	rs := common.NewResources()
+	defer rs.StopAll()
+	r1 := &fakeResource{}
+	err := rs.RegisterNamed("1", r1)
+	c.Check(err, gc.ErrorMatches, `RegisterNamed does not allow integer names: "1"`)
+	c.Check(rs.Count(), gc.Equals, 0)
+	c.Check(rs.Get("fake1"), gc.IsNil)
+}
+
+func (resourceSuite) TestRegisterNamedIntegerStart(c *gc.C) {
+	rs := common.NewResources()
+	defer rs.StopAll()
+	r1 := &fakeResource{}
+	err := rs.RegisterNamed("1fake", r1)
+	c.Assert(err, gc.IsNil)
+	c.Check(rs.Count(), gc.Equals, 1)
+	c.Check(rs.Get("1fake"), gc.Equals, r1)
+}
+
 func (resourceSuite) TestConcurrency(c *gc.C) {
 	// This test is designed to cause the race detector
 	// to fail if the locking is not done correctly.
@@ -54,6 +98,9 @@
 		rs.Register(&fakeResource{})
 	})
 	start(func() {
+		rs.RegisterNamed("named", &fakeResource{})
+	})
+	start(func() {
 		rs.Stop("1")
 	})
 	start(func() {
@@ -65,6 +112,9 @@
 	start(func() {
 		rs.Get("2")
 	})
+	start(func() {
+		rs.Get("named")
+	})
 	wg.Wait()
 }
 

=== modified file 'state/apiserver/pinger_test.go'
--- state/apiserver/pinger_test.go	2014-04-04 16:52:41 +0000
+++ state/apiserver/pinger_test.go	2014-05-20 08:25:48 +0000
@@ -86,6 +86,25 @@
 	c.Assert(err, gc.ErrorMatches, "connection is shut down")
 }
 
+func (s *stateSuite) TestAgentConnectionDelaysShutdownWithPing(c *gc.C) {
+	// We have to be careful, because Login can take 25ms, so we ping
+	// immediately after connecting.
+	s.PatchValue(apiserver.MaxClientPingInterval, 50*time.Millisecond)
+	st, _ := s.OpenAPIAsNewMachine(c)
+	err := st.Ping()
+	c.Assert(err, gc.IsNil)
+	// As long as we don't wait too long, the connection stays open
+	for i := 0; i < 10; i++ {
+		time.Sleep(10 * time.Millisecond)
+		err = st.Ping()
+		c.Assert(err, gc.IsNil)
+	}
+	// However, once we stop pinging for too long, the connection dies
+	time.Sleep(75 * time.Millisecond)
+	err = st.Ping()
+	c.Assert(err, gc.ErrorMatches, "connection is shut down")
+}
+
 type mongoPingerSuite struct {
 	testing.JujuConnSuite
 }

=== modified file 'state/apiserver/root.go'
--- state/apiserver/root.go	2014-05-14 16:07:11 +0000
+++ state/apiserver/root.go	2014-05-20 08:25:48 +0000
@@ -56,10 +56,9 @@
 // after it has logged in.
 type srvRoot struct {
 	clientAPI
-	srv         *Server
-	rpcConn     *rpc.Conn
-	resources   *common.Resources
-	pingTimeout *pingTimeout
+	srv       *Server
+	rpcConn   *rpc.Conn
+	resources *common.Resources
 
 	entity taggedAuthenticator
 }
@@ -82,9 +81,6 @@
 // cleaning up to ensure that all outstanding requests return.
 func (r *srvRoot) Kill() {
 	r.resources.StopAll()
-	if r.pingTimeout != nil {
-		r.pingTimeout.stop()
-	}
 }
 
 // requireAgent checks whether the current client is an agent and hence
@@ -347,10 +343,11 @@
 // is not called frequently enough, the connection
 // will be dropped.
 func (r *srvRoot) Pinger(id string) (pinger, error) {
-	if r.pingTimeout == nil {
+	pingTimeout, ok := r.resources.Get("pingTimeout").(pinger)
+	if !ok {
 		return nullPinger{}, nil
 	}
-	return r.pingTimeout, nil
+	return pingTimeout, nil
 }
 
 type nullPinger struct{}
@@ -438,8 +435,8 @@
 	}
 }
 
-// stop terminates the ping timeout.
-func (pt *pingTimeout) stop() error {
+// Stop terminates the ping timeout.
+func (pt *pingTimeout) Stop() error {
 	pt.tomb.Kill(nil)
 	return pt.tomb.Wait()
 }

=== modified file 'state/apiserver/root_test.go'
--- state/apiserver/root_test.go	2014-01-22 19:28:08 +0000
+++ state/apiserver/root_test.go	2014-05-20 08:25:48 +0000
@@ -58,7 +58,6 @@
 	// Expect action to be executed about 50ms after last ping.
 	broken := time.Now()
 	var closed time.Time
-	time.Sleep(100 * time.Millisecond)
 	select {
 	case closed = <-closedc:
 	case <-time.After(testing.LongWait):
@@ -67,3 +66,19 @@
 	closeDiff := closed.Sub(broken) / time.Millisecond
 	c.Assert(50 <= closeDiff && closeDiff <= 100, gc.Equals, true)
 }
+
+func (r *rootSuite) TestPingTimeoutStopped(c *gc.C) {
+	closedc := make(chan time.Time, 1)
+	action := func() {
+		closedc <- time.Now()
+	}
+	timeout := apiserver.NewPingTimeout(action, 20*time.Millisecond)
+	timeout.Ping()
+	timeout.Stop()
+	// The action should never trigger
+	select {
+	case <-closedc:
+		c.Fatalf("action triggered after Stop()")
+	case <-time.After(testing.ShortWait):
+	}
+}