Use `$MEMCACHE_SERVERS` as `MemCacheStore` fallback #40420

sambostock · 2020-10-21T03:39:53Z

Summary

This makes ActiveSupport::Cache::MemCacheStore try falling back to ENV['MEMCACHE_SERVERS'] before using localhost:11211 if no addresses are given to it.

Details

By default, Dalli::Client 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:

-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.

Motivation

Dalli has deprecated the :dalli_store in favor of :mem_cache_store, with removal slated for version 3 of the gem. When migrating over, it is easy to assume that the new adapter will also check for $MEMCACHE_SERVERS. However, this assumption is wrong and effectively disables caching if used.

It is unlikely anyone is relying on the fallback to localhost:11211 on a system where $MEMCACHE_SERVERS is set to something different and they would not want to connect to it, so this change should be safe. Moreover, it improves ease of use and prevents easy mistakes. 🦶 🔫 🙅

Other Information

I extracted #40419 from this, which allowed me to properly run the MemCacheStore tests locally.

- Remove unused instance variable - Add client test helper

This documents existing behavior ahead of changes.

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.

The store now uses $MEMCACHE_SERVERS and 127.0.0.1:11211 as fallbacks, so there is no need to specify them them again.

sambostock · 2020-10-21T03:41:56Z

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)


I couldn't find anywhere this was used in this file or any of the included modules, and this file's tests continued to pass for me locally. 🤷

sambostock · 2020-10-21T03:43:43Z

activesupport/test/cache/stores/mem_cache_store_test.rb

+    def servers(cache = @cache)
+      client(cache).instance_variable_get(:@servers)
+    end
+
+    def client(cache = @cache)
+      cache.instance_variable_get(:@data)
+    end


I didn't feel great about doing this, but neither of these are publicly accessible, and we were already using cache.instance_variable_get(:@data) in a couple tests.

sambostock · 2020-10-21T03:44:26Z

activesupport/test/cache/stores/mem_cache_store_test.rb

+      if original_value.nil?
+        ENV.delete("MEMCACHE_SERVERS")


This might not be needed, but I figured it would make sense to remove it as a key entirely if it wasn't there originally.

sambostock · 2020-10-21T03:45:24Z

guides/CHANGELOG.md

+*   Updated `ActiveSupport::Cache::MemCacheStore` docs to reflect support for `$MEMCACHE_SERVERS`.
+
+    *Sam Bostock*
+


I'm not sure if this is required. I couldn't find anything describing what changes to the Guides require CHANGELOG entries.

sambostock · 2020-10-21T03:52:09Z

activesupport/test/cache/stores/mem_cache_store_test.rb

  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]


It seemed redundant not to leverage the code I just added 😅

`:dalli_store` falls back to `$MEMCACHE_SERVERS` if no addresses are given, whereas `:mem_cache_store` goes straight to `localhost:11211`. This is not immediately obvious, especially to users not providing any custom options, and makes it easy to accidentally effectively disable caching entirely (if `localhost:11211` is not a memcached server). Note that rails/rails#40420 would add support.

`:dalli_store` falls back to `$MEMCACHE_SERVERS` if no addresses are given, whereas (prior to Rails 6.1) `:mem_cache_store` goes straight to `localhost:11211`. This is not immediately obvious, especially to users not providing any custom options, and makes it easy to accidentally effectively disable caching entirely (if `localhost:11211` is not a memcached server). Note that rails/rails#40420 added support for `$MEMCACHE_SERVERS`, and is slated for release as part of Rails 6.1. [ci skip]

movermeyer · 2021-02-08T22:12:48Z

activesupport/lib/active_support/cache/mem_cache_store.rb

      #   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?


Users of :dalli_store may have been passing an explicit nil parameter for the servers:

config.cache_store = :dalli_cache, nil, { expires_in: 2.hour, compress: true }

If they simply changed :dalli_cache and :mem_cache_store, this code passes addresses = [nil] to Dalli (instead of nil, which cause exceptions when people try to access the cache.

NoMethodError: undefined method `match' for nil:NilClass

I've seen at least 2 cases within repos where this was the case, which confused hapless developers who were just trying to migrate in order to clear the deprecation warning (causing them to revert instead of debugging the issue).

Perhaps we could do something like add a .compact:

addresses = nil if addresses.compact.empty?

This would unify the interface between the two stores.

Or perhaps raise an exception that explained what they were doing wrong / the difference in interface between :dalli_store and :mem_cache_store.

I think it makes sense to explain, but I think what needs to explain how the migration should be done is the dalli_store project.

Of some relevance, I opened petergoldstein/dalli#764 a while back to document the change in implicit ENV variable handling.

I also think the compact version is fine. @movermeyer want to open a PR?

@rafaelfranca It would be my pleasure!

This is necessary in Rails 6.0. We don't get the automatic fallback to ENV['MEMCACHED_SERVERS'] until Rails 6.1 (rails/rails#40420).

sambostock added 2 commits October 20, 2020 22:53

Refactor MemCacheStoreTest

9445748

- Remove unused instance variable - Add client test helper

Add tests for :mem_cache_store initialization

f50be30

This documents existing behavior ahead of changes.

rails-bot bot added activesupport docs labels Oct 21, 2020

sambostock added 2 commits October 20, 2020 23:50

Remove redundant arguments in store test helper

85cde3c

The store now uses $MEMCACHE_SERVERS and 127.0.0.1:11211 as fallbacks, so there is no need to specify them them again.

sambostock commented Oct 21, 2020

View reviewed changes

sambostock force-pushed the use-dalli-fallback branch from f17acb3 to 85cde3c Compare October 21, 2020 03:52

sambostock mentioned this pull request Oct 21, 2020

Document :mem_cache_store $MEMCACHE_SERVERS incompatibility petergoldstein/dalli#764

Closed

rafaelfranca merged commit e66e057 into rails:master Oct 21, 2020

sambostock deleted the use-dalli-fallback branch October 21, 2020 14:25

movermeyer reviewed Feb 8, 2021

View reviewed changes

fatkodima mentioned this pull request Mar 9, 2022

Dalli::Client doesn't pick up the value of MEMCACHE_SERVERS when used with Rails #44642

Closed

jvendetti added a commit to ncbo/bioportal_web_ui that referenced this pull request Mar 17, 2022

Reinstate server addresses for cache_store

d3210a0

This is necessary in Rails 6.0. We don't get the automatic fallback to ENV['MEMCACHED_SERVERS'] until Rails 6.1 (rails/rails#40420).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `$MEMCACHE_SERVERS` as `MemCacheStore` fallback #40420

Use `$MEMCACHE_SERVERS` as `MemCacheStore` fallback #40420

sambostock commented Oct 21, 2020

sambostock Oct 21, 2020

sambostock Oct 21, 2020

sambostock Oct 21, 2020

sambostock Oct 21, 2020

sambostock Oct 21, 2020

movermeyer Feb 8, 2021

rafaelfranca Feb 8, 2021

sambostock Feb 9, 2021

rafaelfranca Feb 9, 2021

movermeyer Feb 9, 2021

movermeyer Feb 9, 2021

		* Updated `ActiveSupport::Cache::MemCacheStore` docs to reflect support for `$MEMCACHE_SERVERS`.

		Sam Bostock

Use $MEMCACHE_SERVERS as MemCacheStore fallback #40420

Use $MEMCACHE_SERVERS as MemCacheStore fallback #40420

Conversation

sambostock commented Oct 21, 2020

Summary

Details

Motivation

Other Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `$MEMCACHE_SERVERS` as `MemCacheStore` fallback #40420

Use `$MEMCACHE_SERVERS` as `MemCacheStore` fallback #40420