Fix data race with concurrent map read and write

This fixes grafana#207

lib/models.go

@@ -148,46 +148,35 @@ func NewGroup(name string, parent *Group) (*Group, error) {
 	}, nil
 }
 
-// Create a child group belonging to this group.
+// Group creates a child group belonging to this group.
 // This is safe to call from multiple goroutines simultaneously.
 func (g *Group) Group(name string) (*Group, error) {
-	snapshot := g.Groups
-	group, ok := snapshot[name]
+	g.groupMutex.Lock()
+	defer g.groupMutex.Unlock()
+	group, ok := g.Groups[name]
 	if !ok {
-		g.groupMutex.Lock()
-		defer g.groupMutex.Unlock()
-
-		group, ok := g.Groups[name]
-		if !ok {
-			group, err := NewGroup(name, g)
-			if err != nil {
-				return nil, err
-			}
-			g.Groups[name] = group
-			return group, nil
+		group, err := NewGroup(name, g)
+		if err != nil {
+			return nil, err
 		}
+		g.Groups[name] = group
 		return group, nil
 	}
 	return group, nil
 }
 
-// Create a check belonging to this group.
+// Check creates a chold check belonging to this group.
 // This is safe to call from multiple goroutines simultaneously.
 func (g *Group) Check(name string) (*Check, error) {
-	snapshot := g.Checks
-	check, ok := snapshot[name]
+	g.checkMutex.Lock()
+	defer g.checkMutex.Unlock()
+	check, ok := g.Checks[name]
 	if !ok {
-		g.checkMutex.Lock()
-		defer g.checkMutex.Unlock()
-		check, ok := g.Checks[name]
-		if !ok {
-			check, err := NewCheck(name, g)
-			if err != nil {
-				return nil, err
-			}
-			g.Checks[name] = check
-			return check, nil
+		check, err := NewCheck(name, g)
+		if err != nil {
+			return nil, err
 		}
+		g.Checks[name] = check
 		return check, nil
 	}
 	return check, nil

lib/models_test.go

@@ -22,6 +22,7 @@ package lib
 
 import (
 	"encoding/json"
+	"sync"
 	"testing"
 	"time"
 
@@ -41,3 +42,49 @@ func TestStageJSON(t *testing.T) {
 	assert.NoError(t, json.Unmarshal(data, &s2))
 	assert.Equal(t, s, s2)
 }
+
+// Suggested by @nkovacs in https://github.com/loadimpact/k6/issues/207#issuecomment-330545467
+func TestDataRaces(t *testing.T) {
+	t.Run("Check race", func(t *testing.T) {
+		group, err := NewGroup("test", nil)
+		assert.Nil(t, err, "NewGroup")
+		wg := sync.WaitGroup{}
+		wg.Add(2)
+		var check1, check2 *Check
+		go func() {
+			var err error // using the outer err would result in a data race
+			check1, err = group.Check("race")
+			assert.Nil(t, err, "Check 1")
+			wg.Done()
+		}()
+		go func() {
+			var err error
+			check2, err = group.Check("race")
+			assert.Nil(t, err, "Check 2")
+			wg.Done()
+		}()
+		wg.Wait()
+		assert.Equal(t, check1, check2, "Checks are the same")
+	})
+	t.Run("Group race", func(t *testing.T) {
+		group, err := NewGroup("test", nil)
+		assert.Nil(t, err, "NewGroup")
+		wg := sync.WaitGroup{}
+		wg.Add(2)
+		var group1, group2 *Group
+		go func() {
+			var err error
+			group1, err = group.Group("race")
+			assert.Nil(t, err, "Group 1")
+			wg.Done()
+		}()
+		go func() {
+			var err error
+			group2, err = group.Group("race")
+			assert.Nil(t, err, "Group 2")
+			wg.Done()
+		}()
+		wg.Wait()
+		assert.Equal(t, group1, group2, "Groups are the same")
+	})
+}