Skip to content

Conversation

@joelnordell
Copy link
Contributor

  • 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
Copy link
Contributor

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

@joelnordell
Copy link
Contributor Author

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, #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
Copy link
Contributor Author

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:

  1. RSpec fix
  2. This fix
  3. Cleanup blocks (this was my ultimate purpose for these)

@ssimeonov
Copy link
Contributor

Separate PRs are best: you made the right decision. I merged RSpec PR. You can pick up the changes.

@joelnordell
Copy link
Contributor Author

great, I'll rebase this one now so it will have a passing travis status

- 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
@joelnordell joelnordell force-pushed the eris-fix_bug_in_shrink_if_needed branch from f7983b1 to 0486328 Compare November 17, 2015 21:51
@joelnordell
Copy link
Contributor Author

after this one is merged, I can rebase #3 and it should be ready to merge too.

@joelnordell
Copy link
Contributor Author

@ssimeonov what's the status of this -- are you going to merge it?

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants