rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

Do we expect `StandardEWMA.Tick` to be called concurrently? #286

Open localvar opened 2 years ago

localvar commented 2 years ago

Do we expect StandardEWMA.Tick to be called concurrently?

If not, then there's no need to use a mutex and atomic operations on a.init, just below code is fine:

    if a.init == 0 {
        atomic.StoreUint64(&a.rate, math.Float64bits(a.fetchInstantRate())) 
        a.init = 1
    } else {
         a.updateRate(a.fetchInstantRate()) 
    }

if yes, then the current implementation is incorrect, because, after the current goroutine executed L115, other goroutines could execute L103, which creates a race condition with L116 of the current goroutine, and L103 itself could create race conditions between concurrent goroutines.

https://github.com/rcrowley/go-metrics/blob/cf1acfcdf4751e0554ffa765d03e479ec491cad6/ewma.go#L100-L120

localvar commented 2 years ago

After review the code and comments, I don't think StandardEWMA.Tick should be called concurrently, otherwise, it will output wrong results.

for example, after the first tick, even if the race condition is not triggered because we are lucky enough, calling this function from two goroutines at the same time, is just like doing below in a single goroutine:

a.updateRate(a.fetchInstantRate()) // desired update
a.updateRate(a.fetchInstantRate()) // not desired

so we can remove the mutex to make the code simpler and faster.

zeim839 commented 11 months ago

updateRate and fetchInstanceRate are not public functions, so as long as they're not unexpectedly called by the go-metrics library itself, then they wont race.

Tick() is meant to be called by an external clock every 5 seconds. See meter.go: L226-L251