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.

Sign up for GitHub

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

cache the exceeded window using an expiration timestamp thus making reloaded_limit_exceeded? private #27

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+27 −15
Draft

cache the exceeded window using an expiration timestamp thus making reloaded_limit_exceeded? private #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rate_limit/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def throttle(**args)
end

def limit_exceeded?(**args)
Worker.new(**args).reloaded_limit_exceeded?
Worker.new(**args).limit_exceeded?
end

def reset_counters(**args)
Expand Down
2 changes: 1 addition & 1 deletion lib/rate_limit/throttler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module RateLimit
module Throttler
def throttle
return failure! if reloaded_limit_exceeded?
return failure! if limit_exceeded?

yield if block_given?

Expand Down
21 changes: 14 additions & 7 deletions lib/rate_limit/worker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ module RateLimit
class Worker
include Throttler

attr_accessor :topic, :value, :limits, :windows, :exceeded_window, :result, :raise_errors, :only_failures
attr_accessor :topic, :value, :limits, :windows, :result, :raise_errors, :only_failures,
:exceeded_window, :exceeded_window_expires_at

def initialize(topic:, value:, raise_errors: false, only_failures: false)
@topic = topic.to_s
Expand All @@ -23,13 +24,9 @@ def clear_cache_counter
Window.clear_cache_counter(windows)
end

def reloaded_limit_exceeded?
@exceeded_window = Window.find_exceeded(windows)

limit_exceeded?
end

def limit_exceeded?
reload_limit_exceeded if exceeded_window.nil? || exceeded_window_expires_at >= Time.now

exceeded_window.present?
end

Expand All @@ -45,5 +42,15 @@ def failure!

raise Errors::LimitExceededError, result if raise_errors
end

private

def reload_limit_exceeded
@exceeded_window = Window.find_exceeded(windows)
@exceeded_window_expires_at = nil
return if @exceeded_window.nil?

@exceeded_window_expires_at = Time.now + @exceeded_window.interval
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a Window related logic.. would it be better if we expose Window#expires_at method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window know the interval, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup that's where I get the it right now.

Copy link
Contributor

@aka-momo aka-momo Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@aka-momo aka-momo Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
end
end
4 changes: 2 additions & 2 deletions spec/rate_limit/base_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,11 @@
expect(Hash).not_to have_received(:new).with(any_args)
end

it 'exceeded limit for atribute 1' do
it 'exceeded limit for attribute 1' do
expect(described_class.limit_exceeded?(**options)).to be(true)
end

it 'exceeded limit for atribute 2' do
it 'exceeded limit for attribute 2' do
expect(
described_class.limit_exceeded?(topic: topic_login, value: value_five)
).to be(true)
Expand Down
13 changes: 9 additions & 4 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
# frozen_string_literal: true

require 'active_support/core_ext/kernel'
require 'rate_limit'
require 'byebug'
require 'simplecov'

SimpleCov.start
SimpleCov.start do
add_filter '/spec/'

enable_coverage :branch
end

require 'active_support/core_ext/kernel'
require 'byebug'
require 'rate_limit'

RSpec.configure do |config|
# Enable flags like --only-failures and --next-failure
Expand Down