From 41bdc8d44e99f6f2bc4c88d8afabf8d4ebf59958 Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Wed, 8 Mar 2023 20:04:49 +0100 Subject: [PATCH 1/4] Refactor default entropy and fix test --- ulid.go | 20 +++++++------------- ulid_test.go | 4 +++- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/ulid.go b/ulid.go index 81f9c8d..4ef84fa 100644 --- a/ulid.go +++ b/ulid.go @@ -126,24 +126,18 @@ func MustNew(ms uint64, entropy io.Reader) ULID { // DefaultEntropy as the entropy. It may panic if the given time.Time is too // large or too small. func MustNewDefault(t time.Time) ULID { - return MustNew(Timestamp(t), DefaultEntropy()) + return MustNew(Timestamp(t), defaultEntropy) } -var ( - entropy io.Reader - entropyOnce sync.Once -) +var defaultEntropy = func() io.Reader { + rng := rand.New(rand.NewSource(time.Now().UnixNano())) + return &LockedMonotonicReader{MonotonicReader: Monotonic(rng, 0)} +}() // DefaultEntropy returns a thread-safe per process monotonically increasing // entropy source. func DefaultEntropy() io.Reader { - entropyOnce.Do(func() { - rng := rand.New(rand.NewSource(time.Now().UnixNano())) - entropy = &LockedMonotonicReader{ - MonotonicReader: Monotonic(rng, 0), - } - }) - return entropy + return defaultEntropy } // Make returns a ULID with the current time in Unix milliseconds and @@ -152,7 +146,7 @@ func DefaultEntropy() io.Reader { // contention. func Make() (id ULID) { // NOTE: MustNew can't panic since DefaultEntropy never returns an error. - return MustNew(Now(), DefaultEntropy()) + return MustNew(Now(), defaultEntropy) } // Parse parses an encoded ULID, returning an error in case of failure. diff --git a/ulid_test.go b/ulid_test.go index c4cefde..47f3b30 100644 --- a/ulid_test.go +++ b/ulid_test.go @@ -621,7 +621,8 @@ func TestMonotonicSafe(t *testing.T) { t.Parallel() var ( - safe = ulid.DefaultEntropy() + rng = rand.New(rand.NewSource(time.Now().UnixNano())) + safe = &ulid.LockedMonotonicReader{MonotonicReader: ulid.Monotonic(rng, 0)} t0 = ulid.Timestamp(time.Now()) ) @@ -644,6 +645,7 @@ func TestMonotonicSafe(t *testing.T) { errs <- nil }() } + for i := 0; i < cap(errs); i++ { if err := <-errs; err != nil { t.Fatal(err) From ba948e558873ef365724ac06dd4d614f5f36a2d4 Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Wed, 8 Mar 2023 20:07:03 +0100 Subject: [PATCH 2/4] Update staticcheck in test action --- .github/workflows/test.yml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8602a88..66ef27a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,7 +13,7 @@ jobs: - name: vet run: go vet ./... - name: staticcheck - uses: dominikh/staticcheck-action@v1.2.0 + uses: dominikh/staticcheck-action@v1.3.0 with: install-go: false @@ -24,11 +24,11 @@ jobs: os: [ubuntu-latest, macos-latest, windows-latest] runs-on: ${{ matrix.os }} steps: - - name: Install Go - uses: actions/setup-go@v3 - with: - go-version: ${{ matrix.go-version }} - - name: Check out code - uses: actions/checkout@v2 - - name: Test - run: go test -race ./... + - name: Install Go + uses: actions/setup-go@v3 + with: + go-version: ${{ matrix.go-version }} + - name: Check out code + uses: actions/checkout@v2 + - name: Test + run: go test -race ./... From ca5b9c47f7bc3f3695fbd5555b275d9eafe509de Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Wed, 8 Mar 2023 20:09:05 +0100 Subject: [PATCH 3/4] Another fix to the test action --- .github/workflows/test.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 66ef27a..3da9b56 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,12 +6,18 @@ jobs: steps: - name: Check out code uses: actions/checkout@v3 + - name: Install Go uses: actions/setup-go@v3 + with: + go-version: 1.x + - name: fmt run: test -z $(gofmt -l .) + - name: vet run: go vet ./... + - name: staticcheck uses: dominikh/staticcheck-action@v1.3.0 with: @@ -28,7 +34,9 @@ jobs: uses: actions/setup-go@v3 with: go-version: ${{ matrix.go-version }} + - name: Check out code uses: actions/checkout@v2 + - name: Test run: go test -race ./... From 07cd8cd85617403c478fdfd3c8a0e2ddc1e57d22 Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Mon, 27 Mar 2023 21:54:06 +0200 Subject: [PATCH 4/4] Update comments --- ulid.go | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/ulid.go b/ulid.go index 4ef84fa..8f37026 100644 --- a/ulid.go +++ b/ulid.go @@ -533,20 +533,23 @@ func (id ULID) Value() (driver.Value, error) { return id.MarshalBinary() } -// Monotonic returns an entropy source that is guaranteed to yield -// strictly increasing entropy bytes for the same ULID timestamp. -// On conflicts, the previous ULID entropy is incremented with a -// random number between 1 and `inc` (inclusive). +// Monotonic returns a source of entropy that yields strictly increasing entropy +// bytes, to a limit governeed by the `inc` parameter. // -// The provided entropy source must actually yield random bytes or else -// monotonic reads are not guaranteed to terminate, since there isn't -// enough randomness to compute an increment number. +// Specifically, calls to MonotonicRead within the same ULID timestamp return +// entropy incremented by a random number between 1 and `inc` inclusive. If an +// increment results in entropy that would overflow available space, +// MonotonicRead returns ErrMonotonicOverflow. // -// When `inc == 0`, it'll be set to a secure default of `math.MaxUint32`. -// The lower the value of `inc`, the easier the next ULID within the -// same millisecond is to guess. If your code depends on ULIDs having -// secure entropy bytes, then don't go under this default unless you know -// what you're doing. +// Passing `inc == 0` results in the reasonable default `math.MaxUint32`. Lower +// values of `inc` provide more monotonic entropy in a single millisecond, at +// the cost of easier "guessability" of generated ULIDs. If your code depends on +// ULIDs having secure entropy bytes, then it's recommended to use the secure +// default value of `inc == 0`, unless you know what you're doing. +// +// The provided entropy source must actually yield random bytes. Otherwise, +// monotonic reads are not guaranteed to terminate, since there isn't enough +// randomness to compute an increment number. // // The returned type isn't safe for concurrent use. func Monotonic(entropy io.Reader, inc uint64) *MonotonicEntropy { @@ -568,8 +571,8 @@ func Monotonic(entropy io.Reader, inc uint64) *MonotonicEntropy { type rng interface{ Int63n(n int64) int64 } -// LockedMonotonicReader wraps a MonotonicReader with a sync.Mutex for -// safe concurrent use. +// LockedMonotonicReader wraps a MonotonicReader with a sync.Mutex for safe +// concurrent use. type LockedMonotonicReader struct { mu sync.Mutex MonotonicReader