segmentio / ksuid

K-Sortable Globally Unique IDs
https://segment.com/blog/a-brief-history-of-the-uuid/
MIT License
4.9k stars 173 forks source link

Go's default random number generators are not safe for concurrent use? #34

Closed Deleplace closed 6 years ago

Deleplace commented 6 years ago

Hi, this is an observation, not a bug.

ksuid.go says

Go's default random number generators are not safe for concurrent use by multiple goroutines, the use of the rander and randBuffer are explicitly synchronized here.

Thus it proceeds to lock its own randMutex.

I have the intuition that crypto/rand.Reader would be broken if it wasn't safe for concurrent use, and that actually it is safe. This seems confirmed by the implementations in rand_unix.go and rand_windows.go which already use their own mutex.

I was curious if removing randMutex would improve the benchmark numbers, but it doesn't.

Still, randMutex might be unnecessary.

achille-roussel commented 6 years ago

Actually there's an optimization to load data into a global buffer to avoid dynamic memory allocations https://github.com/segmentio/ksuid/blob/master/ksuid.go#L221-L226 which is what the randMutex is used for.

Maybe we could change this code to use sync.Pool to manage a pool of buffers instead, but otherwise loading random data into the KSUID value directly causes escape analysis to fail and results in a placing a temporary object on the heap.

Deleplace commented 6 years ago

Ha, now I understand that randMutex actually protects randBuffer. Thanks for the kind explanation!