Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in #shrink_if_needed. #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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?

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