[perfloop]

@peterbourgon @tsenart

DefaultEntropy returns an io.Reader whose dynamic value is *LockedMonotonicReader, but the promoted Read path bypassed the mutex used by MonotonicRead. A pointer-receiver Read now takes the same mutex and forwards the inner reader's bytes, count, and error unchanged. This also synchronizes callers who construct LockedMonotonicReader themselves.

New still sees the default reader as a MonotonicReader, so Make keeps its monotonic-generation path. The pointer form satisfies io.Reader; the value form no longer does. A copied value cannot share the mutex and does not implement MonotonicReader, so use the pointer form.

TestDefaultEntropyConcurrentReadAndMake exercises the public default operations under the race detector. TestLockedMonotonicReaderConcurrentReadAndNew covers a caller-constructed wrapper. TestLockedMonotonicReaderRead checks byte and error passthrough, and TestDefaultEntropyMonotonicReader protects dispatch. BenchmarkDefaultEntropyRead in ulid_test.go measures serial public reads. The separate race-comparison harness is not part of this pull request.

This condition holds with this change and does not hold at 75921a7395850ea47ed4dfd832184a5a294c8fde: Concurrent calls to Make and Read on the reader returned by DefaultEntropy do not race on the wrapped entropy state, assuming the wrapped reader is accessed only through LockedMonotonicReader and is not concurrently replaced..

The Unreleased changelog records the fix. On Linux, all packages built, and formatting, vet, static analysis, the race-enabled test suite, and the native benchmark passed. The macOS and Windows race-test matrix jobs did not run here because this environment is Linux-only; those jobs remain target-CI work.

---

Generated by Perfloop.

[peterbourgon]

Thanks for the spot on the race.

I think a simpler fix is to just add a pointer-receiver Read method to LockedMonotonicReader itself, and leave defaultEntropy as it is:

// Read synchronizes calls to the wrapped MonotonicReader.
func (r *LockedMonotonicReader) Read(p []byte) (int, error) {
	r.mu.Lock()
	defer r.mu.Unlock()
	return r.MonotonicReader.Read(p)
}

(That also fixes LockedMonotonicReaders that callers construct themselves, not just the default one.)

It seems the reason this PR doesn’t take that approach is because it’s trying to preserve the property that a LockedMonotonicReader value implements io.Reader. But AFAICT there’s no reason to do that, as it’s not valid in the first place:

• Using the type by value copies its sync.Mutex -- go vet flags this, and each copy’s lock protects nothing
• MonotonicRead has a pointer receiver, so a value isn’t a MonotonicReader -- New falls back to plain io.ReadFull, with no locking and no monotonicity

So the only code that would break is code that’s already silently broken.

wdyt?

[perfloop]

Response to this feedback.

Agreed. I’ll add Read directly to *LockedMonotonicReader using the existing mutex, rather than wrap only the default reader. That covers callers’ constructed wrappers too. A value receiver would copy the mutex, and the value does not implement MonotonicReader because MonotonicRead has a pointer receiver, so preserving io.Reader on the value is not a useful compatibility guarantee. I’ll verify the ordinary pointer-backed path and value-form behavior against the source and tests before recording the repair.

[perfloop]

Addressed in 4c483c3f6151bfc81cca98a6ef5a28cc99535340. Response to this feedback.

Added a pointer-receiver Read directly to LockedMonotonicReader using the existing mu, so every constructed wrapper serializes direct reads with monotonic reads. The default reader still dispatches through MonotonicRead. I removed the value-form io.Reader assertion; a copied mutex cannot provide synchronization, and the value does not satisfy MonotonicReader. The new tests cover caller-created wrappers and preserve byte/error results.

[tsenart]

I believe this is ready @peterbourgon