alisaifee / limits

Rate limiting using various strategies and storage backends such as redis & memcached
https://limits.readthedocs.org
MIT License
428 stars 58 forks source link

`get_window_stats` for `MovingWindowRateLimiter` returns `reset_time` based on the newest item #200

Closed skalpel closed 9 months ago

skalpel commented 9 months ago

For the following code:

limiter = MovingWindowRateLimiter(storage=(MemoryStorage()))
rate_limit = RateLimitItemPerMinute(2)
# hit 1
limiter.hit(rate_limit, "some_id")
time.sleep(10)
# hit 2
limiter.hit(rate_limit, "some_id")
time.sleep(10)
# hit 3
limiter.hit(rate_limit, "some_id")
reset_time = limiter.get_window_stats(rate_limit, "some_id").reset_time
retry_after = reset_time - int(time.time())

retry_after is 50, whereas in this case it should probably be 40, because the event from hit 1 will be removed after 40 seconds, opening the window for the next event (and indeed, when 40 seconds have passed, limiter.hit will return True).

Current Behaviour

For MemoryStorage.get_moving_window events are being iterated from the newest to the oldest one, so the newest one is used as the beginning of the window, while probably the oldest one should be used.

Your Environment

alisaifee commented 9 months ago

Thank you! It took me a moment, but I believe you are absolutely right and this bug exists in all the storages (async & sync). I'll put up a PR and hope you can help review to make sure we get this right, this time.

alisaifee commented 9 months ago

Thinking about it again: reset_time is technically better tied to the newest entry expiring as that is when the full limit will be available again. Deriving retry_after from reset_time isn't completely accurate since there will be some capacity before that time.

EDIT: scratch the above thought - I haven't fully thought that through.

skalpel commented 9 months ago

Thanks a lot! Please let me know if you need any help including review, testing etc.

alisaifee commented 9 months ago

Resolved in #202