Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bonus Token by adjustavailableTokens #30

Closed
kingsamchen opened this issue Sep 25, 2019 · 3 comments
Closed

Bonus Token by adjustavailableTokens #30

kingsamchen opened this issue Sep 25, 2019 · 3 comments

Comments

@kingsamchen
Copy link
Contributor

I noticed that inside the function adjustavailableTokens()

func (tb *Bucket) adjustavailableTokens(tick int64) {
	if tb.availableTokens >= tb.capacity {
		return
	}
	tb.availableTokens += (tick - tb.latestTick) * tb.quantum
	if tb.availableTokens > tb.capacity {
		tb.availableTokens = tb.capacity
	}
	tb.latestTick = tick
	return
}

the tb.latestTick is not updated if tb.availableTokens >= tb.capacity

That makes the description of the variable latestTick holds the latest tick for which we know the number of tokens in the bucket. not so accurate, IMO.

And I wrote a snippet of code which can produce surprising results:

func main() {
	bucket := ratelimit.NewBucketWithQuantum(time.Second*1, 100, 20)
	fmt.Printf("Avail=%d\n", bucket.Available())

	fmt.Printf("%v\n", time.Now())
	fmt.Printf("Pause wait for 5s\n")
	time.Sleep(time.Second * 5)
	fmt.Printf("%v\n", time.Now())
	fmt.Printf("Avail=%d\n", bucket.Available())

	fmt.Printf("Request token up to 100\n")
	cnt := bucket.TakeAvailable(100)
	fmt.Printf("Token taken=%d\n", cnt)

        // It will surprise you.
	fmt.Printf("Avail=%d\n", bucket.Available())
}

Output

Avail=100                                                                                          
2019-09-26 01:12:47.9410106 +0800 CST m=+0.003992001                                                Pause wait for 5s                                                                                   
2019-09-26 01:12:52.9614404 +0800 CST m=+5.024421801                                                Avail=100                                                                                           
Request token up to 100                                                                             
Token taken=100                                                                                     
Avail=100             

That is, after taken all tokens out of the bucket, the bucket is still full.

Is this by design or just an implementation bug?

@manadart
Copy link
Member

Thanks for this; it does indeed look like a bug. I'll add a fix for it soon.
Alternatively, if you already have one, feel free to propose it.

kingsamchen added a commit to kingsamchen/ratelimit that referenced this issue Sep 28, 2019
Once adjustavailableTokens() is called, the number of tokens should be
updated in accordance with the tick even if the buket is full.

Semantically, extra tokens are discarded though we, in fact, simply
returns from the function.
@kingsamchen
Copy link
Contributor Author

I proposed a PR to fix this, and your review comments are welcome.

I also added a test case for this issue in case future changes might need a regression test.

FYI: I noticed there are some test-cases failures, they might need some careful examination and fix.

manadart added a commit that referenced this issue Oct 2, 2019
Fix bonus tokens by function adjustavailableTokens (#30)
@manadart
Copy link
Member

manadart commented Oct 2, 2019

Resolved via #31.

@manadart manadart closed this as completed Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants