MemCacheStore's move to dalli as a backend broke :race_condition_ttl support #7913

Merged
merged 1 commit into from Oct 12, 2012

6 participants

@mje113

Sending dalli an expires_in integer that matches the Entry instance's expires_at will invalidate the cached value in Memcache at the same time the Entry is expired. This breaks the ability for Cache#fetch to determine if the :race_condition_ttl has occurred since the value will never make it back from Memcached.

@mje113

Sorry for no test coverage, but this is pretty hard to test.

@jwinter

You can see where the additional 5.minutes used to be added to the expires_in sent to memcached here: https://github.com/rails/rails/blob/a02b40a3d28c4b262dd49a8a6166c0275dfd9964/activesupport/lib/active_support/cache/mem_cache_store.rb#L142
Without that padding, there will never be a window where both your entry is expired (via its expired_at) and memcached still has its value, so you'll never be able to serve the cached value to the other readers.

@guilleiguaran
Ruby on Rails member

The removal of race_condition_ttl was discussed here: http://github.com/rails/rails/pull/6903/files#r1078311

@mje113

What if someone is relying on race_condition_ttl to work correctly when upgrading to 4.0? Perhaps the removal of the functionality should be pointed out--or if it's agreed that it's up to the user to implement their own race_condition_ttl functionality, shouldn't it also be yanked from ActiveSupport::Cache#fetch?

Finally, if ActiveSupport::Cache isn't keeping its own tabs on the expires_in, and modifying that value to avoid multiple concurrent fetches on a key miss, why wrap the cached value in an Entity instance at all? The overhead with the extra marshaling and compressing (Dalli also will marshal and compress the value sent to it) just doesn't seem worth it.

@fxn
Ruby on Rails member

race_condition_ttl is provided by the cache store, see https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb#L23, and we recently had a thread about this feature and the ability to distinguish between an unset key and a cached nil value. Those two are the ones that justify having ActiveSupport::Cache::Entry objects serialized.

The outcome of the discussion was that we want to support them, and @bdurand provided a patch that made serialized entry objects much lighter. If someone wants to store raw values in a different cache store implementation at the cost of losing those features, he can do it in a plugin.

So, we need to bring those 5 minutes back.

@fxn fxn merged commit 168df24 into rails:master Oct 12, 2012
@whatbird

FWIW: there is currently a pull request on Dalli to support :race_condition_ttl

petergoldstein/dalli#277

@annaswims

@mje113 Could you explain why adding 5 minutes to the expires_in is the appropriate thing to do here? I was using dali 2.6.4 and set the expires_in to 30.days. Memcached interprets anything over 30 days as a timestamp and adding 5 minutes made it greater than 30 days, therefore my cache didn't work.

The issue resolved in this commit was bringing back the functionality that supported the :race_condition_ttl option that had been accidentally (I believe?) removed. I agree that that is problematic with a 30 day cache, but I think that's a question rails core devs should answer.

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