From 2c09566ef13fb5556401ddff3c53c3dbc2a42dac Mon Sep 17 00:00:00 2001 From: Zeke Lu Date: Thu, 3 Nov 2022 04:09:16 +0000 Subject: [PATCH] rate: the state of the limiter should not be changed when the requests failed In the following cases, the reserveN is a no-op, and the state of the limiter should not be changed: 1. n exceeds the Limiter's burst size 2. maxFutureReserve is less than the waitDuration Fixes golang/go#52584 Change-Id: I5f38afa5da696bc10178a4bd0640d92062a8b009 GitHub-Last-Rev: e408718f070679209aca64bfbc18e4297777c311 GitHub-Pull-Request: golang/time#22 Reviewed-on: https://go-review.googlesource.com/c/time/+/406154 Reviewed-by: Russ Cox Auto-Submit: Sameer Ajmani TryBot-Result: Gopher Robot Reviewed-by: Sameer Ajmani Auto-Submit: Russ Cox Run-TryBot: Russ Cox --- rate/rate.go | 20 ++++++++------------ rate/rate_test.go | 6 ++++++ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/rate/rate.go b/rate/rate.go index 8f7c29f..f0e0cf3 100644 --- a/rate/rate.go +++ b/rate/rate.go @@ -83,7 +83,7 @@ func (lim *Limiter) Burst() int { // TokensAt returns the number of tokens available at time t. func (lim *Limiter) TokensAt(t time.Time) float64 { lim.mu.Lock() - _, _, tokens := lim.advance(t) // does not mutute lim + _, tokens := lim.advance(t) // does not mutate lim lim.mu.Unlock() return tokens } @@ -183,7 +183,7 @@ func (r *Reservation) CancelAt(t time.Time) { return } // advance time to now - t, _, tokens := r.lim.advance(t) + t, tokens := r.lim.advance(t) // calculate new number of tokens tokens += restoreTokens if burst := float64(r.lim.burst); tokens > burst { @@ -304,7 +304,7 @@ func (lim *Limiter) SetLimitAt(t time.Time, newLimit Limit) { lim.mu.Lock() defer lim.mu.Unlock() - t, _, tokens := lim.advance(t) + t, tokens := lim.advance(t) lim.last = t lim.tokens = tokens @@ -321,7 +321,7 @@ func (lim *Limiter) SetBurstAt(t time.Time, newBurst int) { lim.mu.Lock() defer lim.mu.Unlock() - t, _, tokens := lim.advance(t) + t, tokens := lim.advance(t) lim.last = t lim.tokens = tokens @@ -356,7 +356,7 @@ func (lim *Limiter) reserveN(t time.Time, n int, maxFutureReserve time.Duration) } } - t, last, tokens := lim.advance(t) + t, tokens := lim.advance(t) // Calculate the remaining number of tokens resulting from the request. tokens -= float64(n) @@ -379,15 +379,11 @@ func (lim *Limiter) reserveN(t time.Time, n int, maxFutureReserve time.Duration) if ok { r.tokens = n r.timeToAct = t.Add(waitDuration) - } - // Update state - if ok { + // Update state lim.last = t lim.tokens = tokens lim.lastEvent = r.timeToAct - } else { - lim.last = last } return r @@ -396,7 +392,7 @@ func (lim *Limiter) reserveN(t time.Time, n int, maxFutureReserve time.Duration) // advance calculates and returns an updated state for lim resulting from the passage of time. // lim is not changed. // advance requires that lim.mu is held. -func (lim *Limiter) advance(t time.Time) (newT time.Time, newLast time.Time, newTokens float64) { +func (lim *Limiter) advance(t time.Time) (newT time.Time, newTokens float64) { last := lim.last if t.Before(last) { last = t @@ -409,7 +405,7 @@ func (lim *Limiter) advance(t time.Time) (newT time.Time, newLast time.Time, new if burst := float64(lim.burst); tokens > burst { tokens = burst } - return t, last, tokens + return t, tokens } // durationFromTokens is a unit conversion function from the number of tokens to the duration diff --git a/rate/rate_test.go b/rate/rate_test.go index d0d9149..a063e35 100644 --- a/rate/rate_test.go +++ b/rate/rate_test.go @@ -427,6 +427,12 @@ func TestReserveJumpBack(t *testing.T) { runReserve(t, lim, request{t1, 2, t1, true}) // start at t1 runReserve(t, lim, request{t0, 1, t1, true}) // should violate Limit,Burst runReserve(t, lim, request{t2, 2, t3, true}) + // burst size is 2, so n=3 always fails, and the state of lim should not be changed + runReserve(t, lim, request{t0, 3, time.Time{}, false}) + runReserve(t, lim, request{t2, 1, t4, true}) + // the maxReserve is not enough so it fails, and the state of lim should not be changed + runReserveMax(t, lim, request{t0, 2, time.Time{}, false}, d) + runReserve(t, lim, request{t2, 1, t5, true}) } func TestReserveJumpBackCancel(t *testing.T) {