From f7ae6d61320ea58cdc07839bd27630f5a0a19748 Mon Sep 17 00:00:00 2001
From: Josh Humphries <2035234+jhump@users.noreply.github.com>
Date: Wed, 7 Jun 2023 17:23:49 -0400
Subject: [PATCH] don't use global rand

---
 balancer/internal/rand.go     | 83 +++++++++++++++++++++++++++++++++++
 balancer/picker/random.go     |  5 ++-
 balancer/picker/roundrobin.go |  5 ++-
 3 files changed, 89 insertions(+), 4 deletions(-)
 create mode 100644 balancer/internal/rand.go

diff --git a/balancer/internal/rand.go b/balancer/internal/rand.go
new file mode 100644
index 0000000..95fc8de
--- /dev/null
+++ b/balancer/internal/rand.go
@@ -0,0 +1,83 @@
+// Copyright 2023 Buf Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package internal
+
+import (
+	"hash/maphash"
+	"math/rand"
+	"sync"
+)
+
+// This is based off discussion in a Reddit thread about creating new instances of
+// rand.Rand that are properly seeded, to avoid the global rand's synchronization
+// overhead. In particular, the use maphash.Hash to create a high-quality seed for
+// creating new instances of *rand.Rand.
+
+// NewRand returns a properly seeded *rand.Rand. The seed is computed using
+// the "hash/maphash" package, which can be used concurrently and is
+// lock-free. Effectively, we're using runtime.fastrand to seed a new
+// rand.Rand.
+func NewRand() *rand.Rand {
+	seed := (&maphash.Hash{}).Sum64()
+	return rand.New(rand.NewSource(int64(seed))) //nolint:gosec // don't need cryptographic RNG
+}
+
+// NewLockedRand is just like NewRand except the returned value uses a
+// mutex to enable safe usage from concurrent goroutines.
+//
+// Despite having mutex overhead, this is better than using the global rand
+// because you don't have to worry about other code linked into the same
+// program that might be abusing the global rand:
+//  1. It is possible for code to call rand.Seed, which could cause issues
+//     with the quality of the pseudo-random number sequence.
+//  2. It is possible for code to make *heavy* use of the global rand, which
+//     can mean extensive lock contention on its mutex, which will slow down
+//     clients trying to generate a random number.
+//
+// By creating a new locked *rand.Rand, nothing else will be using it and
+// contending over the mutex except other code that has access to the same
+// instance.
+func NewLockedRand() *rand.Rand {
+	seed := (&maphash.Hash{}).Sum64()
+	//nolint:forcetypeassert,errcheck // specs say value returned by NewSource implements Source64
+	src := rand.NewSource(int64(seed)).(rand.Source64)
+	return rand.New(&lockedSource{src: src}) //nolint:gosec // don't need cryptographic RNG
+}
+
+type lockedSource struct {
+	mu sync.Mutex
+	// +checklocks:mu
+	src rand.Source64
+}
+
+func (l *lockedSource) Int63() int64 {
+	l.mu.Lock()
+	ret := l.src.Int63()
+	l.mu.Unlock()
+	return ret
+}
+
+func (l *lockedSource) Uint64() uint64 {
+	l.mu.Lock()
+	ret := l.src.Uint64()
+	l.mu.Unlock()
+	return ret
+}
+
+func (l *lockedSource) Seed(seed int64) {
+	l.mu.Lock()
+	l.src.Seed(seed)
+	l.mu.Unlock()
+}
diff --git a/balancer/picker/random.go b/balancer/picker/random.go
index bb990b3..9854317 100644
--- a/balancer/picker/random.go
+++ b/balancer/picker/random.go
@@ -15,10 +15,10 @@
 package picker
 
 import (
-	"math/rand"
 	"net/http"
 
 	"github.com/bufbuild/go-http-balancer/balancer/conn"
+	"github.com/bufbuild/go-http-balancer/balancer/internal"
 )
 
 //nolint:gochecknoglobals
@@ -29,7 +29,8 @@ var (
 type randomFactory struct{}
 
 func (r randomFactory) New(_ Picker, allConns conn.Connections) Picker {
+	rnd := internal.NewLockedRand()
 	return pickerFunc(func(*http.Request) (conn conn.Conn, whenDone func(), err error) {
-		return allConns.Get(rand.Intn(allConns.Len())), nil, nil //nolint:gosec
+		return allConns.Get(rnd.Intn(allConns.Len())), nil, nil
 	})
 }
diff --git a/balancer/picker/roundrobin.go b/balancer/picker/roundrobin.go
index 254b787..ba579e7 100644
--- a/balancer/picker/roundrobin.go
+++ b/balancer/picker/roundrobin.go
@@ -15,11 +15,11 @@
 package picker
 
 import (
-	"math/rand"
 	"net/http"
 	"sync/atomic"
 
 	"github.com/bufbuild/go-http-balancer/balancer/conn"
+	"github.com/bufbuild/go-http-balancer/balancer/internal"
 )
 
 type roundRobinFactory struct{}
@@ -39,10 +39,11 @@ func NewRoundRobinFactory() Factory {
 }
 
 func (f roundRobinFactory) New(_ Picker, allConns conn.Connections) Picker {
+	rnd := internal.NewRand()
 	numConns := allConns.Len()
 	conns := make([]conn.Conn, numConns)
 	for i := 0; i < numConns; i++ {
-		j := rand.Intn(i + 1) //nolint:gosec
+		j := rnd.Intn(i + 1)
 		conns[i] = conns[j]
 		conns[j] = allConns.Get(i)
 	}