search

sorentwo / readthis

:newspaper: Pooled active support compliant caching with redis
MIT License
504 stars 40 forks source link

Handle decimal expiration more gracefully, i.e. when SETEX expires_in.to_i is SETEX 0 #41

Closed bf4 closed 8 years ago

bf4 commented 8 years ago

Example from the ActiveModelSerializers caching benchmark:

Setting expire_in: 0.1 works fine using the ActiveSupport memory store

class CachingPostSerializer < ActiveModel::Serializer
  cache key: 'post', expires_in: 0.1, skip_digest: true

but readthis casts it to to_i #=> 0 which causes the below failure and stacktrace. (Perhaps Float(0.1).ceil would be preferable?)

/Users/benjamin/.rvm/gems/ruby-2.2.3/gems/readthis-1.2.1/lib/readthis.rb:26: warning: instance variable @fault_tolerant not initialized
I, [2016-05-30T23:57:14.753062 #37104]  INFO -- : Rendered CachingPostSerializer with ActiveModelSerializers::Adapter::Json (6.66ms)
I, [2016-05-30T23:57:14.753165 #37104]  INFO -- : Completed 500 Internal Server Error in 10ms
F, [2016-05-30T23:57:14.754827 #37104] FATAL -- :
Redis::CommandError (ERR invalid expire time in SETEX):
  redis (3.3.0) lib/redis/client.rb:121:in `call'
  redis (3.3.0) lib/redis.rb:769:in `block in setex'
  redis (3.3.0) lib/redis.rb:58:in `block in synchronize'
  /Users/benjamin/.rvm/rubies/ruby-2.2.3/lib/ruby/2.2.0/monitor.rb:211:in `mon_synchronize'
  redis (3.3.0) lib/redis.rb:58:in `synchronize'
  redis (3.3.0) lib/redis.rb:768:in `setex'
  readthis (1.2.1) lib/readthis/cache.rb:318:in `write_entity'
  readthis (1.2.1) lib/readthis/cache.rb:86:in `block in write'
  connection_pool (2.2.0) lib/connection_pool.rb:64:in `block (2 levels) in with'
  connection_pool (2.2.0) lib/connection_pool.rb:63:in `handle_interrupt'
  connection_pool (2.2.0) lib/connection_pool.rb:63:in `block in with'
  connection_pool (2.2.0) lib/connection_pool.rb:60:in `handle_interrupt'
  connection_pool (2.2.0) lib/connection_pool.rb:60:in `with'
  readthis (1.2.1) lib/readthis/cache.rb:346:in `block in invoke'
  readthis (1.2.1) lib/readthis/cache.rb:338:in `block in instrument'
  activesupport (4.2.6) lib/active_support/notifications.rb:166:in `instrument'
  readthis (1.2.1) lib/readthis/cache.rb:338:in `instrument'
  readthis (1.2.1) lib/readthis/cache.rb:345:in `invoke'
  readthis (1.2.1) lib/readthis/cache.rb:85:in `write'
  readthis (1.2.1) lib/readthis/cache.rb:146:in `fetch'
  lib/active_model/serializer/caching.rb:220:in `cache_check'
sorentwo commented 8 years ago

Interesting, that's the first I've seen using a float for the expiration period. I've thought about this a little bit, and whether this would be a breaking change. Considering that it has been impossible to use a float when setting the expiration this seems backward compatible, and would increase parity with other AS caches.