Fixes expiry calculation of cache keys

[melopilosyan]

Hi!

Big PR. But not to worry. It's not that complicated.
Aims to fix #480 and addresses #380.

The Initial goal was to try to fix the throttle limit reaching up to 2n. See detailed explanation from @peter-roland-toth. But the development end up with dropping proxies in favor of adapters, because this whole thing makes real sense in current form.
Hence, touching 2 problems in one PR.

This is a different design approach. And just like every design it's a trade-off. In this case accuracy and performance vs complexity.
I think the best way to explore this is walk through the concerns that I had implementing it.

From the top-level perspective, RackAttack calculates the TTL of the cache key at the moment based on a given period and instructs the underlying cache server to update the expiry of that key on every increment attempt.
But why bother trying do the job those servers are designed for? What if to set the expiration first time this key is created and let the server expire it.

The application I'm trying to solve this problem for is using Redis. Something like this should do the job.

redis = Redis.new

def increment(redis, key, expires_in)
  redis.synchronize do
    redis.incr(key).tap do |count|
      count == 1 && redis.expire(key, expires_in)
    end
  end
end

10.times.map { increment(r, 'key', 3).tap { sleep(1) } }
# => [1, 2, 3, 1, 2, 3, 1, 2, 3, 1]

More extreme conditions.

15.times.map { increment(r, 'key', 1).tap { sleep(0.1) } }
# => [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 1, 2, 3, 4, 5]

Working. Next step is to integrate it with RackAttack in Rails environment.

Obviously I can't call incr or expire on ActiveSupport::Cache::RedisCacheStore, but definitely can take Redis object from RedisCacheStore and use those methods.

There were a few more places in the application where this increment was needed. Plus from RackAttack the app used only throttle rules. Couple of more tunings to work in development, when Rails.cache is not using Redis, and here we have it. This dead simple interface.

class SimpleCounterCache
  attr_reader :prefix, :store

  def initialize(prefix:)
    @prefix = prefix
    @store = defined?(Rails.cache.redis) ? SimpleRedisCounterStore.instance : SimpleStubCounterStore.new
  end

  def count(key, period)
    period = period.to_i
    store.increment "#{prefix}:#{period}:#{key}", expires_in: period
  end
end

class SimpleRedisCounterStore
  include Singleton

  attr_reader :redis

  def initialize
    @redis = Rails.cache.redis
  end

  def increment(key, expires_in:)
    redis.synchronize do
      redis.incr(key).tap do |count|
        count == 1 && redis.expire(key, expires_in)
      end
    end
  rescue => e
    Appsignal.send_error e
    0
  end
end

class SimpleStubCounterStore
  def increment(*)
    0   
  end 
end

class Rack::Attack
  def self.cache
    @cache ||= SimpleCounterCache.new prefix: 'throttle'
  end

  # the rest of the RackAttack configuration
end

This setup has been successfully working in production for several months.

OK, the pattern is clear. But this is a simple, opinionated case, dropping RedisCacheStore can have consequences for others.

Well, let's see what it does using ActiveSupport 6.1 as an example.

https://github.com/rails/rails/blob/v6.1.4.4/activesupport/lib/active_support/cache/redis_cache_store.rb#L173-L181

def initialize(*args_list)
  @redis_options = redis_options

  @max_key_bytesize = MAX_KEY_BYTESIZE
  @error_handler = error_handler

  super namespace: namespace,
    compress: compress, compress_threshold: compress_threshold,
    expires_in: expires_in, race_condition_ttl: race_condition_ttl,
    coder: coder
end

• MAX_KEY_BYTESIZE is 1kB - not applicable for RackAttack
• error_handler - Good point 👍
• namespace - RackAttack has it's own name space
• compress, compress_threshold & coder - not applicable for RackAttack
• race_condition_ttl - doesn't apply if raw is true, and RackAttack use it with raw: true

There is more:

• instrumentation - again, RackAttack has it's own
• failsafe blocks - RackAttack got it too

The error_handler: this could be a good candidate as RackAttack feature.

With this said, let's assume RedisCacheStore, while being a usefull tool in general, has little value for RackAttack and move to performance area.

Again, exploring RedisCacheStore.

This is the increment of current RedisCacheStoreProxy

if options[:expires_in] && !read(name)
  write(name, amount, options)
  amount
else
  super
end

What the super does? (significant part)

redis.with do |c|
  c.incrby(key, amount).tap do
    write_key_expiry(c, key, options)
  end
end

Finally the write_key_expiry

if options[:expires_in] && client.ttl(key).negative?
  client.expire key, options[:expires_in].to_i
end

Notice how the execution path goes read - incrby - ttl during the entire lifetime of this key, except the first create request.
But read and ttl are not necessary.

Benchmarks

This is Rack::Attack::StoreProxy::ActiveSupportRedisStoreProxy as proxy vs Rack::Attack::Adapters::RedisAdapter as adapter. See the benchmark script here.

Warming up --------------------------------------
               proxy   583.000  i/100ms
             adapter     3.824k i/100ms
Calculating -------------------------------------
               proxy      8.764k (±11.0%) i/s -     86.867k in  10.038466s
             adapter     39.742k (± 9.0%) i/s -    393.872k in  10.000429s

Comparison:
             adapter:    39742.2 i/s
               proxy:     8764.4 i/s - 4.53x  (± 0.00) slower

Results varies from

Comparison:
             adapter:    39474.1 i/s
               proxy:     9565.6 i/s - 4.13x  (± 0.00) slower

to

Comparison:
             adapter:    43716.3 i/s
               proxy:     9341.1 i/s - 4.68x  (± 0.00) slower

ActiveSupportRedisStoreProxy implementation is at least 4 times slower than RedisAdapter.

In my understanding RedisCacheStore is not suitable for use in RackAttack. And all other wrappers on cache server clients for that matter. They designed as general purpose tools. Having another layer over them will only increase code execution paths.
But I'd like to hear other opinions.

Memcached

Things are much simpler here.

dc = Dalli::Client.new
10.times.map { dc.incr('key', 1, 3, 1).tap { sleep(1) } }
# => [1, 2, 3, 1, 2, 3, 1, 2, 3, 1]

Turns out Memcached completely ignores the TTL during subsequent incr calls. Works smoothly with this new approach.

The Stucture

lib/rack/attack
├── adapters
│   ├── active_support_memory_store_adapter.rb
│   ├── base.rb
│   ├── dalli_client_adapter.rb
│   ├── redis_adapter.rb
│   └── redis_store_adapter.rb
├── store_handlers
│   ├── active_support_mem_cache_store_handler.rb
│   ├── active_support_memory_store_handler.rb
│   ├── active_support_redis_cache_store_handler.rb
│   ├── active_support_redis_store_handler.rb
│   ├── dalli_client_handler.rb
│   ├── redis_handler.rb
│   └── redis_store_handler.rb
├── store_handlers.rb

All adapters implement read, write, increment, delete methods. Store handlers decide which adapter to initiate.

There is one special case. ActiveSupport::Cache::MemoryStore#increment doesn't actually increment existing cache value.
It reads and writes new cache entry with incremented value and given TTL. Which doesn't work with this approach.
The ActiveSupportMemoryStoreAdapter implementation assumes that no one use MemoryStore in production.

[grzuy]

Hi @melopilosyan ,

Thank you for taking the time to work on this PR and the thorough description.

My impression is given the maturity of a gem like rack-attack is unfortunately, difficult to consider a rework/refactor of this magnitude to address something like #480, which is technically not a bug or issue but more like a (possibly for some non-intuitive) design decision from the early rack-attack days.

If you feel strongly about this and want to still address it and push to improve it, feel free to re-open, considering that a smaller PR only scoped to the smallest possible changes addressing the issue are more probable to have a change to be merged than mixed with big refactors.

Thanks again!

[melopilosyan]

Totally valid point, @grzuy. Wouldn't accept a PR on this scale either.
In fact, I should have closed it right after submission. 🙂

The refactoring was needed to support the fix. Well, maybe not as elaborate, but still.
It was an interesting experiment. Wanted to see how far I can go.

Anyway, thanks for the feedback!