Change https://golang.org/cl/325289 mentions this issue: rate: Avoid precision loss with edge cases

Thanks for reporting. Could you explain how this is related to the loss of precision and why it is expected to work that way? Thanks.

Would you please also have a look on the PR above? Thanks

For the explanation:

The 'lim.tokens' may be end up with any value, so 0.54990492004805558 is possible, which was just a case I ran into. Then, say the time has passed a very long period, Allow() should return true. However, in the method 'advance()', we calculate maxElapsed := lim.limit.durationFromTokens(float64(lim.burst) - lim.tokens), which has float64 multiplication and division. What makes it worse, it uses maxElaspsed to produce delta, which has float64 multiplication. Any of the float64 calculation might have precision loss that results in we having token 0.999999999728, which never exceeds 1. As a result of it, Allow() never returns true

cc @Sajmani @bradfitz

I have a bugreport against kubernetes that comes down to this same issue. The repro we boiled down to is below. The referenced patch above (https://golang.org/cl/325289) does fix the repro case. I am not familiar enough with how this lib operates to say if it is exactly correct or is the best fix.

So I'm here to +1 the issue and ask for someone in-the-know to consider the patch above.

Another repro (let it run 8 loops):

package main

import (
    "fmt"
    "time"

    "golang.org/x/time/rate"
)

func main() {
    minInterval := 1031425 * time.Microsecond
    qps := float32(time.Second) / float32(minInterval) // don't ask
    limiter := rate.NewLimiter(rate.Limit(qps), 2)                              
    for {
        if limiter.Allow() {
            syncFunc()
            time.Sleep(2 * time.Second)
        } else {
            fmt.Printf("limiter said no, retry in 1s\n")
            time.Sleep(time.Second)
        }
    }
}

func syncFunc() {
    fmt.Printf("------------------------------- do syncFunc %v --------------------------------\n\n\n\n", time.Now())
}

Hi everyone. Has anyone had a moment to look at this one?

@thockin, did @josharian's https://go-review.googlesource.com/c/time/+/336469 merged a few days ago resolve this for you?

Yes, it looks like it does.

The unit test from the top post fails before that CL and passes with it.

And your repo 3 comments up seems fine at head. (but it doesn't fail for me before @josharian's change either)

Thanks. I didn't see it merged.