Increment/decrement semantics vary between Rails.cache stores

[Doug Barth]

Hello,

We ran into an issue where the semantics for incrementing or
decrementing a value in the Rails generic cache vary based on which
store you are using. I'd like to work up a patch, but I want to
solicit some opinions on the best way to implement the changes.

We were attempting to implement the delete by namespace pattern that
memcached recommends [1]. In short, you keep a counter in a namespace
key and use the value in your target cache keys. When you want to
delete the namespace, you increment the key, effectively forgetting
the previous versions.

We found that implementing this pattern required custom code for each
store.

The MemoryStore will pass the following test case and is what I assume
is the expected behavior.

def test_increment
@cache.write('foo', 1)
assert_equal 1, @cache.read('foo')
assert_equal 2, @cache.increment('foo')
assert_equal 2, @cache.read('foo')
end

The FileStore will fail the above test because increment always
returns 1, not the new value. Is increment expected to return the new
value?

The MemCacheStore requires the test case to be written completely
differently because memcached's atomic increment and decrement methods
require raw values to be written. The version of memcache-client that
is bundled with activesupport will hang if number is supplied for a
raw write. This is a known bug [2] and is fixed in a number of
alternative memcache-clients [3]. The following test will work around
the issues but is completely different from the memory store version,
defeating the purpose of the facade.

def test_increment
@cache.write('foo', '1', :raw => true)
assert_equal '1', @cache.read('foo', :raw => true)
assert_equal 2, @cache.increment('foo')
assert_equal '2', @cache.read('foo', :raw => true)
end

Finally, the CompressedMemCacheStore completely breaks the increment
and decrement functionality. Upon trying to read an incremented value,
the following exception is thrown.

Zlib::GzipFile::Error: not in gzip format
./test/../lib/active_support/gzip.rb:13:in `initialize'
./test/../lib/active_support/gzip.rb:13:in `new'
./test/../lib/active_support/gzip.rb:13:in `decompress'
./test/../lib/active_support/cache/compressed_mem_cache_store.rb:
6:in `read'
test/caching_test.rb:195:in `test_increment'
/Library/Ruby/Gems/1.8/gems/mocha-0.9.0/lib/mocha/
test_case_adapter.rb:69:in `__send__'
/Library/Ruby/Gems/1.8/gems/mocha-0.9.0/lib/mocha/
test_case_adapter.rb:69:in `run'

What are the expected semantics for the cache store facade? I can work
up a patch with test cases to ensure that the current implementations
fall in line. My own suggestion would be to mimic the memory store
test case, with the exception that :raw => true should be supplied
(and presumably ignored by every impl other than memcache).

def test_increment
@cache.write('foo', 1, :raw => true)
assert_equal 1, @cache.read('foo', :raw => true)
assert_equal 2, @cache.increment('foo')
assert_equal 2, @cache.read('foo', :raw => true)
end

Alternatively, we could create special counter methods (counter_init,
counter_read, counter_write) that one would need to call to interact
with counters in the cache.

Thoughts?

[1] http://www.socialtext.net/memcached/index.cgi?faq#deleting_by_namespace
[2] http://rubyforge.org/tracker/index.php?func=detail&aid=13721&group_id=1513&atid=5921
[3] http://blog.fiveruns.com/2008/6/11/fiveruns-and-seattle-rb-s-memcache-client

[Michael Koziarski]

Doug Barth wrote:
> Hello,
>
> We ran into an issue where the semantics for incrementing or
> decrementing a value in the Rails generic cache vary based on which
> store you are using. I'd like to work up a patch, but I want to
> solicit some opinions on the best way to implement the changes.
>
> We were attempting to implement the delete by namespace pattern that
> memcached recommends [1]. In short, you keep a counter in a namespace
> key and use the value in your target cache keys. When you want to
> delete the namespace, you increment the key, effectively forgetting
> the previous versions.
>
> We found that implementing this pattern required custom code for each
> store.
>
> The MemoryStore will pass the following test case and is what I assume
> is the expected behavior.
>
> def test_increment
> @cache.write('foo', 1)
> assert_equal 1, @cache.read('foo')
> assert_equal 2, @cache.increment('foo')
> assert_equal 2, @cache.read('foo')
> end
>
> The FileStore will fail the above test because increment always
> returns 1, not the new value. Is increment expected to return the new
> value?

That seems like a reasonable assumption, it appears the current return
values are just an artifact of the current implementations.

> def test_increment
> @cache.write('foo', '1', :raw => true)
> assert_equal '1', @cache.read('foo', :raw => true)
> assert_equal 2, @cache.increment('foo')
> assert_equal '2', @cache.read('foo', :raw => true)
> end

That's just nasty... I assume someone's pinged Eric about the bug? I'm
hesitant to take unofficial upstream patches unless memcache-client is
'officially' not getting any more updates. If there's some reason for
the patches not to be merged I'd hate to pull them and have our own fork
to maintain.

> What are the expected semantics for the cache store facade? I can work
> up a patch with test cases to ensure that the current implementations
> fall in line. My own suggestion would be to mimic the memory store
> test case, with the exception that :raw => true should be supplied
> (and presumably ignored by every impl other than memcache).
>
> def test_increment
> @cache.write('foo', 1, :raw => true)
> assert_equal 1, @cache.read('foo', :raw => true)
> assert_equal 2, @cache.increment('foo')
> assert_equal 2, @cache.read('foo', :raw => true)
> end

This seems like a good api, if you can make the changes and send a
patch, we can apply it. Obviously the memcache-client bug is another
issue but hopefully that can be resolved too.

--
Cheers,

Koz

[Doug Barth]

On Oct 4, 6:41 am, Michael Koziarski <mich...@koziarski.com> wrote:
> That's just nasty...  I assume someone's pinged Eric about the bug? I'm
> hesitant to take unofficial upstream patches unless memcache-client is
> 'officially' not getting any more updates.  If there's some reason for
> the patches not to be merged I'd hate to pull them and have our own fork
> to maintain.

Yes, there is a bug that was last touched in September 2007.
http://rubyforge.org/tracker/index.php?func=detail&aid=13721&group_id=1513&atid=5921

From the comments, it appears that Eric is hesitant to apply the
suggested change because he thinks the API for the memcache-client
should not support non-string arguments. I agree that maintaining your
own patches is less than ideal, so I am making the required changes in
the MemCacheStore class instead of modifying the memcache-client.

> >   def test_increment
> >     @cache.write('foo', 1, :raw => true)
> >     assert_equal 1, @cache.read('foo', :raw => true)
> >     assert_equal 2, @cache.increment('foo')
> >     assert_equal 2, @cache.read('foo', :raw => true)
> >   end
>
> This seems like a good api, if you can make the changes and send a
> patch, we can apply it.  Obviously the memcache-client bug is another
> issue but hopefully that can be resolved too.

I had to change the required API slightly. I forgot that memcache-
client will return a string when reading a value with :raw => true.
Therefore, you need to also call to_i on the returned value to get the
cache stores to be consistent. Separate counter manipulating methods
are looking more and more attractive.

def test_increment
@cache.write('foo', 1, :raw => true)

assert_equal 1, @cache.read('foo', :raw => true).to_i

assert_equal 2, @cache.increment('foo')

assert_equal 2, @cache.read('foo', :raw => true).to_i
end

I opened a ticket and uploaded a patch implementing the above API.

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1186-incrementdecrement-semantics-vary-between-railscache-stores

--
Doug Barth