Merge pull request #40420 from sambostock/use-dalli-fallback

Use `$MEMCACHE_SERVERS` as `MemCacheStore` fallback

activesupport/CHANGELOG.md

@@ -1,3 +1,19 @@
+*   `ActiveSupport::Cache::MemCacheStore` now checks `ENV["MEMCACHE_SERVERS"]` before falling back to `"localhost:11211"` if configured without any addresses.
+
+    ```ruby
+    config.cache_store = :mem_cache_store
+
+    # is now equivalent to
+
+    config.cache_store = :mem_cache_store, ENV["MEMCACHE_SERVERS"] || "localhost:11211"
+
+    # instead of
+
+    config.cache_store = :mem_cache_store, "localhost:11211" # ignores ENV["MEMCACHE_SERVERS"]
+    ```
+
+    *Sam Bostock*
+
 *   `ActiveSupport::Subscriber#attach_to` now accepts an `inherit_all:` argument. When set to true,
     it allows a subscriber to receive events for methods defined in the subscriber's ancestor class(es).
 

activesupport/lib/active_support/cache/mem_cache_store.rb

@@ -53,16 +53,18 @@ def self.supports_cache_versioning?
       ESCAPE_KEY_CHARS = /[\x00-\x20%\x7F-\xFF]/n
 
       # Creates a new Dalli::Client instance with specified addresses and options.
-      # By default address is equal localhost:11211.
+      # If no addresses are provided, we give nil to Dalli::Client, so it uses its fallbacks:
+      # - ENV["MEMCACHE_SERVERS"] (if defined)
+      # - "127.0.0.1:11211"        (otherwise)
       #
       #   ActiveSupport::Cache::MemCacheStore.build_mem_cache
-      #     # => #<Dalli::Client:0x007f98a47d2028 @servers=["localhost:11211"], @options={}, @ring=nil>
+      #     # => #<Dalli::Client:0x007f98a47d2028 @servers=["127.0.0.1:11211"], @options={}, @ring=nil>
       #   ActiveSupport::Cache::MemCacheStore.build_mem_cache('localhost:10290')
       #     # => #<Dalli::Client:0x007f98a47b3a60 @servers=["localhost:10290"], @options={}, @ring=nil>
       def self.build_mem_cache(*addresses) # :nodoc:
         addresses = addresses.flatten
         options = addresses.extract_options!
-        addresses = ["localhost:11211"] if addresses.empty?
+        addresses = nil if addresses.empty?
         pool_options = retrieve_pool_options(options)
 
         if pool_options.empty?
@@ -79,8 +81,8 @@ def self.build_mem_cache(*addresses) # :nodoc:
       #
       #   ActiveSupport::Cache::MemCacheStore.new("localhost", "server-downstairs.localnetwork:8229")
       #
-      # If no addresses are specified, then MemCacheStore will connect to
-      # localhost port 11211 (the default memcached port).
+      # If no addresses are provided, but ENV['MEMCACHE_SERVERS'] is defined, it will be used instead. Otherwise,
+      # MemCacheStore will connect to localhost:11211 (the default memcached port).
       def initialize(*addresses)
         addresses = addresses.flatten
         options = addresses.extract_options!

activesupport/test/cache/stores/mem_cache_store_test.rb

@@ -45,7 +45,6 @@ def setup
     @namespace = "test-#{SecureRandom.hex}"
     @cache = lookup_store(expires_in: 60)
     @peek = lookup_store
-    @data = @cache.instance_variable_get(:@data)
     @cache.silence!
     @cache.logger = ActiveSupport::Logger.new(File::NULL)
   end
@@ -64,7 +63,6 @@ def setup
   # Overrides test from LocalCacheBehavior in order to stub out the cache clear
   # and replace it with a delete.
   def test_clear_also_clears_local_cache
-    client = @cache.instance_variable_get(:@data)
     key = "#{@namespace}:foo"
     client.stub(:flush_all, -> { client.delete(key) }) do
       super
@@ -102,14 +100,14 @@ def test_local_cache_raw_values
 
   def test_increment_expires_in
     cache = lookup_store(raw: true, namespace: nil)
-    assert_called_with cache.instance_variable_get(:@data), :incr, [ "foo", 1, 60 ] do
+    assert_called_with client(cache), :incr, [ "foo", 1, 60 ] do
       cache.increment("foo", 1, expires_in: 60)
     end
   end
 
   def test_decrement_expires_in
     cache = lookup_store(raw: true, namespace: nil)
-    assert_called_with cache.instance_variable_get(:@data), :decr, [ "foo", 1, 60 ] do
+    assert_called_with client(cache), :decr, [ "foo", 1, 60 ] do
       cache.decrement("foo", 1, expires_in: 60)
     end
   end
@@ -164,18 +162,47 @@ def test_no_multiple_compress
 
   def test_unless_exist_expires_when_configured
     cache = ActiveSupport::Cache.lookup_store(:mem_cache_store)
-    assert_called_with cache.instance_variable_get(:@data), :add, [ "foo", ActiveSupport::Cache::Entry, 1, Hash ] do
+    assert_called_with client(cache), :add, [ "foo", ActiveSupport::Cache::Entry, 1, Hash ] do
       cache.write("foo", "bar", expires_in: 1, unless_exist: true)
     end
   end
 
+  def test_uses_provided_dalli_client_if_present
+    cache = ActiveSupport::Cache.lookup_store(:mem_cache_store, Dalli::Client.new("custom_host"))
+
+    assert_equal ["custom_host"], servers(cache)
+  end
+
+  def test_forwards_string_addresses_if_present
+    expected_addresses = ["first", "second"]
+    cache = ActiveSupport::Cache.lookup_store(:mem_cache_store, expected_addresses)
+
+    assert_equal expected_addresses, servers(cache)
+  end
+
+  def test_falls_back_to_localhost_if_no_address_provided_and_memcache_servers_undefined
+    with_memcache_servers_environment_variable(nil) do
+      cache = ActiveSupport::Cache.lookup_store(:mem_cache_store)
+
+      assert_equal ["127.0.0.1:11211"], servers(cache)
+    end
+  end
+
+  def test_falls_back_to_localhost_if_no_address_provided_and_memcache_servers_defined
+    with_memcache_servers_environment_variable("custom_host") do
+      cache = ActiveSupport::Cache.lookup_store(:mem_cache_store)
+
+      assert_equal ["custom_host"], servers(cache)
+    end
+  end
+
   private
     def random_string(length)
       (0...length).map { (65 + rand(26)).chr }.join
     end
 
     def store
-      [:mem_cache_store, ENV["MEMCACHE_SERVERS"] || "localhost:11211"]
+      [:mem_cache_store]
     end
 
     def emulating_latency
@@ -197,4 +224,24 @@ def emulating_unavailability
       Dalli.send(:remove_const, :Server)
       Dalli.const_set(:Server, old_server)
     end
+
+    def servers(cache = @cache)
+      client(cache).instance_variable_get(:@servers)
+    end
+
+    def client(cache = @cache)
+      cache.instance_variable_get(:@data)
+    end
+
+    def with_memcache_servers_environment_variable(value)
+      original_value = ENV["MEMCACHE_SERVERS"]
+      ENV["MEMCACHE_SERVERS"] = value
+      yield
+    ensure
+      if original_value.nil?
+        ENV.delete("MEMCACHE_SERVERS")
+      else
+        ENV["MEMCACHE_SERVERS"] = original_value
+      end
+    end
 end

guides/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Updated `ActiveSupport::Cache::MemCacheStore` docs to reflect support for `$MEMCACHE_SERVERS`.
+
+    *Sam Bostock*
+
 *   Use Bookstore as a unified use-case for all examples in Active Record Query Interface Guide.
 
     *Ashley Engelund*, *Vipul A M*

guides/source/caching_with_rails.md

@@ -453,17 +453,22 @@ no explicit `config.cache_store` is supplied.
 
 This cache store uses Danga's `memcached` server to provide a centralized cache for your application. Rails uses the bundled `dalli` gem by default. This is currently the most popular cache store for production websites. It can be used to provide a single, shared cache cluster with very high performance and redundancy.
 
-When initializing the cache, you need to specify the addresses for all
-memcached servers in your cluster. If none are specified, it will assume
-memcached is running on localhost on the default port, but this is not an ideal
-setup for larger sites.
-
-The `write` and `fetch` methods on this cache accept two additional options that take advantage of features specific to memcached. You can specify `:raw` to send a value directly to the server with no serialization. The value must be a string or number. You can use memcached direct operations like `increment` and `decrement` only on raw values. You can also specify `:unless_exist` if you don't want memcached to overwrite an existing entry.
+When initializing the cache, you should specify the addresses for all memcached servers in your cluster, or ensure the `MEMCACHE_SERVERS` environment variable has been set appropriately.
 
 ```ruby
 config.cache_store = :mem_cache_store, "cache-1.example.com", "cache-2.example.com"
 ```
 
+If neither are specified, it will assume memcached is running on localhost on the default port (`127.0.0.1:11211`), but this is not an ideal setup for larger sites.
+
+```ruby
+config.cache_store = :mem_cache_store # Will fallback to $MEMCACHE_SERVERS, then 127.0.0.1:11211
+```
+
+See the [`Dalli::Client` documentation](https://www.rubydoc.info/github/mperham/dalli/Dalli%2FClient:initialize) for supported address types.
+
+The `write` and `fetch` methods on this cache accept two additional options that take advantage of features specific to memcached. You can specify `:raw` to send a value directly to the server with no serialization. The value must be a string or number. You can use memcached direct operations like `increment` and `decrement` only on raw values. You can also specify `:unless_exist` if you don't want memcached to overwrite an existing entry.
+
 ### ActiveSupport::Cache::RedisCacheStore
 
 The Redis cache store takes advantage of Redis support for automatic eviction