-
Notifications
You must be signed in to change notification settings - Fork 363
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
Deprecate EnableRandPool and DisableRandPool #86
Comments
ping @pborman |
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 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)
}
}
})
}
These are also lock-free (or as lock-free as the reader allows) approaches. On my system |
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. |
v1.3.0 introduced the
EnableRandPool
andDisableRandPool
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:
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 text was updated successfully, but these errors were encountered: