state/presence: fix data race on fakeOffset

state/presence: fix data race on fakeOffset



https://code.launchpad.net/~dave-cheney/juju-core/062-state-presence-fix-race/+merge/139144

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-12-11 05:37:30]

Please take a look.

[jameinel, 2012-12-11 05:52:08]

This doesn't seem to fix a "race", in that you don't guarantee that you'll have
set the offset by the time value() is called. It does make the changing of
offset atomic.

Do you actually need to order the set before value?

https://codereview.appspot.com/6926045/diff/1/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6926045/diff/1/state/presence/presence.go#newc...
state/presence/presence.go:624: func (o *offset) value() int { o.Lock(); defer
o.Unlock(); return o.v }
Why do you use defer for value but not for set? (mostly just a go question). I
realize you can't do just Unlock in value, because it happens "after" the
return. By why not use it in set()?

[dave_cheney.net, 2012-12-11 06:08:50]

On 2012/12/11 05:52:08, jameinel wrote:
> This doesn't seem to fix a "race", in that you don't guarantee that you'll
have
> set the offset by the time value() is called. It does make the changing of
> offset atomic.

Correct, this is just fixing the data race on the fakeOffset package level
variable, not any potential logic error.

> Do you actually need to order the set before value?

Dunno, the test was passing before and after this change. This change cleans up
the potential data race so we can see if further more serious ones exist.

> Why do you use defer for value but not for set? (mostly just a go question). I
> realize you can't do just Unlock in value, because it happens "after" the
> return. By why not use it in set()?

Defer comes with a small cost (a closure is allocated on the heap), for
consistency I will use the same form for both calls.

[dave_cheney.net, 2012-12-11 06:12:00]

Please take a look.

[fwereade, 2012-12-11 09:31:05]

LGTM

[rog, 2012-12-11 11:48:29]

not keen on this one. a suggestion for an alternative below.

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go#n...
state/presence/presence.go:623: func (o *offset) set(v int)  { o.Lock(); defer
o.Unlock(); o.v = v }
i'm not sure about this. why does fakeOffset require a mutex but not fakeNow?

i'd like to see less test-related cruft in the real code, not more.

how about making the code in this file always access time.Now through a
globally-defined function pointer?

e.g.
var now = time.Now

then tests can change that function (before any possibility of a race) to return
faked-up values from a mutexed variable as required.

and we can remove all the fake stuff from this file into test files, where it
belongs.

[niemeyer, 2012-12-14 17:17:46]

LGTM in advance if the branch is changed to do just the suggested below.

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go#n...
state/presence/presence.go:627: var fakeOffset offset
Dave, can you please just declare a fakeMutex variable here, and protect all of
timeSlot, fakeTimeSlot and realTimeSlot on it? This will deal with the race
detector on both variables, and is a trivial change.

Roger, Dave is just trying to get the race detector happy, and code is trivial
and small here. Refactoring all tests to a different preferred flavor is out of
scope I think.

John (and Dave), just for the record, both of the deferreds that were used in
this branch are misuses in my opinion. There's little reason to defer when
there's a single line being protected that can't possibly fail except for a
compiler bug. That said, the logic I suggested above can make good use of
defers, since the logic is going to be more involved and include calls to remove
logic that we can't be certain about.

[dave_cheney.net, 2012-12-18 06:40:19]

*** Submitted:

state/presence: fix data race on fakeOffset

R=jameinel, fwereade, rog, niemeyer
CC=
https://codereview.appspot.com/6926045

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go
File state/presence/presence.go (right):

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go#n...
state/presence/presence.go:623: func (o *offset) set(v int)  { o.Lock(); defer
o.Unlock(); o.v = v }
On 2012/12/11 11:48:29, rog wrote:
> i'm not sure about this. why does fakeOffset require a mutex but not fakeNow?
>

fakeNow is not mutated during the test, only fakeOffset via calls to
FakeTImeSlot() while the presence watcher is running.
 
> i'd like to see less test-related cruft in the real code, not more.
> 
> how about making the code in this file always access time.Now through a
> globally-defined function pointer?
> 
> e.g.
> var now = time.Now
> 
> then tests can change that function (before any possibility of a race) to
return
> faked-up values from a mutexed variable as required.
> 
> and we can remove all the fake stuff from this file into test files, where it
> belongs.

https://codereview.appspot.com/6926045/diff/4002/state/presence/presence.go#n...
state/presence/presence.go:627: var fakeOffset offset
On 2012/12/14 17:17:47, niemeyer wrote:
> Dave, can you please just declare a fakeMutex variable here, and protect all
of
> timeSlot, fakeTimeSlot and realTimeSlot on it? This will deal with the race
> detector on both variables, and is a trivial change.
> 
> Roger, Dave is just trying to get the race detector happy, and code is trivial
> and small here. Refactoring all tests to a different preferred flavor is out
of
> scope I think.
> 
> John (and Dave), just for the record, both of the deferreds that were used in
> this branch are misuses in my opinion. There's little reason to defer when
> there's a single line being protected that can't possibly fail except for a
> compiler bug. That said, the logic I suggested above can make good use of
> defers, since the logic is going to be more involved and include calls to
remove
> logic that we can't be certain about.

Done.