Eagerly check for store validity

Ref: #315

`respond_to?` is a relatively expensive method that doesn't
benefit from inline caching, so it has to keep looking up
the method every time. Hence it's best to avoid it in hotspots.

In this case, `@store` can only change through the `store=` method,
so we might as well eagerly validate it there.

First because this way it's a one time fixed cost, but also because
this way the error is raised early during boot/configuration rather
than at runtime where it has availability impact.

Author: byroot

.github/workflows/build.yml

@@ -18,6 +18,7 @@ jobs:
       fail-fast: false
       matrix:
         ruby:
+          - '4.0'
           - '3.4'
           - '3.3'
           - '3.2'

lib/rack/attack/cache.rb

@@ -26,6 +26,12 @@ def store=(store)
           else
             store
           end
+        if @store
+          enforce_store_method_presence!(:read)
+          enforce_store_method_presence!(:write)
+          enforce_store_method_presence!(:delete)
+          enforce_store_method_presence!(:increment)
+        end
       end
 
       def count(unprefixed_key, period)
@@ -34,13 +40,14 @@ def count(unprefixed_key, period)
       end
 
       def read(unprefixed_key)
-        enforce_store_presence!
-        enforce_store_method_presence!(:read)
+        raise Rack::Attack::MissingStoreError if store.nil?
 
         store.read("#{prefix}:#{unprefixed_key}")
       end
 
       def write(unprefixed_key, value, expires_in)
+        raise Rack::Attack::MissingStoreError if store.nil?
+
         store.write("#{prefix}:#{unprefixed_key}", value, expires_in: expires_in)
       end
 
@@ -74,26 +81,17 @@ def key_and_expiry(unprefixed_key, period)
       end
 
       def do_count(key, expires_in)
-        enforce_store_presence!
-        enforce_store_method_presence!(:increment)
+        raise Rack::Attack::MissingStoreError if store.nil?
 
         result = store.increment(key, 1, expires_in: expires_in)
 
         # NB: Some stores return nil when incrementing uninitialized values
         if result.nil?
-          enforce_store_method_presence!(:write)
-
           store.write(key, 1, expires_in: expires_in)
         end
         result || 1
       end
 
-      def enforce_store_presence!
-        if store.nil?
-          raise Rack::Attack::MissingStoreError
-        end
-      end
-
       def enforce_store_method_presence!(method_name)
         if !store.respond_to?(method_name)
           raise(

rack-attack.gemspec

@@ -29,11 +29,11 @@ Gem::Specification.new do |s|
   s.add_runtime_dependency 'rack', ">= 1.0", "< 4"
 
   s.add_development_dependency 'appraisal', '~> 2.2'
-  s.add_development_dependency "bundler", ">= 1.17", "< 3.0"
   s.add_development_dependency 'minitest', "~> 5.11"
   s.add_development_dependency "minitest-stub-const", "~> 0.6"
   s.add_development_dependency 'rack-test', "~> 2.0"
   s.add_development_dependency 'rake', "~> 13.0"
+  s.add_development_dependency 'ostruct'
   s.add_development_dependency "rubocop", "1.82"
   s.add_development_dependency "rubocop-minitest", "~> 0.38"
   s.add_development_dependency "rubocop-performance", "~> 1.26"

spec/acceptance/cache_store_config_for_allow2ban_spec.rb

@@ -27,13 +27,13 @@
       def write(key, value); end
 
       def increment(key, count, options = {}); end
+
+      def delete(key); end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
       raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/scarce-resource"
+        Rack::Attack.cache.store = FakeStore.new
       end
     end
 
@@ -47,13 +47,13 @@ def increment(key, count, options = {}); end
       def read(key); end
 
       def increment(key, count, options = {}); end
+
+      def delete(key); end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
       raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/scarce-resource"
+        Rack::Attack.cache.store = FakeStore.new
       end
     end
 
@@ -67,13 +67,13 @@ def increment(key, count, options = {}); end
       def read(key); end
 
       def write(key, value); end
+
+      def delete(key); end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
       raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/scarce-resource"
+        Rack::Attack.cache.store = FakeStore.new
       end
     end
 
@@ -100,6 +100,10 @@ def increment(key, _count, _options = {})
         @backend[key] ||= 0
         @backend[key] += 1
       end
+
+      def delete(key)
+        @backend.delete(key)
+      end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do

spec/acceptance/cache_store_config_for_fail2ban_spec.rb

@@ -30,10 +30,8 @@ def increment(key, count, options = {}); end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
       raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/private-place"
+        Rack::Attack.cache.store = FakeStore.new
       end
     end
 
@@ -47,13 +45,13 @@ def increment(key, count, options = {}); end
       def read(key); end
 
       def increment(key, count, options = {}); end
+
+      def delete(key); end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
       raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/private-place"
+        Rack::Attack.cache.store = FakeStore.new
       end
     end
 
@@ -67,13 +65,13 @@ def increment(key, count, options = {}); end
       def read(key); end
 
       def write(key, value); end
+
+      def delete(key); end
     end
 
     Object.stub_const(:FakeStore, fake_store_class) do
-      Rack::Attack.cache.store = FakeStore.new
-
       raised_exception = assert_raises(Rack::Attack::MisconfiguredStoreError) do
-        get "/private-place"
+        Rack::Attack.cache.store = FakeStore.new
       end
     end
 
@@ -100,6 +98,10 @@ def increment(key, _count, _options = {})
         @backend[key] ||= 0
         @backend[key] += 1
       end
+
+      def delete(key)
+        @backend.delete(key)
+      end
     end
 
     Rack::Attack.cache.store = fake_store_class.new

spec/acceptance/cache_store_config_for_throttle_spec.rb

@@ -18,10 +18,8 @@
   end
 
   it "gives semantic error if incompatible store was configured" do
-    Rack::Attack.cache.store = Object.new
-
     assert_raises(Rack::Attack::MisconfiguredStoreError) do
-      get "/", {}, "REMOTE_ADDR" => "1.2.3.4"
+      Rack::Attack.cache.store = Object.new
     end
   end
 
@@ -33,10 +31,22 @@ def initialize
         @counts = {}
       end
 
+      def read(key)
+        @counts[key]
+      end
+
+      def write(key, count)
+        @counts[key] = count
+      end
+
       def increment(key, _count, _options)
         @counts[key] ||= 0
         @counts[key] += 1
       end
+
+      def delete(key)
+        @counts.delete(key)
+      end
     end
 
     Rack::Attack.cache.store = basic_store_class.new

spec/rack_attack_reset_spec.rb

@@ -4,9 +4,22 @@
 
 describe "Rack::Attack.reset!" do
   it "raises an error when is not supported by cache store" do
-    Rack::Attack.cache.store = Class.new
-    assert_raises(Rack::Attack::IncompatibleStoreError) do
-      Rack::Attack.reset!
+    fake_store_class = Class.new do
+      def read(key); end
+
+      def write(key, value); end
+
+      def increment(key, count, options = {}); end
+
+      def delete(key); end
+
+    end
+
+    Object.stub_const(:FakeStore, fake_store_class) do
+      Rack::Attack.cache.store = FakeStore.new
+      assert_raises(Rack::Attack::IncompatibleStoreError) do
+        Rack::Attack.reset!
+      end
     end
   end