cache the exceeded window using an expiration timestamp thus making reloaded_limit_exceeded? private
#27
Conversation
dhruvCW
changed the title
reloaded_limit_exceeded? privatereloaded_limit_exceeded? private
| @exceeded_window = Window.find_exceeded(windows) | ||
| return if @exceeded_window.nil? | ||
|
|
||
| @exceeded_window_expires_at = Time.now + @exceeded_window.interval |
There was a problem hiding this comment.
Since this is a Window related logic.. would it be better if we expose Window#expires_at method?
There was a problem hiding this comment.
it's kind of difficult for me to decide, since a window doesn't know it has exceeded and thus the cooldown period (expires_at) will always be now + interval.
does this make sense in your design ? if yes I can indeed change it.
There was a problem hiding this comment.
window know the interval, right?
There was a problem hiding this comment.
yup that's where I get the it right now.
There was a problem hiding this comment.
Aha.. I get your point now 👍
What if user initializes another Worker instance in 30 seconds (while the remaining time the window is 10 seconds at that time) ?
There was a problem hiding this comment.
IMO the current issue is Window doesn't know it's exceeded until checked. we could set the expiration timestamp at the moment of checking, which should be okay IMO though then the quest is how do we clear the timestamp.
There was a problem hiding this comment.
we could set the expiration timestamp at the moment of checking
I believe if want to get an accurate timestamp, we need to get if from redis. Because at the moment of checking does not guarantee the accuracy
Example
Window: 1 request per day
interval: 86400
threshold: 1
If we check the limit exceeded 1 hour before the end of the day. it would gives us 24 hours + Time.now
Which in fact should be 1 hours + Time.now
lib/rate_limit/worker.rb
Outdated
| def limit_exceeded? | ||
| reload_limit_exceeded if exceeded_window.nil? || exceeded_window_expires_at >= Time.current | ||
|
|
||
| exceeded_window.present? | ||
| end |
There was a problem hiding this comment.
This makes sense now, since limit_exceeded? is being called only once during the throttle cycle 👍
dhruvCW
force-pushed
the
limit_exceeded
branch
from
29eb2d9 to
2bdaafb
Compare
Description
PR Description goes here!
PR Contains:
Related Issues
TODO
Changelog
Added
Changed
Deprecated
Removed
Fixed
Security