Data race fixes & enable testing with -race

.circleci/config.yml

@@ -37,7 +37,7 @@ jobs:
             export PATH=$GOPATH/bin:$PATH
             echo "mode: set" > coverage.txt
             for pkg in $(go list ./... | grep -v vendor); do
-                go test -timeout 30s -coverprofile=$(echo $pkg | tr / -).coverage $pkg
+                go test -race -timeout 180s -coverprofile=$(echo $pkg | tr / -).coverage $pkg
             done
             grep -h -v "^mode:" *.coverage >> coverage.txt
             rm -f *.coverage

CONTRIBUTING.md

@@ -24,7 +24,7 @@ Contributing code
 If you'd like to contribute code to k6, this is the basic procedure. Make sure to follow the [style guide](#style-guide) described below!
 
 1. Find an issue you'd like to fix. If there is none already, or you'd like to add a feature, please open one and we can talk about how to do it.
-   
+
    Remember, there's more to software development than code; if it's not properly planned, stuff gets messy real fast.
 
 2. Create a fork and open a feature branch - `feature/my-cool-feature` is the classic way to name these, but it really doesn't matter.
@@ -74,22 +74,22 @@ vendorcheck ./...
 
 To exercise the entire test suite:
 ```bash
-go test ./...
+go test -race ./...
 ```
 
 To run the tests of a specific package:
 ```bash
-go test github.com/loadimpact/k6/core
+go test -race github.com/loadimpact/k6/core
 ```
 
 To run just a specific test case use `-run` and pass in a regex that matches the name of the test:
 ```bash
-go test ./... -run ^TestEngineRun$
+go test -race ./... -run ^TestEngineRun$
 ```
 
 Combining the two above we can run a specific test case in a specific package:
 ```bash
-go test github.com/loadimpact/k6/core -run ^TestEngineRun$
+go test -race github.com/loadimpact/k6/core -run ^TestEngineRun$
 ```
 
 Style guide

Makefile

@@ -13,7 +13,7 @@ format:
 .PHONY: check
 check:
 	gometalinter --deadline 10m --config gometalinter.json --vendor ./...
-	go test -timeout 60s ./...
+	go test -race -timeout 180s ./...
 
 .PHONY: docs
 docs:

core/engine.go

@@ -57,7 +57,7 @@ type Engine struct {
 	logger *log.Logger
 
 	Metrics     map[string]*stats.Metric
-	MetricsLock sync.RWMutex
+	MetricsLock sync.Mutex
 
 	// Assigned to metrics upon first received sample.
 	thresholds map[string]stats.Thresholds
@@ -319,7 +319,7 @@ func (e *Engine) processSamples(samples ...stats.Sample) {
 	for i, sample := range samples {
 		m, ok := e.Metrics[sample.Metric.Name]
 		if !ok {
-			m = sample.Metric
+			m = stats.New(sample.Metric.Name, sample.Metric.Type, sample.Metric.Contains)
 			m.Thresholds = e.thresholds[m.Name]
 			m.Submetrics = e.submetrics[m.Name]
 			e.Metrics[m.Name] = m

core/engine_test.go

@@ -23,17 +23,16 @@ package core
 import (
 	"context"
 	"fmt"
-	"net/http/httptest"
 	"testing"
 	"time"
 
 	"github.com/loadimpact/k6/core/local"
 	"github.com/loadimpact/k6/js"
 	"github.com/loadimpact/k6/lib"
+	"github.com/loadimpact/k6/lib/testutils"
 	"github.com/loadimpact/k6/lib/types"
 	"github.com/loadimpact/k6/stats"
 	"github.com/loadimpact/k6/stats/dummy"
-	"github.com/mccutchen/go-httpbin/httpbin"
 	log "github.com/sirupsen/logrus"
 	logtest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/spf13/afero"
@@ -530,41 +529,45 @@ func getMetricSum(samples []stats.Sample, name string) (result float64) {
 	return
 }
 func TestSentReceivedMetrics(t *testing.T) {
-	//t.Parallel()
-	srv := httptest.NewServer(httpbin.NewHTTPBin().Handler())
-	defer srv.Close()
-
-	burl := func(bytecount uint32) string {
-		return fmt.Sprintf(`"%s/bytes/%d"`, srv.URL, bytecount)
-	}
-
+	t.Parallel()
+	tb := testutils.NewHTTPMultiBin(t)
+	defer tb.Cleanup()
+
+	script := []byte(tb.Replacer.Replace(`
+		import http from "k6/http";
+		export default function() {
+			http.get("HTTPBIN_URL/bytes/5000");
+			http.get("HTTPSBIN_URL/bytes/5000");
+			http.batch(["HTTPBIN_URL/bytes/10000", "HTTPBIN_URL/bytes/20000", "HTTPSBIN_URL/bytes/10000"]);
+		}
+	`))
 	expectedSingleData := 50000.0
-	r, err := js.New(&lib.SourceData{
-		Filename: "/script.js",
-		Data: []byte(`
-			import http from "k6/http";
-			export default function() {
-				http.get(` + burl(10000) + `);
-				http.batch([` + burl(10000) + `,` + burl(20000) + `,` + burl(10000) + `]);
-			}
-		`),
-	}, afero.NewMemMapFs(), lib.RuntimeOptions{})
-	require.NoError(t, err)
 
-	testCases := []struct{ Iterations, VUs int64 }{
-		{1, 1}, {1, 2}, {2, 1}, {2, 2}, {3, 1}, {5, 2}, {10, 3}, {25, 2},
+	type testCase struct{ Iterations, VUs int64 }
+	testCases := []testCase{
+		{1, 1}, {1, 2}, {2, 1}, {2, 2}, {3, 1}, {5, 2}, {10, 3}, {25, 2}, {50, 5},
 	}
 
-	for testn, tc := range testCases {
-		t.Run(fmt.Sprintf("SentReceivedMetrics_%d", testn), func(t *testing.T) {
-			//t.Parallel()
+	getTestCase := func(t *testing.T, tc testCase) func(t *testing.T) {
+		return func(t *testing.T) {
+			t.Parallel()
+			r, err := js.New(
+				&lib.SourceData{Filename: "/script.js", Data: script},
+				afero.NewMemMapFs(),
+				lib.RuntimeOptions{},
+			)
+			require.NoError(t, err)
+
 			options := lib.Options{
 				Iterations: null.IntFrom(tc.Iterations),
 				VUs:        null.IntFrom(tc.VUs),
 				VUsMax:     null.IntFrom(tc.VUs),
+				Hosts:      tb.Dialer.Hosts,
+				InsecureSkipTLSVerify: null.BoolFrom(true),
 			}
 			//TODO: test for differences with NoConnectionReuse enabled and disabled
 
+			r.SetOptions(options)
 			engine, err := NewEngine(local.New(r), options)
 			require.NoError(t, err)
 
@@ -576,7 +579,7 @@ func TestSentReceivedMetrics(t *testing.T) {
 			go func() { errC <- engine.Run(ctx) }()
 
 			select {
-			case <-time.After(5 * time.Second):
+			case <-time.After(10 * time.Second):
 				cancel()
 				t.Fatal("Test timed out")
 			case err := <-errC:
@@ -595,6 +598,16 @@ func TestSentReceivedMetrics(t *testing.T) {
 					receivedData,
 				)
 			}
-		})
+		}
 	}
+
+	// This Run will not return until the parallel subtests complete.
+	t.Run("group", func(t *testing.T) {
+		for testn, tc := range testCases {
+			t.Run(
+				fmt.Sprintf("SentReceivedMetrics_%d(%d, %d)", testn, tc.Iterations, tc.VUs),
+				getTestCase(t, tc),
+			)
+		}
+	})
 }

js/bundle.go

@@ -215,7 +215,7 @@ func (b *Bundle) Instantiate() (*BundleInstance, error) {
 // of other things, will potentially thrash data and makes a mess in it if the operation fails.
 func (b *Bundle) instantiate(rt *goja.Runtime, init *InitContext) error {
 	rt.SetFieldNameMapper(common.FieldNameMapper{})
-	rt.SetRandSource(common.DefaultRandSource)
+	rt.SetRandSource(common.NewRandSource())
 
 	if _, err := rt.RunProgram(jslib.CoreJS); err != nil {
 		return err

js/common/randsource.go

@@ -23,18 +23,20 @@ package common
 import (
 	crand "crypto/rand"
 	"encoding/binary"
+	"fmt"
 	"math/rand"
 
 	"github.com/dop251/goja"
-	"github.com/pkg/errors"
 )
 
-var DefaultRandSource = NewRandSource()
-
+// NewRandSource is copied from goja's source code:
+// https://github.com/dop251/goja/blob/master/goja/main.go#L44
+// The returned RandSource is NOT safe for concurrent use:
+// https://golang.org/pkg/math/rand/#NewSource
 func NewRandSource() goja.RandSource {
 	var seed int64
 	if err := binary.Read(crand.Reader, binary.LittleEndian, &seed); err != nil {
-		panic(errors.New("Couldn't read bytes for random seed"))
+		panic(fmt.Errorf("Could not read random bytes: %v", err))
 	}
 	return rand.New(rand.NewSource(seed)).Float64
 }

js/compiler/compiler.go

@@ -21,6 +21,7 @@
 package compiler
 
 import (
+	"sync"
 	"time"
 
 	"github.com/GeertJohan/go.rice"
@@ -52,6 +53,7 @@ type Compiler struct {
 	// JS pointers.
 	this      goja.Value
 	transform goja.Callable
+	mutex     sync.Mutex //TODO: cache goja.CompileAST() in an init() function?
 }
 
 // Constructs a new compiler.
@@ -78,6 +80,9 @@ func (c *Compiler) Transform(src, filename string) (code string, srcmap SourceMa
 	}
 	opts["filename"] = filename
 
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
 	startTime := time.Now()
 	v, err := c.transform(c.this, c.vm.ToValue(src), c.vm.ToValue(opts))
 	if err != nil {

lib/metrics/metrics.go

@@ -24,6 +24,8 @@ import (
 	"github.com/loadimpact/k6/stats"
 )
 
+//TODO: refactor this, using non thread-safe global variables seems like a bad idea for various reasons...
+
 var (
 	// Engine-emitted.
 	VUs               = stats.New("vus", stats.Gauge)

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 child 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")
+	})
+}