TestMonotonicSafe fails sometimes

[peterbourgon]

ulid.Monotonic is not goroutine-safe, so I want to protect it with a mutex via e.g. safeReader. However, when I do that, it no longer reliably produces monotonically-increasing entropy. Here is the test output on my machine.

$ go test -count 10 -run TestMonotonicSafe
--- FAIL: TestMonotonicSafe (0.00s)
    ulid_test.go:599: 01D7JB3BSKDWJJQM6K907K8ZMR (time 1554320043827 entropy 6f252bd0d3480f347e98) >= 01D7JB3BSK480NQ77QR5VBMEV9 (time 1554320043827 entropy 22015b9cf7c176ba3b69)
--- FAIL: TestMonotonicSafe (0.00s)
    ulid_test.go:599: 01D7JB3BSKRBDYR7RJ3JXGBQA6 (time 1554320043827 entropy c2dbec1f121cbb05dd46) >= 01D7JB3BSKCZV1CK492PVYBR30 (time 1554320043827 entropy 67f6164c8915b7e5e060)
--- FAIL: TestMonotonicSafe (0.00s)
    ulid_test.go:599: 01D7JB3BSMWMHW4CR5HTZMXMAX (time 1554320043828 entropy e523c233058ebf4ed15d) >= 01D7JB3BSMKK2ZND39ZCMTSRG1 (time 1554320043828 entropy 9cc5fab469fb29ace201)
--- FAIL: TestMonotonicSafe (0.00s)
    ulid_test.go:599: 01D7JB3BSMYB78B3ZMVNJ1X2NS (time 1554320043828 entropy f2ce858ff4dd641e8ab9) >= 01D7JB3BSMXW5PV5M1G5XK279Y (time 1554320043828 entropy ef0b6d9681817b311d3e)
--- FAIL: TestMonotonicSafe (0.00s)
    ulid_test.go:599: 01D7JB3BSMQJHBF38XFHR9HY1Q (time 1554320043828 entropy bca2b78d1d7c7098f837) >= 01D7JB3BSM5MWQPN17VF676BTP (time 1554320043828 entropy 2d397b5427dbcc732f56)
FAIL
exit status 1
FAIL	github.com/oklog/ulid	0.005s

[peterbourgon]

Interestingly, the following patch causes it to behave as expected.

diff --git a/ulid_test.go b/ulid_test.go
index 91aba2b..6177396 100644
--- a/ulid_test.go
+++ b/ulid_test.go
@@ -584,7 +584,7 @@ func TestMonotonicOverflow(t *testing.T) {
 
 func TestMonotonicSafe(t *testing.T) {
 	var (
-		seed      = time.Now().UnixNano()
+		seed      = int64(12345)
 		src       = rand.NewSource(seed)
 		entropy   = rand.New(src)
 		monotonic = ulid.Monotonic(entropy, 0)

[coveralls]

Coverage remained the same at 97.938% when pulling 2a732c5 on safe-monotonic into 6f8b175 on master.

[peterbourgon]

More results:

• seed = int64(123456) — pass ✓
• seed = int64(1234567) — FAIL ✘
• seed = int64(12345678) — pass ✓
• seed = int64(123456780) — pass ✓
• seed = int64(123456781) — FAIL ✘

[peterbourgon]

I've updated the test case to range across a variety of seed and inc parameters. It fails more often when seed is larger, but I haven't figured out a pattern besides that. And, just to make sure it's clear, the following patch results in 100% success:

diff --git a/ulid_test.go b/ulid_test.go
index 31c39af..f5f3fc4 100644
--- a/ulid_test.go
+++ b/ulid_test.go
@@ -612,12 +612,11 @@ func TestMonotonicSafe(t *testing.T) {
 					src       = rand.NewSource(seed)
 					entropy   = rand.New(src)
 					monotonic = ulid.Monotonic(entropy, inc)
-					safe      = &safeReader{r: monotonic}
 				)
 
 				t0 := time.Now()
-				u0 := ulid.MustNew(ulid.Timestamp(t0), safe)
-				u1 := ulid.MustNew(ulid.Timestamp(t0), safe)
+				u0 := ulid.MustNew(ulid.Timestamp(t0), monotonic)
+				u1 := ulid.MustNew(ulid.Timestamp(t0), monotonic)
 
 				if u0.String() >= u1.String() {
 					t.Fatalf("%s (time %d entropy %x) >= %s (time %d entropy %x)", u0.String(), u0.Time(), u0.Entropy(), u1.String(), u1.Time(), u1.Entropy())

[peterbourgon]

@seebs has spotted the problem. ulid.New does a type assertion on the entropy io.Reader parameter, and checks explicitly for *monotonic:

func New(ms uint64, entropy io.Reader) (id ULID, err error) {
	if err = id.SetTime(ms); err != nil {
		return id, err
	}

	switch e := entropy.(type) {
	case nil:
		return id, err
	case *monotonic: // <--------
		err = e.MonotonicRead(ms, id[6:])
	default:
		_, err = io.ReadFull(e, id[6:])
	}

Since I am passing a *safeReader this check fails.

I am not sure the best way to patch this.

[peterbourgon]

I think that should work. @tsenart let me know if these changes to the package API surface area are acceptable to you.

We could also offer SafeMonotonicReader in the package itself, if you think that would have value.

[golangcibot]

SA2002: the goroutine calls T.Fatalf, which must be called in the same goroutine as the test (from staticcheck)

[peterbourgon]

Whoa really

[seebs]

yup! it's not reliably enforced, but it's there. you have to snag the error (maybe use an ErrGroup) and fatal out from the original goroutine.