Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TestMonotonicSafe fails sometimes #44

Merged
merged 6 commits into from
Apr 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions ulid.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,15 @@ var (
ErrScanValue = errors.New("ulid: source value must be a string or byte slice")
)

// MonotonicReader is an interface that should yield monotonically increasing
// entropy into the provided slice for all calls with the same ms parameter. If
// a MonotonicReader is provided to the New constructor, its MonotonicRead
// method will be used instead of Read.
type MonotonicReader interface {
io.Reader
MonotonicRead(ms uint64, p []byte) error
}

// New returns an ULID with the given Unix milliseconds timestamp and an
// optional entropy source. Use the Timestamp function to convert
// a time.Time to Unix milliseconds.
Expand All @@ -93,7 +102,7 @@ func New(ms uint64, entropy io.Reader) (id ULID, err error) {
switch e := entropy.(type) {
case nil:
return id, err
case *monotonic:
case MonotonicReader:
err = e.MonotonicRead(ms, id[6:])
default:
_, err = io.ReadFull(e, id[6:])
Expand Down Expand Up @@ -487,9 +496,9 @@ func (id ULID) Value() (driver.Value, error) {
// secure entropy bytes, then don't go under this default unless you know
// what you're doing.
//
// The returned io.Reader isn't safe for concurrent use.
func Monotonic(entropy io.Reader, inc uint64) io.Reader {
m := monotonic{
// The returned type isn't safe for concurrent use.
func Monotonic(entropy io.Reader, inc uint64) *MonotonicEntropy {
m := MonotonicEntropy{
Reader: bufio.NewReader(entropy),
inc: inc,
}
Expand All @@ -505,7 +514,8 @@ func Monotonic(entropy io.Reader, inc uint64) io.Reader {
return &m
}

type monotonic struct {
// MonotonicEntropy is an opaque type that provides monotonic entropy.
type MonotonicEntropy struct {
io.Reader
ms uint64
inc uint64
Expand All @@ -514,7 +524,8 @@ type monotonic struct {
rng *rand.Rand
}

func (m *monotonic) MonotonicRead(ms uint64, entropy []byte) (err error) {
// MonotonicRead implements the MonotonicReader interface.
func (m *MonotonicEntropy) MonotonicRead(ms uint64, entropy []byte) (err error) {
if !m.entropy.IsZero() && m.ms == ms {
err = m.increment()
m.entropy.AppendTo(entropy)
Expand All @@ -527,7 +538,7 @@ func (m *monotonic) MonotonicRead(ms uint64, entropy []byte) (err error) {

// increment the previous entropy number with a random number
// of up to m.inc (inclusive).
func (m *monotonic) increment() error {
func (m *MonotonicEntropy) increment() error {
if inc, err := m.random(); err != nil {
return err
} else if m.entropy.Add(inc) {
Expand All @@ -539,7 +550,7 @@ func (m *monotonic) increment() error {
// random returns a uniform random value in [1, m.inc), reading entropy
// from m.Reader. When m.inc == 0 || m.inc == 1, it returns 1.
// Adapted from: https://golang.org/pkg/crypto/rand/#Int
func (m *monotonic) random() (inc uint64, err error) {
func (m *MonotonicEntropy) random() (inc uint64, err error) {
if m.inc <= 1 {
return 1, nil
}
Expand Down
50 changes: 50 additions & 0 deletions ulid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"math"
"math/rand"
"strings"
"sync"
"testing"
"testing/iotest"
"testing/quick"
Expand Down Expand Up @@ -581,6 +582,55 @@ func TestMonotonicOverflow(t *testing.T) {
}
}

func TestMonotonicSafe(t *testing.T) {
t.Parallel()

var (
src = rand.NewSource(time.Now().UnixNano())
entropy = rand.New(src)
monotonic = ulid.Monotonic(entropy, 0)
safe = &safeMonotonicReader{MonotonicReader: monotonic}
t0 = ulid.Timestamp(time.Now())
)

errs := make(chan error, 100)
for i := 0; i < cap(errs); i++ {
go func() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa really

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

u0 := ulid.MustNew(t0, safe)
u1 := ulid.MustNew(t0, safe)
for j := 0; j < 1024; j++ {
u0, u1 = u1, ulid.MustNew(t0, safe)
if u0.String() >= u1.String() {
errs <- fmt.Errorf(
"%s (%d %x) >= %s (%d %x)",
u0.String(), u0.Time(), u0.Entropy(),
u1.String(), u1.Time(), u1.Entropy(),
)
return
}
}
errs <- nil
}()
}
for i := 0; i < cap(errs); i++ {
if err := <-errs; err != nil {
t.Fatal(err)
}
}
}

type safeMonotonicReader struct {
mtx sync.Mutex
ulid.MonotonicReader
}

func (r *safeMonotonicReader) MonotonicRead(ms uint64, p []byte) (err error) {
r.mtx.Lock()
err = r.MonotonicReader.MonotonicRead(ms, p)
r.mtx.Unlock()
return err
}

func BenchmarkNew(b *testing.B) {
benchmarkMakeULID(b, func(timestamp uint64, entropy io.Reader) {
_, _ = ulid.New(timestamp, entropy)
Expand Down