Merge lp:~jtv/juju-core/generic-locking-test into lp:~go-bot/juju-core/trunk

[Jeroen T. Vermeulen]

Helper: test a function for correct locking.

This is the outcome of discussion at
https://code.launchpad.net/~jtv/juju-core/create-gwacl-sessions/+merge/170032
but produces a helper that is much, much simpler. It could become generic,
although one thing is still missing: it currently only works for locks that
are of type sync.Mutex. It probably ought to work for any type of lock.

Failure of a function to acquire the right lock(s) is a constant risk in
concurrent programs, so one of the things most worth testing, but it's also
relatively hard to test for and so nobody ever does it. Of course if you
can forget the locking you can also forget the test, but this helper makes it
very easy to guard against regressions.

Helper: test a function for correct locking.

This is the outcome of discussion at
https://code.launchpad.net/~jtv/juju-core/create-gwacl-sessions/+merge/170032
but produces a helper that is much, much simpler. It could become generic,
although one thing is still missing: it currently only works for locks that
are of type sync.Mutex. It probably ought to work for any type of lock.

Failure of a function to acquire the right lock(s) is a constant risk in
concurrent programs, so one of the things most worth testing, but it's also
relatively hard to test for and so nobody ever does it. Of course if you
can forget the locking you can also forget the test, but this helper makes it
very easy to guard against regressions.

https://codereview.appspot.com/10433043/

[Tim Penhey (thumper)]

LGTM for what it is, but might be nice to have as a checker.

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode39
environs/azure/environ_test.go:39: func testLockingFunction(lock
*sync.Mutex, function func()) {
If this is to be a generic testing function, you should move it to the
testing package and export it.

Here is a thought. Want to write this as a checker?

c.Assert(some_func, ObservesLockOn(some_mutex)) ?

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode60
environs/azure/environ_test.go:60: // misbehaved "function" plenty of
rope to hang itself.
You could have a few sleep(0) type calls, as this should do the same
thing.

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode74
environs/azure/environ_test.go:74: blankLock := sync.Mutex{}
Wow, surprised there is not a way to ask if a mutex is locked.

https://codereview.appspot.com/10433043/

[Tim Penhey (thumper)]

review: Approve

[John A Meinel (jameinel)]

LGTM though maybe it could be tweaked a bit.

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go
File environs/azure/environ_test.go (right):

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode57
environs/azure/environ_test.go:57: <-proceed
You could probably buffer proceed so that function gets a bit more time
to hang itself. We don't need 'function' to *wait* until the main loop
gets to this point, so buffering the channel makes sense to me.

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode60
environs/azure/environ_test.go:60: // misbehaved "function" plenty of
rope to hang itself.
On 2013/06/20 02:47:22, thumper wrote:
> You could have a few sleep(0) type calls, as this should do the same
thing.

runtime.GoSched exists in 1.1, and we want to support using 1.1 (just
not require it).
Is there a reason you can't just add those calls?

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode70
environs/azure/environ_test.go:70: panic(errors.New("function did not
obey lock"))
Rather than panic to indicate failures, you could have this function
take a c *C and then call c.Assert instead.

Or use Tim's idea and make it a Checker.

https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go#newcode76
environs/azure/environ_test.go:76: panic(errors.New("function did not
release lock"))
I would think that supporting the Locker interface might be more useful
than this final assert. I guess the tradeoff is that you would have to
do something like try to acquire the lock, which potentially leaves you
with a hanging test. (Right now if function doesn't release the lock,
you get a panic, the alternative would give you a hanging test).

I just know a fair amount of code uses a RWMutex rather than only a
plain Mutex, and it seems a shame to need to have 2 code paths for
something so similar.

Also, you could use runtime type assertions to still confirm the Mutex
behavior. Just use:

if mutex, ok := lock.(*sync.Mutex); ok {
   blankMutex := sync.Mutex{}
   ...
}

Or even a:

switch lock.(type) {
   case *sync.Mutex:
    ...
   case *sync.RWMutex:
    ...
}

https://codereview.appspot.com/10433043/

[Dave Cheney (dave-cheney)]

Umm, race detector anyone ?

On Thu, Jun 20, 2013 at 11:43 PM, <email address hidden> wrote:

> The proposal to merge lp:~jtv/juju-core/generic-locking-test into
> lp:juju-core has been updated.
>
> Status: Approved => Merged
>
> For more details, see:
>
> https://code.launchpad.net/~jtv/juju-core/generic-locking-test/+merge/170477
> --
>
> https://code.launchpad.net/~jtv/juju-core/generic-locking-test/+merge/170477
> You are subscribed to branch lp:juju-core.
>

=== modified file 'environs/azure/environ.go'
--- environs/azure/environ.go	2013-06-18 09:37:20 +0000
+++ environs/azure/environ.go	2013-06-20 09:42:24 +0000
@@ -4,6 +4,7 @@
 package azure
 
 import (
+	"launchpad.net/gwacl"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
@@ -64,7 +65,8 @@
 
 // Config is specified in the Environ interface.
 func (env *azureEnviron) Config() *config.Config {
-	panic("unimplemented")
+	snap := env.getSnapshot()
+	return snap.ecfg.Config
 }
 
 // SetConfig is specified in the Environ interface.
@@ -126,3 +128,34 @@
 func (env *azureEnviron) Provider() environs.EnvironProvider {
 	panic("unimplemented")
 }
+
+// TODO: Temporarily deactivating this code.  Passing certificate in-memory
+// may require gwacl change.
+/*
+// getManagementAPI obtains a context object for interfacing with Azure's
+// management API.
+// For now, each invocation just returns a separate object.  This is probably
+// wasteful (each context gets its own SSL connection) and may need optimizing
+// later.
+func (env *azureEnviron) getManagementAPI() (*gwacl.ManagementAPI, error) {
+	snap := env.getSnapshot()
+	subscription := snap.ecfg.ManagementSubscriptionId()
+	cert := snap.ecfg.ManagementCertificate()
+	return gwacl.NewManagementAPI(subscription, cert)
+}
+*/
+
+// getStorageContext obtains a context object for interfacing with Azure's
+// storage API.
+// For now, each invocation just returns a separate object.  This is probably
+// wasteful (each context gets its own SSL connection) and may need optimizing
+// later.
+func (env *azureEnviron) getStorageContext() (*gwacl.StorageContext, error) {
+	snap := env.getSnapshot()
+	context := gwacl.StorageContext{
+		Account: snap.ecfg.StorageAccountName(),
+		Key:     snap.ecfg.StorageAccountKey(),
+	}
+	// There is currently no way for this to fail.
+	return &context, nil
+}

=== modified file 'environs/azure/environ_test.go'
--- environs/azure/environ_test.go	2013-06-17 10:24:42 +0000
+++ environs/azure/environ_test.go	2013-06-20 09:42:24 +0000
@@ -5,6 +5,8 @@
 
 import (
 	. "launchpad.net/gocheck"
+	"launchpad.net/juju-core/environs/config"
+	"launchpad.net/juju-core/testing"
 	"sync"
 )
 
@@ -14,6 +16,18 @@
 
 var _ = Suite(new(EnvironSuite))
 
+func makeEnviron(c *C) *azureEnviron {
+	attrs := makeAzureConfigMap(c)
+	cfg, err := config.New(attrs)
+	c.Assert(err, IsNil)
+	ecfg, err := azureEnvironProvider{}.newConfig(cfg)
+	c.Assert(err, IsNil)
+	return &azureEnviron{
+		name: "env",
+		ecfg: ecfg,
+	}
+}
+
 func (EnvironSuite) TestGetSnapshot(c *C) {
 	original := azureEnviron{name: "this-env", ecfg: new(azureEnvironConfig)}
 	snapshot := original.getSnapshot()
@@ -32,7 +46,44 @@
 	c.Check(snapshot.Mutex, Equals, sync.Mutex{})
 }
 
+func (EnvironSuite) TestGetSnapshotLocksEnviron(c *C) {
+	original := azureEnviron{}
+	testing.TestLockingFunction(&original.Mutex, func() { original.getSnapshot() })
+}
+
 func (EnvironSuite) TestName(c *C) {
 	env := azureEnviron{name: "foo"}
 	c.Check(env.Name(), Equals, env.name)
 }
+
+func (EnvironSuite) TestConfigReturnsConfig(c *C) {
+	cfg := new(config.Config)
+	ecfg := azureEnvironConfig{Config: cfg}
+	env := azureEnviron{ecfg: &ecfg}
+	c.Check(env.Config(), Equals, cfg)
+}
+
+func (EnvironSuite) TestConfigLocksEnviron(c *C) {
+	env := azureEnviron{name: "env", ecfg: new(azureEnvironConfig)}
+	testing.TestLockingFunction(&env.Mutex, func() { env.Config() })
+}
+
+// TODO: Temporarily deactivating this code.  Passing certificate in-memory
+// may require gwacl change.
+/*
+func (EnvironSuite) TestGetManagementAPI(c *C) {
+	env := makeEnviron(c)
+	management, err := env.getManagementAPI()
+	c.Assert(err, IsNil)
+	c.Check(management, NotNil)
+}
+*/
+
+func (EnvironSuite) TestGetStorageContext(c *C) {
+	env := makeEnviron(c)
+	storage, err := env.getStorageContext()
+	c.Assert(err, IsNil)
+	c.Assert(storage, NotNil)
+	c.Check(storage.Account, Equals, env.ecfg.StorageAccountName())
+	c.Check(storage.Key, Equals, env.ecfg.StorageAccountKey())
+}

=== added file 'testing/locking.go'
--- testing/locking.go	1970-01-01 00:00:00 +0000
+++ testing/locking.go	2013-06-20 09:42:24 +0000
@@ -0,0 +1,71 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	"errors"
+	"sync"
+	"time"
+)
+
+// TestLockingFunction verifies that a function obeys a given lock.
+//
+// Use this as a building block in your own tests for proper locking.
+// Parameters are a lock that you expect your function to block on, and
+// the function that you want to test for proper locking on that lock.
+//
+// This helper attempts to verify that the function both obtains and releases
+// the lock.  It will panic if the function fails to do either.
+// TODO: Support generic sync.Locker instead of just Mutex.
+// TODO: This could be a gocheck checker.
+func TestLockingFunction(lock *sync.Mutex, function func()) {
+	// We record two events that must happen in the right order.
+	// Buffer the channel so that we don't get hung up during attempts
+	// to push the events in.
+	events := make(chan string, 2)
+	// Synchronization channel, to make sure that the function starts
+	// trying to run at the point where we're going to make it block.
+	proceed := make(chan bool, 1)
+
+	goroutine := func() {
+		proceed <- true
+		function()
+		events <- "complete function"
+	}
+
+	lock.Lock()
+	go goroutine()
+	// Make the goroutine start now.  It should block inside "function()."
+	// (It's fine, technically even better, if the goroutine started right
+	// away, and this channel is buffered specifically so that it can.)
+	<-proceed
+
+	// Give a misbehaved function plenty of rope to hang itself.  We don't
+	// want it to block for a microsecond, hand control back to here so we
+	// think it's stuck on the lock, and then later continue on its merry
+	// lockless journey to finish last, as expected but for the wrong
+	// reason.
+	for counter := 0; counter < 10; counter++ {
+		// TODO: In Go 1.1, use runtime.GoSched instead.
+		time.Sleep(0)
+	}
+
+	// Unblock the goroutine.
+	events <- "release lock"
+	lock.Unlock()
+
+	// Now that we've released the lock, the function is unblocked.  Read
+	// the 2 events.  (This will wait until the function has completed.)
+	firstEvent := <-events
+	secondEvent := <-events
+	if firstEvent != "release lock" || secondEvent != "complete function" {
+		panic(errors.New("function did not obey lock"))
+	}
+
+	// Also, the function must have released the lock.
+	blankLock := sync.Mutex{}
+	if *lock != blankLock {
+		panic(errors.New("function did not release lock"))
+	}
+}

=== added file 'testing/locking_test.go'
--- testing/locking_test.go	1970-01-01 00:00:00 +0000
+++ testing/locking_test.go	2013-06-20 09:42:24 +0000
@@ -0,0 +1,45 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	"errors"
+	. "launchpad.net/gocheck"
+	"sync"
+)
+
+type LockingSuite struct{}
+
+var _ = Suite(&LockingSuite{})
+
+func (LockingSuite) TestTestLockingFunctionPassesCorrectLock(c *C) {
+	lock := sync.Mutex{}
+	function := func() {
+		lock.Lock()
+		lock.Unlock()
+	}
+	// TestLockingFunction succeeds.
+	TestLockingFunction(&lock, function)
+}
+
+func (LockingSuite) TestTestLockingFunctionDetectsDisobeyedLock(c *C) {
+	lock := sync.Mutex{}
+	function := func() {}
+	c.Check(
+		func() { TestLockingFunction(&lock, function) },
+		Panics,
+		errors.New("function did not obey lock"))
+}
+
+func (LockingSuite) TestTestLockingFunctionDetectsFailureToReleaseLock(c *C) {
+	lock := sync.Mutex{}
+	defer lock.Unlock()
+	function := func() {
+		lock.Lock()
+	}
+	c.Check(
+		func() { TestLockingFunction(&lock, function) },
+		Panics,
+		errors.New("function did not release lock"))
+}