Skip to content
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

Fix worker random race condition #179

Merged
merged 1 commit into from Jan 28, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 16 additions & 8 deletions statsd/worker.go
Expand Up @@ -7,21 +7,23 @@ import (
)

type worker struct {
pool *bufferPool
buffer *statsdBuffer
sender *sender
random *rand.Rand
pool *bufferPool
buffer *statsdBuffer
sender *sender
random *rand.Rand
randomLock sync.Mutex
sync.Mutex

inputMetrics chan metric
stop chan struct{}
}

func newWorker(pool *bufferPool, sender *sender) *worker {
// Each worker uses its own random source to prevent workers in separate
// goroutines from contending for the lock on the "math/rand" package-global
// random source (e.g. calls like "rand.Float64()" must acquire a shared
// lock to get the next pseudorandom number).
// Each worker uses its own random source and random lock to prevent
// workers in separate goroutines from contending for the lock on the
// "math/rand" package-global random source (e.g. calls like
// "rand.Float64()" must acquire a shared lock to get the next
// pseudorandom number).
// Note that calling "time.Now().UnixNano()" repeatedly quickly may return
// very similar values. That's fine for seeding the worker-specific random
// source because we just need an evenly distributed stream of float values.
Expand Down Expand Up @@ -71,9 +73,15 @@ func (w *worker) processMetric(m metric) error {
}

func (w *worker) shouldSample(rate float64) bool {
// rand.NewSource is not thread safe.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: for clarity, I suggest:

Suggested change
// rand.NewSource is not thread safe.
// sources created by rand.NewSource() (ie. w.random) are not thread safe.

// TODO: use defer once the lowest Go version we support is 1.14 (defer
// has an overhead before that).
w.randomLock.Lock()
if rate < 1 && w.random.Float64() > rate {
w.randomLock.Unlock()
return false
}
w.randomLock.Unlock()
hush-hush marked this conversation as resolved.
Show resolved Hide resolved
return true
}

Expand Down