state: fix data race in all watcher

state/allwatcher_internal_test.go

@@ -348,56 +348,57 @@ type changeTestCase struct {
 
 func substNilSinceTimeForStatus(c *gc.C, sInfo *multiwatcher.StatusInfo) {
 	if sInfo.Current != "" {
-		c.Assert(sInfo.Since, gc.NotNil)
+		c.Assert(sInfo.Since, gc.NotNil) // TODO(dfc) WTF does this check do ? separation of concerns much
 	}
 	sInfo.Since = nil
 }
 
-func substNilSinceTimeForStatusNoCheck(sInfo *multiwatcher.StatusInfo) {
-	sInfo.Since = nil
-}
-
 // substNilSinceTimeForEntities zeros out any updated timestamps for unit
 // or service status values so we can easily check the results.
 func substNilSinceTimeForEntities(c *gc.C, entities []multiwatcher.EntityInfo) {
 	// Zero out any updated timestamps for unit or service status values
 	// so we can easily check the results.
-	for i, entity := range entities {
-		if unitInfo, ok := entity.(*multiwatcher.UnitInfo); ok {
+	for i := range entities {
+		switch e := entities[i].(type) {
+		case *multiwatcher.UnitInfo:
+			unitInfo := *e // must copy because this entity came out of the multiwatcher cache.
 			substNilSinceTimeForStatus(c, &unitInfo.WorkloadStatus)
 			substNilSinceTimeForStatus(c, &unitInfo.JujuStatus)
-			entities[i] = unitInfo
-		}
-		if serviceInfo, ok := entity.(*multiwatcher.ServiceInfo); ok {
+			entities[i] = &unitInfo
+		case *multiwatcher.ServiceInfo:
+			serviceInfo := *e // must copy because this entity came out of the multiwatcher cache.
 			substNilSinceTimeForStatus(c, &serviceInfo.Status)
-			entities[i] = serviceInfo
-		}
-		if machineInfo, ok := entity.(*multiwatcher.MachineInfo); ok {
+			entities[i] = &serviceInfo
+		case *multiwatcher.MachineInfo:
+			machineInfo := *e // must copy because this entity came out of the multiwatcher cache.
 			substNilSinceTimeForStatus(c, &machineInfo.JujuStatus)
 			substNilSinceTimeForStatus(c, &machineInfo.MachineStatus)
-			entities[i] = machineInfo
+			entities[i] = &machineInfo
 		}
 	}
 }
 
 func substNilSinceTimeForEntityNoCheck(entity multiwatcher.EntityInfo) multiwatcher.EntityInfo {
 	// Zero out any updated timestamps for unit or service status values
 	// so we can easily check the results.
-	if unitInfo, ok := entity.(*multiwatcher.UnitInfo); ok {
-		substNilSinceTimeForStatusNoCheck(&unitInfo.WorkloadStatus)
-		substNilSinceTimeForStatusNoCheck(&unitInfo.JujuStatus)
-		return unitInfo
-	}
-	if serviceInfo, ok := entity.(*multiwatcher.ServiceInfo); ok {
-		substNilSinceTimeForStatusNoCheck(&serviceInfo.Status)
-		return serviceInfo
+	switch e := entity.(type) {
+	case *multiwatcher.UnitInfo:
+		unitInfo := *e // must copy because this entity came out of the multiwatcher cache.
+		unitInfo.WorkloadStatus.Since = nil
+		unitInfo.JujuStatus.Since = nil
+		return &unitInfo
+	case *multiwatcher.ServiceInfo:
+		serviceInfo := *e // must copy because this entity came out of the multiwatcher cache.
+		serviceInfo.Status.Since = nil
+		return &serviceInfo
+	case *multiwatcher.MachineInfo:
+		machineInfo := *e // must copy because we this entity came out of the multiwatcher cache.
+		machineInfo.JujuStatus.Since = nil
+		machineInfo.MachineStatus.Since = nil
+		return &machineInfo
+	default:
+		return entity
 	}
-	if machineInfo, ok := entity.(*multiwatcher.MachineInfo); ok {
-		substNilSinceTimeForStatusNoCheck(&machineInfo.JujuStatus)
-		substNilSinceTimeForStatusNoCheck(&machineInfo.MachineStatus)
-		return machineInfo
-	}
-	return entity
 }
 
 // changeTestFunc is a function for the preparation of a test and
@@ -621,6 +622,7 @@ func (s *allWatcherStateSuite) TestClosingPorts(c *gc.C) {
 	})
 	c.Assert(err, jc.ErrorIsNil)
 	entities = all.All()
+	substNilSinceTimeForEntities(c, entities)
 	assertEntitiesEqual(c, entities, []multiwatcher.EntityInfo{
 		&multiwatcher.UnitInfo{
 			ModelUUID:      s.state.ModelUUID(),
@@ -1131,23 +1133,19 @@ func (s *allModelWatcherStateSuite) performChangeTestCases(c *gc.C, changeTestFu
 
 			entities = all.All()
 
-			// substNilSinceTimeForEntities gets upset if it sees non-nil
-			// times - which the entities for the first env will have - so
-			// build a list of the entities for the second env.
-			newEntities := make([]multiwatcher.EntityInfo, 0)
-			for _, entity := range entities {
-				if entity.EntityId().ModelUUID == s.state1.ModelUUID() {
-					newEntities = append(newEntities, entity)
-				}
-			}
-			substNilSinceTimeForEntities(c, newEntities)
-
 			// Expected to see entities for both envs.
 			var expectedEntities entityInfoSlice = append(
 				test0.expectContents,
 				test1.expectContents...)
 			sort.Sort(entities)
 			sort.Sort(expectedEntities)
+
+			// for some reason substNilSinceTimeForStatus cares if the Current is not blank
+			// and will abort if it is. Apparently this happens and it's totally fine. So we
+			// must use the NoCheck variant, rather than substNilSinceTimeForEntities(c, entities)
+			for i := range entities {
+				entities[i] = substNilSinceTimeForEntityNoCheck(entities[i])
+			}
 			assertEntitiesEqual(c, entities, expectedEntities)
 		}()
 	}
@@ -1616,13 +1614,16 @@ func (s *allModelWatcherStateSuite) TestStateWatcher(c *gc.C) {
 
 func zeroOutTimestampsForDeltas(c *gc.C, deltas []multiwatcher.Delta) {
 	for i, delta := range deltas {
-		if unitInfo, ok := delta.Entity.(*multiwatcher.UnitInfo); ok {
+		switch e := delta.Entity.(type) {
+		case *multiwatcher.UnitInfo:
+			unitInfo := *e // must copy, we may not own this reference
 			substNilSinceTimeForStatus(c, &unitInfo.WorkloadStatus)
 			substNilSinceTimeForStatus(c, &unitInfo.JujuStatus)
-			delta.Entity = unitInfo
-		} else if serviceInfo, ok := delta.Entity.(*multiwatcher.ServiceInfo); ok {
+			delta.Entity = &unitInfo
+		case *multiwatcher.ServiceInfo:
+			serviceInfo := *e // must copy, we may not own this reference
 			substNilSinceTimeForStatus(c, &serviceInfo.Status)
-			delta.Entity = serviceInfo
+			delta.Entity = &serviceInfo
 		}
 		deltas[i] = delta
 	}
@@ -3224,7 +3225,6 @@ func assertEntitiesEqual(c *gc.C, got, want []multiwatcher.EntityInfo) {
 			g := got[i]
 			w := want[i]
 			if !jcDeepEqualsCheck(c, g, w) {
-				firstDiffError += "\n"
 				firstDiffError += fmt.Sprintf("first difference at position %d\n", i)
 				firstDiffError += "got:\n"
 				firstDiffError += fmt.Sprintf("  %T %#v\n", g, g)
@@ -3237,8 +3237,3 @@ func assertEntitiesEqual(c *gc.C, got, want []multiwatcher.EntityInfo) {
 	}
 	c.FailNow()
 }
-
-func deepEqual(c *gc.C, got, want interface{}) bool {
-	same, err := jc.DeepEqual(got, want)
-	return err == nil && same
-}

state/multiwatcher.go

@@ -414,7 +414,7 @@ func (a *multiwatcherStore) All() []multiwatcher.EntityInfo {
 // add adds a new entity with the given id and associated
 // information to the list.
 func (a *multiwatcherStore) add(id interface{}, info multiwatcher.EntityInfo) {
-	if a.entities[id] != nil {
+	if _, ok := a.entities[id]; ok {
 		panic("adding new entry with duplicate id")
 	}
 	a.latestRevno++
@@ -439,8 +439,8 @@ func (a *multiwatcherStore) decRef(entry *entityEntry) {
 		return
 	}
 	id := entry.info.EntityId()
-	elem := a.entities[id]
-	if elem == nil {
+	elem, ok := a.entities[id]
+	if !ok {
 		panic("delete of non-existent entry")
 	}
 	delete(a.entities, id)
@@ -449,8 +449,8 @@ func (a *multiwatcherStore) decRef(entry *entityEntry) {
 
 // delete deletes the entry with the given info id.
 func (a *multiwatcherStore) delete(id multiwatcher.EntityId) {
-	elem := a.entities[id]
-	if elem == nil {
+	elem, ok := a.entities[id]
+	if !ok {
 		return
 	}
 	delete(a.entities, id)
@@ -480,8 +480,8 @@ func (a *multiwatcherStore) Remove(id multiwatcher.EntityId) {
 // Update updates the information for the given entity.
 func (a *multiwatcherStore) Update(info multiwatcher.EntityInfo) {
 	id := info.EntityId()
-	elem := a.entities[id]
-	if elem == nil {
+	elem, ok := a.entities[id]
+	if !ok {
 		a.add(id, info)
 		return
 	}
@@ -498,14 +498,14 @@ func (a *multiwatcherStore) Update(info multiwatcher.EntityInfo) {
 	a.list.MoveToFront(elem)
 }
 
-// Get returns the stored entity with the given
-// id, or nil if none was found. The contents of the returned entity
-// should not be changed.
+// Get returns the stored entity with the given id, or nil if none was found.
+// The contents of the returned entity MUST not be changed.
 func (a *multiwatcherStore) Get(id multiwatcher.EntityId) multiwatcher.EntityInfo {
-	if e := a.entities[id]; e != nil {
-		return e.Value.(*entityEntry).info
+	e, ok := a.entities[id]
+	if !ok {
+		return nil
 	}
-	return nil
+	return e.Value.(*entityEntry).info
 }
 
 // ChangesSince returns any changes that have occurred since

state/package_test.go

@@ -4,18 +4,19 @@
 package state_test
 
 import (
-	stdtesting "testing"
+	"testing"
 
 	"github.com/juju/utils/os"
 
 	coretesting "github.com/juju/juju/testing"
 )
 
-func TestPackage(t *stdtesting.T) {
+func TestPackage(t *testing.T) {
 	// At this stage, Juju only supports running the apiservers and database
 	// on Ubuntu. If we end up officially supporting CentOS, then we should
 	// make sure we run the tests there.
-	if os.HostOS() == os.Ubuntu {
-		coretesting.MgoTestPackage(t)
+	if os.HostOS() != os.Ubuntu {
+		t.Skipf("skipping tests on %v", os.HostOS())
 	}
+	coretesting.MgoTestPackage(t)
 }