Deprecate EnableRandPool and DisableRandPool
rittneje opened this issue · 3 comments
v1.3.0 introduced the EnableRandPool and DisableRandPool functions as a means of optimizing UUID generation at the expense of security. However, there are two main issues with this.
First, as documented, neither of these are thread-safe. However, the described solution is not feasible:
// Both EnableRandPool and DisableRandPool are not thread-safe and should
// only be called when there is no possibility that New or any other
// UUID Version 4 generation function will be called concurrently.
I cannot in general easily account for what all the package initializers of all of my transitive dependencies are doing. Instead of a function called a runtime, this opt-in behavior should be controlled by a build tag, so the pool can be enabled at compile time. Then there is no race condition. (You could also replace the poolEnabled bool with an atomically read/written uint32, which at least resolves the race condition. But the build tag is more air tight.)
Second, as documented, the optimization is not suitable for security sensitive applications. However, again, I cannot account for what all of my transitive dependencies are doing. And in general, only having a global flag that effectively disables security seems like a rather poor design choice. It would be preferable if the decision could be made by the client on a case-by-case basis.
Going forward, it probably makes sense to deprecate the various global constructor functions and replace them with a factory object, similar to net.Dialer. Then this factory can easily be configured with various options that accumulate over time without needing to make new constructor functions.
The pool API looks pretty dangerous and unnecessary to me also.
The speed gain comes almost entirely from the buffered I/O, which did not require a new API. The remaining allocation is the UUID itself which could be improved in a generally-useful way by having a function to fill a received pointer:
func NewRandomFromReader(r io.Reader) (UUID, error) {
var uuid UUID
return uuid, ReadRandom(r, &uuid)
}
func ReadRandom(r io.Reader, uuid *UUID) error {
_, err := io.ReadFull(r, uuid[:])
if err != nil {
return err
}
uuid[6] = (uuid[6] & 0x0f) | 0x40 // Version 4
uuid[8] = (uuid[8] & 0x3f) | 0x80 // Variant is 10
return nil
}func BenchmarkUUID_NewBufio(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
r := bufio.NewReaderSize(rander, randPoolSize)
for pb.Next() {
_, err := NewRandomFromReader(r)
if err != nil {
b.Fatal(err)
}
}
})
}
func BenchmarkUUID_ReadRandom(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
r := bufio.NewReaderSize(rander, randPoolSize)
var uuid UUID
for pb.Next() {
err := ReadRandom(r, &uuid)
if err != nil {
b.Fatal(err)
}
}
})
}BenchmarkUUID_NewBufio-4 6769986 176.0 ns/op 16 B/op 1 allocs/op
BenchmarkUUID_ReadRandom-4 7622210 155.6 ns/op 0 B/op 0 allocs/op
These are also lock-free (or as lock-free as the reader allows) approaches. On my system ReadRandom consistently beats NewPooled, I assume because of less lock contention.
As mentioned i a code review, these should be limited to package main. Libraries that want a different generator should call a different function. The good new is, that unlike Version 1 UUIDs, Version 4 uuids can be safely generated by multiple packages. Version 1 UUIDs are based on state and it is possible (though unlikely) for two packages to produce either the same UUIDs or UUIDs in which time goes backwards.