Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(71)

Issue 6926045: state/presence: fix data race on fakeOffset

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years ago by dave
Modified:
13 years ago
Reviewers:
mp+139144, fwereade, niemeyer
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 1

Patch Set 2 : state/presence: fix data race on fakeOffset #

Total comments: 4

Patch Set 3 : state/presence: fix data race on fakeOffset #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -2 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M state/presence/presence.go View 1 2 2 chunks +11 lines, -2 lines 0 comments Download

Messages

Total messages: 8
dave_cheney.net
Please take a look.
13 years ago (2012-12-11 05:37:30 UTC) #1
jameinel
This doesn't seem to fix a "race", in that you don't guarantee that you'll have ...
13 years ago (2012-12-11 05:52:08 UTC) #2
dave_cheney.net
On 2012/12/11 05:52:08, jameinel wrote: > This doesn't seem to fix a "race", in that ...
13 years ago (2012-12-11 06:08:50 UTC) #3
dave_cheney.net
Please take a look.
13 years ago (2012-12-11 06:12:00 UTC) #4
fwereade
LGTM
13 years ago (2012-12-11 09:31:05 UTC) #5
rog
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): ...
13 years ago (2012-12-11 11:48:29 UTC) #6
niemeyer
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 ...
13 years ago (2012-12-14 17:17:46 UTC) #7
dave_cheney.net
13 years ago (2012-12-18 06:40:19 UTC) #8
*** 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b