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: e408718
GitHub-Pull-Request: #22
Reviewed-on: https://go-review.googlesource.com/c/time/+/406154
Reviewed-by: Russ Cox <[email protected]>
Auto-Submit: Sameer Ajmani <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Sameer Ajmani <[email protected]>
Auto-Submit: Russ Cox <[email protected]>
Run-TryBot: Russ Cox <[email protected]>

Author: ZekeLu

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

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) {