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

Already on GitHub? Sign in to your account

Replace deprecated `memcache-client` gem with `dalli` in ActiveSupport::Cache::MemCacheStore #6903

Merged
merged 2 commits into from Aug 18, 2012
Jump to file or symbol
Failed to load files and symbols.
+37 −42
Split
View
@@ -40,7 +40,7 @@ group :doc do
end
# AS
-gem 'memcache-client', '>= 1.8.5'
+gem 'dalli'
# Add your own local bundler stuff
local_gemfile = File.dirname(__FILE__) + "/.Gemfile"
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* Replace deprecated `memcache-client` gem with `dalli` in ActiveSupport::Cache::MemCacheStore
+
+ *Guillermo Iguaran*
+
* Add default values to all `ActiveSupport::NumberHelper` methods, to avoid
errors with empty locales or missing values.
@@ -1,7 +1,7 @@
begin
- require 'memcache'
+ require 'dalli'
rescue LoadError => e
- $stderr.puts "You don't have memcache-client installed in your application. Please add it to your Gemfile and run bundle install"
+ $stderr.puts "You don't have dalli installed in your application. Please add it to your Gemfile and run bundle install"
raise e
end
@@ -22,21 +22,13 @@ module Cache
# MemCacheStore implements the Strategy::LocalCache strategy which implements
# an in-memory cache inside of a block.
class MemCacheStore < Store
- module Response # :nodoc:
- STORED = "STORED\r\n"
- NOT_STORED = "NOT_STORED\r\n"
- EXISTS = "EXISTS\r\n"
- NOT_FOUND = "NOT_FOUND\r\n"
- DELETED = "DELETED\r\n"
- end
@spastorino

spastorino Jun 29, 2012

Owner

How is this handled now?

@guilleiguaran

guilleiguaran Jun 29, 2012

Owner

Dalli::Client methods return truish/falsy values instead of string responses.

@guilleiguaran

guilleiguaran Jun 29, 2012

Owner

Dalli's increment/decrement are returning integer values or nil (You don't have to use response == Response::NOT_FOUND ? nil : response.to_i anymore)

-
ESCAPE_KEY_CHARS = /[\x00-\x20%\x7F-\xFF]/n
def self.build_mem_cache(*addresses)
addresses = addresses.flatten
options = addresses.extract_options!
addresses = ["localhost:11211"] if addresses.empty?
- MemCache.new(addresses, options)
+ Dalli::Client.new(addresses, options)
end
# Creates a new MemCacheStore object, with the given memcached server
@@ -90,11 +82,11 @@ def read_multi(*names)
# to zero.
def increment(name, amount = 1, options = nil) # :nodoc:
options = merged_options(options)
- response = instrument(:increment, name, :amount => amount) do
+ instrument(:increment, name, :amount => amount) do
@data.incr(escape_key(namespaced_key(name, options)), amount)
end
- response == Response::NOT_FOUND ? nil : response.to_i
- rescue MemCache::MemCacheError
+ rescue Dalli::DalliError
+ logger.error("DalliError (#{e}): #{e.message}") if logger
nil
end
@@ -104,18 +96,21 @@ def increment(name, amount = 1, options = nil) # :nodoc:
# to zero.
def decrement(name, amount = 1, options = nil) # :nodoc:
options = merged_options(options)
- response = instrument(:decrement, name, :amount => amount) do
+ instrument(:decrement, name, :amount => amount) do
@data.decr(escape_key(namespaced_key(name, options)), amount)
end
- response == Response::NOT_FOUND ? nil : response.to_i
- rescue MemCache::MemCacheError
+ rescue Dalli::DalliError
+ logger.error("DalliError (#{e}): #{e.message}") if logger
nil
end
# Clear the entire cache on all memcached servers. This method should
# be used with care when shared cache is being used.
def clear(options = nil)
@data.flush_all
+ rescue Dalli::DalliError => e
+ logger.error("DalliError (#{e}): #{e.message}") if logger
+ nil
end
# Get the statistics from the memcached servers.
@@ -126,9 +121,9 @@ def stats
protected
# Read an entry from the cache.
def read_entry(key, options) # :nodoc:
- deserialize_entry(@data.get(escape_key(key), true))
- rescue MemCache::MemCacheError => e
- logger.error("MemCacheError (#{e}): #{e.message}") if logger
+ deserialize_entry(@data.get(escape_key(key), options))
+ rescue Dalli::DalliError => e
+ logger.error("DalliError (#{e}): #{e.message}") if logger
nil
end
@@ -137,23 +132,17 @@ 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
@spastorino

spastorino Jun 29, 2012

Owner

Why did you remove this lines?

@guilleiguaran

guilleiguaran Jun 29, 2012

Owner

This code isn't present in Dalli cache store, I'm pretty sure isn't needed if we are using Dalli.

/cc @mperham

@NZKoz

NZKoz Jul 3, 2012

Member

That code's pretty peculiar too, if the user's giving us an expires_in, why would we increment it? If they want to avoid the race conditions they can increment their ttls themselves surely?

@spastorino

spastorino Jul 3, 2012

Owner

@NZKoz yep agree but I'd leave that for another PR :)

@mperham

mperham Jul 3, 2012

Contributor

I think that code can be removed. I was trying to support the race_condition_ttl option but I don't think it's worth it. As koz notes, if the user is savvy enough to want stale reads, they understand that Dalli's expires_in must be incremented to keep the value around for that stale period.

@spastorino

spastorino Jul 3, 2012

Owner

@mperham agree, anyway isn't that code now in dalli?

@mperham

mperham Jul 3, 2012

Contributor

@spastorino It was removed in 2.0. I made an effort to remove edge case code like this that increased the complexity of the code with little benefit.

- response = @data.send(method, escape_key(key), value, expires_in, options[:raw])
- response == Response::STORED
- rescue MemCache::MemCacheError => e
- logger.error("MemCacheError (#{e}): #{e.message}") if logger
+ @data.send(method, escape_key(key), value, expires_in, options)
+ rescue Dalli::DalliError => e
+ logger.error("DalliError (#{e}): #{e.message}") if logger
false
end
# Delete an entry from the cache.
def delete_entry(key, options) # :nodoc:
- response = @data.delete(escape_key(key))
- response == Response::DELETED
- rescue MemCache::MemCacheError => e
- logger.error("MemCacheError (#{e}): #{e.message}") if logger
+ @data.delete(escape_key(key))
+ rescue Dalli::DalliError => e
+ logger.error("DalliError (#{e}): #{e.message}") if logger
false
end
@@ -22,11 +22,11 @@
require 'active_support'
def uses_memcached(test_name)
- require 'memcache'
+ require 'dalli'
begin
- MemCache.new('localhost:11211').stats
+ Dalli::Client.new('localhost:11211').stats
yield
- rescue MemCache::MemCacheError
+ rescue Dalli::DalliError
$stderr.puts "Skipping #{test_name} tests. Start memcached and try again."
end
end
@@ -83,34 +83,34 @@ def test_file_fragment_cache_store
end
def test_mem_cache_fragment_cache_store
- MemCache.expects(:new).with(%w[localhost], {})
+ Dalli::Client.expects(:new).with(%w[localhost], {})
store = ActiveSupport::Cache.lookup_store :mem_cache_store, "localhost"
assert_kind_of(ActiveSupport::Cache::MemCacheStore, store)
end
def test_mem_cache_fragment_cache_store_with_given_mem_cache
- mem_cache = MemCache.new
- MemCache.expects(:new).never
+ mem_cache = Dalli::Client.new
+ Dalli::Client.expects(:new).never
store = ActiveSupport::Cache.lookup_store :mem_cache_store, mem_cache
assert_kind_of(ActiveSupport::Cache::MemCacheStore, store)
end
def test_mem_cache_fragment_cache_store_with_given_mem_cache_like_object
- MemCache.expects(:new).never
+ Dalli::Client.expects(:new).never
memcache = Object.new
def memcache.get() true end
store = ActiveSupport::Cache.lookup_store :mem_cache_store, memcache
assert_kind_of(ActiveSupport::Cache::MemCacheStore, store)
end
def test_mem_cache_fragment_cache_store_with_multiple_servers
- MemCache.expects(:new).with(%w[localhost 192.168.1.1], {})
+ Dalli::Client.expects(:new).with(%w[localhost 192.168.1.1], {})
store = ActiveSupport::Cache.lookup_store :mem_cache_store, "localhost", '192.168.1.1'
assert_kind_of(ActiveSupport::Cache::MemCacheStore, store)
end
def test_mem_cache_fragment_cache_store_with_options
- MemCache.expects(:new).with(%w[localhost 192.168.1.1], { :timeout => 10 })
+ Dalli::Client.expects(:new).with(%w[localhost 192.168.1.1], { :timeout => 10 })
store = ActiveSupport::Cache.lookup_store :mem_cache_store, "localhost", '192.168.1.1', :namespace => 'foo', :timeout => 10
assert_kind_of(ActiveSupport::Cache::MemCacheStore, store)
assert_equal 'foo', store.options[:namespace]
@@ -447,6 +447,7 @@ def test_increment
assert_equal 2, @cache.read('foo').to_i
assert_equal 3, @cache.increment('foo')
assert_equal 3, @cache.read('foo').to_i
+ assert_nil @cache.increment('bar')
end
def test_decrement
@@ -456,6 +457,7 @@ def test_decrement
assert_equal 2, @cache.read('foo').to_i
assert_equal 1, @cache.decrement('foo')
assert_equal 1, @cache.read('foo').to_i
+ assert_nil @cache.decrement('bar')
end
end