@Morainy You might want to read the documentation for Allow(). It just says whether there is room. You want to use Wait() and you will get your expected behavior.

Guys, this is a real issue and the fix is roughly mentioned in #23145:

--- a/rate/rate.go
+++ b/rate/rate.go
@@ -320,7 +320,7 @@ func (lim *Limiter) reserveN(now time.Time, n int, maxFutureReserve time.Duratio
 		}
 	}
 
-	now, last, tokens := lim.advance(now)
+	now, _, tokens := lim.advance(now)
 
 	// Calculate the remaining number of tokens resulting from the request.
 	tokens -= float64(n)
@@ -350,8 +350,6 @@ func (lim *Limiter) reserveN(now time.Time, n int, maxFutureReserve time.Duratio
 		lim.last = now
 		lim.tokens = tokens
 		lim.lastEvent = r.timeToAct
-	} else {
-		lim.last = last
 	}
 
 	lim.mu.Unlock()

(Now the second return value of (*Limiter).advance is not used anywhere, it can be removed; and the first return value is just the same value as the argument, it can be removed too.)

lim.last should not be updated alone. The comment about this field in rate/rate.go#L60 clearly says last is the last time the limiter's tokens field was updated.

This is the output after applied the fix:

elapsed= 2.0010308s succCount= 20 failCount= 7280081

When Allow is called, time.Now is immediately called before the limiter acquires its lock. So when the limiter holds the lock, the time returned by the time.Now call could be a bit outdated. Technically, this already seems wrong to me. You have to be in the hope that it won't get too racy.

@fraenkel I believe that the documentation for AllowN is incomplete. It doesn't say anywhere that if you call AllowN two or more times without waiting only the first call will return true.

// AllowN reports whether n events may happen at time now.
// Use this method if you intend to drop / skip events that exceed the rate limit.
// Otherwise use Reserve or Wait.

package main

import (
	"fmt"
	"golang.org/x/time/rate"
	"time"
)

func main() {
	limit := rate.Every(100 * time.Millisecond)
	burst := 1
	limiter := rate.NewLimiter(limit, burst)
	for i := 0; i < 5; i++ {
		fmt.Println(limiter.Allow())
	}
	// Output:
	// true
	// false
	// false
	// false
	// false
}

@b97tsk why couldn't this fix be merged into main?

@Morainy I'm not a contributor. I was trying to help. I suggested a fix. It could be wrong.

This is an old package. It's hard to tell if there's any existing application relies on this behavior. So even I am correct, it could still be marked as WON'T FIX. Just my opinion.

It could be a security issue, you know, if the package is used to mitigate DoS attacks or web scraping, it simply won't work.

As a glance, logic you're trying to remove may be related to handling time shift backward (which, you know, may happens).

I can confirm this is an issue. The lowest failCount I could get where succCount is greater than 21 was 1587868: elapsed= 2.0143099s succCount= 21 failCount= 1587868. Unless you expect to rate limit hundreds of thousands of requests per second using a single Limiter, I believe you should be fine. I still hope this issue gets fixed though.

Relevant issue: #52584

And two PRs pending for review:

• fix rate limiter bug time#17
• rate: the state of the limiter should not be changed when the requests failed time#22

Duplicate of #23145