SamSaffron / lru_redux

An efficient optionally thread safe LRU Cache
MIT License
285 stars 20 forks source link

Unbounded cache bug, dead locking each and minor improvements. #4

Closed Seberius closed 9 years ago

Seberius commented 9 years ago

Hello, I was working on a fork of this library as project to learn about Ruby (lirs-cache fork). I ran into to a couple bugs with the legacy (pre 1.9) implementation that I thought I should report and pull request a fix for.

Unbounded Cache (legacy)

Consider:

cache = LruRedux::Cache.new(3)

cache[:a] = 1
cache[:b] = 2
cache[:c] = 3
cache.delete(:a)

cache.count
# => 2

cache[:d] = 4
cache[:e] = 5

cache.count
# => 4

cache.to_a
# => [[:e, 5], [:d, 4], [:c, 3], [:b, 2]]

The .delete method currently does not change the head or tail variables if the node is located there. If the node at the tail is then deleted, the .pop_tail method will try removing that node again when max_size is reached. As a result the cache will grow 1 past (or more, if the node linked to tail is deleted) max_size each time this happens. The pull request also has an expanded test to check for this.

Dead locking .each (legacy)

When I try to run the tests they will hang at test_recursion. This pull request changes .each to the cache19 implementation, namely that a new array is created with .to_a and that new array is iterated over. Since .to_a uses the current .each to generate an ordered array, it has been renamed .each_unsafe.

Count changed to use .size (legacy & 1.9)

Count currently uses Hash.count to return the size of the cache. The performance difference is significant.

Other changes

Fabulous run in 0.002524s, 8320.1268 runs/s, 24167.9873 assertions/s.

21 runs, 61 assertions, 0 failures, 0 errors, 0 skips
21 tests
61 assertions, 0 failures, 0 errors

Thanks!

SamSaffron commented 9 years ago

Great set of changes, thanks so much ... will push out a new gem now

Seberius commented 9 years ago

Thanks! Happy to help.