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: https://github.com/leanovate/gopter/pull/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;

• https://github.com/golang/go/issues/318
• https://github.com/golang/go/issues/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.