Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

Merged
merged 1 commit into from

6 participants

Mike Evans Joe Winter Guillermo Iguaran Xavier Noria chris a Anna Carey
Mike Evans

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.

Mike Evans

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

Joe Winter

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.

Guillermo Iguaran

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

Mike Evans

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.

Xavier Noria
Owner

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.

Xavier Noria fxn merged commit 168df24 into from
chris a

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

mperham/dalli#277

Anna Carey

@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
Commits on Oct 11, 2012
  1. Mike Evans

    Padding expires_in by 5 minutes on dalli key write to facilitate :rac…

    mje113 authored
    …e_condition_ttl working correctly.
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 0 deletions.
  1. +4 −0 activesupport/lib/active_support/cache/mem_cache_store.rb
4 activesupport/lib/active_support/cache/mem_cache_store.rb
View
@@ -132,6 +132,10 @@ def write_entry(key, entry, options) # :nodoc:
method = options && options[:unless_exist] ? :add : :set
value = options[:raw] ? entry.value.to_s : entry
expires_in = options[:expires_in].to_i
+ if expires_in > 0 && !options[:raw]
+ # Set the memcache expire a few minutes in the future to support race condition ttls on read
+ expires_in += 5.minutes
+ end
@data.send(method, escape_key(key), value, expires_in, options)
rescue Dalli::DalliError => e
logger.error("DalliError (#{e}): #{e.message}") if logger
Something went wrong with that request. Please try again.