go-metrics icon indicating copy to clipboard operation
go-metrics copied to clipboard

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

Open localvar opened this issue 4 years ago • 2 comments

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 avatar Sep 14 '21 03:09 localvar

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.

localvar avatar Sep 15 '21 00:09 localvar

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

zeim839 avatar Aug 03 '23 11:08 zeim839