pelikan-io / rustcommon

Common Rust library
Apache License 2.0
26 stars 13 forks source link

feat: ratelimit rewrite #37

Closed brayniac closed 1 year ago

brayniac commented 1 year ago

Address the open issues for this crate by rewriting and simplifying further.

Previously reported issues:

Users may expect .wait() to block on the first call immediately after creating a Ratelimiter. Fixed by setting the first tick to be in the future so that .wait() will block for roughly one interval. (#33)

Quantum argument did not behave as documented. Fixes the interval calculation logic so that quantum tokens are added at the configured rate. For example, rate 1 and quantum 10 adds 10 tokens every second. Rate 10 and quantum 1 would add 1 token every 100ms. (#35)

Rate limit did not apply after a period of inactivity due to bad logic in the tick() function. Refactored that function to make the implementation more straightforward and eliminate the bugs. (#36)

thinkingfish commented 1 year ago

LGTM. I do wonder about two names:

Personally I'm more inclined to set the rate to be the total number of tokens dispensed per second, and compute the interval using 1 / (rate / quantum) instead of 1 / rate and having the token refill throughput be quantum * rate. But that ship may have sailed and it's not a big deal.

thinkingfish commented 1 year ago

I'm stealing you tick_at naming convention for my Heatmap changes, btw. It's clearer.