From 944574872f9aa9eaa0c4601f77bb596050a11865 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 20 Oct 2020 21:30:19 -0400 Subject: [PATCH 1/4] Refactor MemCacheStoreTest - Remove unused instance variable - Add client test helper --- .../test/cache/stores/mem_cache_store_test.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb index 23ad36ffb9..b9466c603e 100644 --- a/activesupport/test/cache/stores/mem_cache_store_test.rb +++ b/activesupport/test/cache/stores/mem_cache_store_test.rb @@ -45,7 +45,6 @@ class MemCacheStoreTest < ActiveSupport::TestCase @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 @@ class MemCacheStoreTest < ActiveSupport::TestCase # 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 @@ class MemCacheStoreTest < ActiveSupport::TestCase 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,7 +162,7 @@ class MemCacheStoreTest < ActiveSupport::TestCase 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 @@ -197,4 +195,8 @@ class MemCacheStoreTest < ActiveSupport::TestCase Dalli.send(:remove_const, :Server) Dalli.const_set(:Server, old_server) end + + def client(cache = @cache) + cache.instance_variable_get(:@data) + end end From f50be302de7bc9c981da5e654f800771f987b5cf Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 20 Oct 2020 15:08:48 -0400 Subject: [PATCH 2/4] Add tests for :mem_cache_store initialization This documents existing behavior ahead of changes. --- .../test/cache/stores/mem_cache_store_test.rb | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb index b9466c603e..b2844ff395 100644 --- a/activesupport/test/cache/stores/mem_cache_store_test.rb +++ b/activesupport/test/cache/stores/mem_cache_store_test.rb @@ -167,6 +167,25 @@ class MemCacheStoreTest < ActiveSupport::TestCase 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 + cache = ActiveSupport::Cache.lookup_store(:mem_cache_store) + + assert_equal ["localhost:11211"], servers(cache) + end + private def random_string(length) (0...length).map { (65 + rand(26)).chr }.join @@ -196,6 +215,10 @@ class MemCacheStoreTest < ActiveSupport::TestCase 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 From e79364610ccaa45d729fb7d9b30306242c341f51 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 20 Oct 2020 19:10:26 -0400 Subject: [PATCH 3/4] Fallback to $MEMCACHE_SERVERS if no servers given By default, Dalli has two fallbacks if no server addresses are given: - $MEMCACHE_SERVERS - "127.0.0.1:11211" However, MemCacheStore does its own check for addresses, and falls back to "localhost:11211" if none are present. This can lead to bugs in migrations from the deprecated :dalli_store (provided by the Dalli) to :mem_cache_store: ```diff -config.cache_store = :dalli_store # could be implicitly relying on $MEMCACHE_SERVERS +config.cache_store = :mem_cache_store # ignores $MEMCACHE_SERVERS ``` By removing our own fallback and simply passing `nil` to Dalli::Client, we get its fallback logic for free. Tests are added so we can detect if this ever changes. --- activesupport/CHANGELOG.md | 16 +++++++++++ .../active_support/cache/mem_cache_store.rb | 12 ++++---- .../test/cache/stores/mem_cache_store_test.rb | 28 +++++++++++++++++-- guides/CHANGELOG.md | 4 +++ guides/source/caching_with_rails.md | 17 +++++++---- 5 files changed, 63 insertions(+), 14 deletions(-) diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 3827ef4194..956c4a74b6 100644 --- a/activesupport/CHANGELOG.md +++ b/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). diff --git a/activesupport/lib/active_support/cache/mem_cache_store.rb b/activesupport/lib/active_support/cache/mem_cache_store.rb index c3ae85a2a0..76176abf68 100644 --- a/activesupport/lib/active_support/cache/mem_cache_store.rb +++ b/activesupport/lib/active_support/cache/mem_cache_store.rb @@ -53,16 +53,18 @@ module ActiveSupport 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 - # # => # + # # => # # ActiveSupport::Cache::MemCacheStore.build_mem_cache('localhost:10290') # # => # 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 @@ module ActiveSupport # # 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! diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb index b2844ff395..fe6ca62458 100644 --- a/activesupport/test/cache/stores/mem_cache_store_test.rb +++ b/activesupport/test/cache/stores/mem_cache_store_test.rb @@ -180,10 +180,20 @@ class MemCacheStoreTest < ActiveSupport::TestCase assert_equal expected_addresses, servers(cache) end - def test_falls_back_to_localhost_if_no_address_provided - cache = ActiveSupport::Cache.lookup_store(:mem_cache_store) + 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 ["localhost:11211"], servers(cache) + 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 @@ -222,4 +232,16 @@ class MemCacheStoreTest < ActiveSupport::TestCase 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 diff --git a/guides/CHANGELOG.md b/guides/CHANGELOG.md index d5e509aef5..9b4feed7a8 100644 --- a/guides/CHANGELOG.md +++ b/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* diff --git a/guides/source/caching_with_rails.md b/guides/source/caching_with_rails.md index a3b09e189a..5582ae3b0f 100644 --- a/guides/source/caching_with_rails.md +++ b/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 From 85cde3cde1c7712b2abb3383c1faa023f8392dae Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Tue, 20 Oct 2020 20:56:08 -0400 Subject: [PATCH 4/4] Remove redundant arguments in store test helper The store now uses $MEMCACHE_SERVERS and 127.0.0.1:11211 as fallbacks, so there is no need to specify them them again. --- activesupport/test/cache/stores/mem_cache_store_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/activesupport/test/cache/stores/mem_cache_store_test.rb b/activesupport/test/cache/stores/mem_cache_store_test.rb index fe6ca62458..ebbed847bc 100644 --- a/activesupport/test/cache/stores/mem_cache_store_test.rb +++ b/activesupport/test/cache/stores/mem_cache_store_test.rb @@ -202,7 +202,7 @@ class MemCacheStoreTest < ActiveSupport::TestCase end def store - [:mem_cache_store, ENV["MEMCACHE_SERVERS"] || "localhost:11211"] + [:mem_cache_store] end def emulating_latency