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

Way too many events allowed when burst is not 1 #24

Closed
zikaeroh opened this issue Sep 1, 2019 · 4 comments
Closed

Way too many events allowed when burst is not 1 #24

zikaeroh opened this issue Sep 1, 2019 · 4 comments

Comments

@zikaeroh
Copy link

zikaeroh commented Sep 1, 2019

Run:

package main

import (
	"log"
	"time"

	"github.com/go-redis/redis/v7"
	"github.com/go-redis/redis_rate/v8"
)

func main() {
	const (
		perPeriod = 17
		period    = 30 * time.Second
	)

	rdb := redis.NewClient(&redis.Options{
		Addr: "localhost:6379",
	})
	defer rdb.Close()

	limiter := redis_rate.NewLimiter(rdb, &redis_rate.Limit{
		Burst:  perPeriod,
		Rate:   perPeriod,
		Period: period,
	})

	allowed := 0
	start := time.Now()

	for i := 0; i < 30; i++ {
		r, _ := limiter.Allow("key")
		if r.Allowed {
			allowed++
		}

		time.Sleep(time.Second / 4)

		r, _ = limiter.Allow("key")
		if r.Allowed {
			allowed++
		}

		time.Sleep(3 * time.Second / 4)
	}

	log.Printf("Allowed %d in %v, expected %d", allowed, time.Since(start), perPeriod)
}

And I get:

2019/08/31 18:25:53 Allowed 32 in 30.073504489s, expected 17

The rate was set to 17 per 30 seconds, but each run allows much more than 17. Change burst to 1:

2019/08/31 18:28:47 Allowed 18 in 30.079242485s, expected 17

Which makes a little more sense. If you make the burst go even higher, then you can end up allowing every event, but I'm assuming a burst above the rate is not a correct configuration.

@zikaeroh
Copy link
Author

zikaeroh commented Sep 1, 2019

Perhaps I'm just not understanding how this new algorithm operates. I guess the burst is the initial token count and you can have any number of them, such that the use rate won't match the actual rate limit set. Unfortunately not the way I was hoping this package would work...

@zikaeroh
Copy link
Author

zikaeroh commented Sep 1, 2019

I guess I'll close this; I was hoping it'd behave like a rolling window rate limiter, but that isn't the case. Just wrote my own with sorted sets of timestamps instead. 😛

@zikaeroh zikaeroh closed this as completed Sep 1, 2019
@vmihailenco
Copy link
Member

Do you have a link to share? :)

I guess the burst is the initial token count and you can have any number of them, such that the use rate won't match the actual rate limit set.

Yeah, you may think of it as the bucket size with is re-filled at the given rate.

@zikaeroh
Copy link
Author

zikaeroh commented Sep 1, 2019

If you mean a link to my code, not yet, no.

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