Allow a way for the rate limiter to return error without blocking if enough tokens are not available about leaky-bucket HOT 10 CLOSED

Been testing for couple of days and it's working great. Feel free to close the ticket. Thanks for the quick turnaround

from leaky-bucket.

So something like this?

if !limiter.try_acquire(100) {
    return Err(..);
}

How would you like it to behave if there are tasks waiting for tokens?

For a fair scheduler (the default) I believe the "correct" behavior under that scenario should be for try_acquire to return false, since there are tasks ahead which takes priority. Does this sound right to you?

from leaky-bucket.

Yes, returning false also works. As long as the task knows it can't get the tokens and can try again later, it should do the trick

from leaky-bucket.

There is an implementation in git now if you want to give it a try and report back.

from leaky-bucket.

Thanks, will give it a test. I see that try_acquire returns true if there's enough tokens. Can we change that to actually acquire the tokens. With current try_acquire, its going be like this

if !limiter.try_acquire(100) {
    return Err(..);
}
limiter.acquire(100)

Instead can we do the above in the same try_acquire?

from leaky-bucket.

Ah. That's actually how try_acquire works, the documentation was a bit fuzzy, so I fixed that.

from leaky-bucket.

Awesome, thanks. I will give it a test over the weekend and update

from leaky-bucket.

Sorry, didn't get time to test this. Will test and update asap this week

from leaky-bucket.

@udoprog, I am seeing this behaviour for this code

        let limiter = LeakyRateLimiter::builder()
            .initial(group.global_max_burst.get() as usize)
            .refill(group.request_per_second.get() as usize)
            .interval(Duration::from_millis(1000))
            .max(group.global_max_burst.get() as usize)
            .build();

The rps is am setting is 500 with max burst being 1500. When I test with limiter.try_acquire of lets say as large number of 10000, I get false but after sometime, when I test again, I get true which seems like the bucket keeps accumulating tokens beyond max which seems like a bug. I would assume any request beyond max should return false always

from leaky-bucket.

Ah, yeah. It seems I reproduced an existing issue that I've already fixed in the async version when implementing try_acquire.

This part here should actually be doing this:

state.balance = state.balance.saturating_add(tokens).min(self.max);

Thanks for testing it out!

I can't fix it right now, but if you want to do a PR, feel free!

from leaky-bucket.