Fix bug in #shrink_if_needed.

swoop-inc / fast_cache

Very fast in-process cache with least-recently used (LRU) and time-to-live (TTL) expiration semantics.

MIT License

11 stars 5 forks source link

Fix bug in #shrink_if_needed. #5

Open joelnordell opened 9 years ago

joelnordell commented 9 years ago

shifted entry was not being removed from @expires_at
replace if with while, in case somehow the @data length is more than 1 greater than @max_size

ssimeonov commented 9 years ago

Can you provide a test that proves this is a bug?

joelnordell commented 8 years ago

Just added a test. Unfortunately, as the bug is purely internal to the FastCache::Cache class, the test has to resort to using instance_variable_get.

My other pull request, https://github.com/swoop-inc/fast_cache/pull/3, actually is what exposed the bug -- one of its tests fails without this fix.

https://github.com/ErisExchange/fast_cache/blob/eris-cleanup_procs/spec/lib/fast_cache/cache_spec.rb#L139

Failures:

  1) FastCache::Cache non-empty cache cleanup block is called when an item is removed when full
     Failure/Error: subject[:d] = 4
     NoMethodError:
       undefined method `value' for nil:NilClass
     # ./lib/fast_cache/cache.rb:217:in `shrink_if_needed'
     # ./lib/fast_cache/cache.rb:210:in `store_entry'
     # ./lib/fast_cache/cache.rb:200:in `store'
     # ./lib/fast_cache/cache.rb:81:in `[]='
     # ./spec/lib/fast_cache/cache_spec.rb:140:in `block (4 levels) in <top (required)>

joelnordell commented 8 years ago

Higher level question: would you prefer if I consolidated all three of my pull requests into one? I had originally done them all together in one branch, but split them up because they are separate concerns. However, there is a clear dependency order: