Using multiple workers causes math/rand to panic intermittently

[jnormington]

Hi @untoldwind,

We've recently started to use gopter (which is a great project thanks) and came across a bug when using multiple workers and a failure occurred it wouldn't return the full test result error, labels or prop args, making hard to understand what generated value caused the test failure. Which I raised a new PR for review: #36

However that exposed another bug unfortunately with math/rand. Essentially rand.New(rand.NewSource(..)) is not thread safe when calling for new rand Int - running across multiple workers and you can validate with this smallest example - it doesn't happen consistently but happens regularly

https://gist.github.com/jnormington/a832472edf9fa0db020ebd028b7849b9
Issues on golang;

• golang/go#318
• golang/go#3611

I've looked at this and there are a few solutions;

1. Use rand.Int... directly from the rand package and set the seed
2. Create a new rand instance with the same seed across multiple workers
3. Copy lockedSource (how golang does it as lockedSource isn't exposed)
   https://github.com/golang/go/blob/master/src/math/rand/rand.go#L371-L410

There are a few problems with 1 & 2 though

1. Means changing the code in a lot of places which I don't feel is great to use rand we lose some control
2. This means multiple workers could generate the same values as they would each have the same seed in a new initialized rand.New(rand.NewSource(..)) call
3. I like three it means very minimal change but gives multiple workers thread safe access to rand source. With the same control with genParams as we have today without a breaking change.

We've updated our vendored version and works great.

So I wanted to get your thoughts on the issue and whether you had any ideas or was happy with this approach or if you had other concerns I haven't thought of. Below is an example of point 3

diff --git a/gen_parameters.go b/gen_parameters.go
index aca147c..a146b10 100644
--- a/gen_parameters.go
+++ b/gen_parameters.go
@@ -51,7 +51,7 @@ func (p *GenParameters) CloneWithSeed(seed int64) *GenParameters {
 		MinSize:        p.MinSize,
 		MaxSize:        p.MaxSize,
 		MaxShrinkCount: p.MaxShrinkCount,
-		Rng:            rand.New(rand.NewSource(seed)),
+		Rng:            rand.New(NewLockedSource(seed)),
 	}
 }
 
@@ -63,6 +63,6 @@ func DefaultGenParameters() *GenParameters {
 		MinSize:        0,
 		MaxSize:        100,
 		MaxShrinkCount: 1000,
-		Rng:            rand.New(rand.NewSource(seed)),
+		Rng:            rand.New(NewLockedSource(seed)),
 	}
 }
diff --git a/rand_source.go b/rand_source.go
new file mode 100644
index 0000000..34f5241
--- /dev/null
+++ b/rand_source.go
@@ -0,0 +1,77 @@
+// Copyright 2009 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+// Taken from golang lockedSource implementation https://github.com/golang/go/blob/master/src/math/rand/rand.go#L371-L410
+
+package gopter
+
+import (
+	"math/rand"
+	"sync"
+)
+
+type lockedSource struct {
+	lk  sync.Mutex
+	src rand.Source64
+}
+
+// NewLockedSource takes a seed and returns a new
+// lockedSource for use with rand.New
+func NewLockedSource(seed int64) *lockedSource {
+	return &lockedSource{
+		src: rand.NewSource(seed).(rand.Source64),
+	}
+}
+
+func (r *lockedSource) Int63() (n int64) {
+	r.lk.Lock()
+	n = r.src.Int63()
+	r.lk.Unlock()
+	return
+}
+
+func (r *lockedSource) Uint64() (n uint64) {
+	r.lk.Lock()
+	n = r.src.Uint64()
+	r.lk.Unlock()
+	return
+}
+
+func (r *lockedSource) Seed(seed int64) {
+	r.lk.Lock()
+	r.src.Seed(seed)
+	r.lk.Unlock()
+}
+
+// seedPos implements Seed for a lockedSource without a race condition.
+func (r *lockedSource) seedPos(seed int64, readPos *int8) {
+	r.lk.Lock()
+	r.src.Seed(seed)
+	*readPos = 0
+	r.lk.Unlock()
+}
+
+// read implements Read for a lockedSource without a race condition.
+func (r *lockedSource) read(p []byte, readVal *int64, readPos *int8) (n int, err error) {
+	r.lk.Lock()
+	n, err = read(p, r.src.Int63, readVal, readPos)
+	r.lk.Unlock()
+	return
+}
+
+func read(p []byte, int63 func() int64, readVal *int64, readPos *int8) (n int, err error) {
+	pos := *readPos
+	val := *readVal
+	for n = 0; n < len(p); n++ {
+		if pos == 0 {
+			val = int63()
+			pos = 7
+		}
+		p[n] = byte(val)
+		val >>= 8
+		pos--
+	}
+	*readPos = pos
+	*readVal = val
+	return
+}

[untoldwind]

I played around a bit and think that the locked source is most likely the best compromise for now.

[jnormington]

Thanks @untoldwind, Just a note we also need to add it to
https://github.com/leanovate/gopter/blob/master/test_parameters.go#L30

which I missed in the original fix/issue but have since fixed in our fork

[untoldwind]

Tanks, obviously missed that as well.