Helper: test a function for correct locking.

Helper: test a function for correct locking.

(The diff still includes that of https://codereview.appspot.com/10433043 for which I haven't had a second review vote yet — please ignore that part here, and focus on testLockingFunction(), its uses, and its tests.)

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://code.launchpad.net/~jtv/juju-core/generic-locking-test/+merge/170477

(do not edit description out of merge proposal)

[jtv.canonical, 2013-06-20 02:21:14]

Please take a look.

[thumper, 2013-06-20 02:47:22]

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...
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...
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...
environs/azure/environ_test.go:74: blankLock := sync.Mutex{}
Wow, surprised there is not a way to ask if a mutex is locked.

[jtv.canonical, 2013-06-20 08:39:29]

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

Thanks!  Yes, this is beginning to look rather like a checker.  For now though,
I have to pull my head out of the rabbit hole and unblock my team.  :)


> If this is to be a generic testing function, you should move it to the testing
> package and export it.

Done.


> You could have a few sleep(0) type calls, as this should do the same thing.

Done.


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

Yup.  And that's just for a concrete lock type.  The Locker interface, which is
what this really should work on, clearly isn't going to have any extras.

I suspect we *could* test this by just grabbing the lock, and if it's already
held, we'll get an instant deadlock panic.  But that may not work so well with
multi-thread test runs.  For that situation we might have to come up with some
kind of "try to grab the lock but give up after n microseconds" horror.

[jameinel, 2013-06-20 08:48:08]

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...
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...
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...
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...
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:
   ...
}

[jtv.canonical, 2013-06-20 09:53:06]

On 2013/06/20 08:48:08, jameinel wrote:
> LGTM though maybe it could be tweaked a bit.

Thanks for the review.  I'm in a bottleneck now where I want to unblock my team
in a hurry, so your reviews are really appreciated.


> 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.

Good idea.  Done.


> 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?

I can't add the GoSched calls because the code won't compile on 1.0.  But I
added thumper's sleep(0) calls, with a note that they should be GoSched calls
once we're on 1.1 or better.


>
https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go...
> 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.

Had that originally, but that complicated the testing of the function itself (in
addition to adding the argument of course).  I'm sure there's a way, I just
didn't have time to go yet further down the rabbit hole.


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

Lovely idea.  But rabbit hole, bottleneck.


>
https://codereview.appspot.com/10433043/diff/1/environs/azure/environ_test.go...
> 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'd prefer to worry about this later, when we have time to look into that
tradeoff.  Maybe somebody more familiar with the locking API could even take it
on if I don't find time first.

This is exactly the kind of rabbit hole I was trying to avoid by not making this
more widely reusable: every time the scope broadens it needs just a bit more
additional polish, and then that takes it closer to a possibility to broaden the
scope yet further.  Let's just see how this develops in practice for a bit more,
and not over-generalize it at the cost of the really urgent work.