Redis.current= is deprecated and will be removed in 5.0

[astronaut-wannabe]

While running through the README, I noticed this deprecation warning:

./run rails db:setup
# `Redis.current=` is deprecated and will be removed in 5.0. (called from: /app/config/initializers/redis.rb:1:in `<main>')
#    (138.5ms)  CREATE DATABASE "hello_development" ENCODING = 'unicode'
# Created database 'hello_development'
#    ...
#    ...
#    ...

I have just started dabbling with your template, but from a
glance it seems like you only use Redis.current:

1. in the redis initializer, to set the Redis.current value.
2. in the health-check controller, to ping redis.

It seems like all other redis integrations use REDIS_URL directly,
so they would not be impacted by leaving Redis.current as nil.

Possible fixes I see include:

1. set a global $redis in the initializer, use that $redis in place of Redis.current

   or

2. in-line Redis.new(url: ENV.fetch("REDIS_URL")) wherever you need to

I can see a case for either, but this commit implements the global
variable version. I'm happy to in-line if that's the preferred pattern
to establish in your template

[nickjj]

Hi,

Thanks. To my understanding this global version is basically the equivalent of Redis.current except now we're explicitly understanding this is a global?

There's an open issue at redis/redis-rb#1064 which goes over potential alternatives (I commented in there too). I'm torn between which option to choose. If the global approach is the same as Redis.current then we're in no worse shape than before except now the deprecation notice is handled.

With Puma being multi-threaded I'm just not sure how good of an option this will be.

Redis is currently being used for this health check, Rails caching, Sidekiq and Action Cable.

[astronaut-wannabe]

Hello @nickjj,

Thanks. To my understanding this global version is basically the equivalent of `Redis.current` except now we're explicitly understanding this is a global?

Yes, this commit would result in the same situation in a technical sense. It removes the risk of anything breaking on a future gem update, but punts a more thought-out solution into the future.

I agree with the sentiment expressed in that linked issue. Namely, that the "correct" thing to do seems very application specific. So I went with the global variable solution since it seemed like that might be the easiest to migrate away from if the specific application required a more intricate interaction with redis

That said, after reading your linked comment I saw you mention that your main goals are:

1. to ping redis in the up_controller
2. have a convenient way to get a redis client whenever you may need one

That got me thinking that maybe some sort of global function would be a better solution to implement.

So you could do something like

    module SomeModule
      REDIS_URL = ENV.fetch("REDIS_URL") { "redis://redis:6379/1" }
    
      def redis(url: REDIS_URL)
        Redis.new(url: url)
      end
    end

and then wherever you need an instance you could do something like

    SomeModule.redis.ping

This way is:

1. just about as convenient as Redis.current
2. you no longer have a globally shared redis instance
3. it is (potentially) forward-compatible. If additional logic is needed in the future, you have one location to modify or branch from.

Redis is currently being used for this health check, Rails caching, Sidekiq and Action Cable.

I saw that sidekiq,action cable, and the rails cache all pass the
REDIS_URL to their libraries, so I am operating on the assumption that those libraries are
creating and managing their own redis instances under the hood

[astronaut-wannabe]

I didn't actually mean to close this PR, I misclicked

[nickjj]

I wonder if we could reach in and grab the Rails cache's instance of Redis.

I guess the trade off here is a global vs creating a new Redis connection on every health check that happens to reference the endpoint that includes a Redis check.

[astronaut-wannabe]

🤔 that does seem possible since ActiveSupport::Cache::RedisCacheStore does have a #redis method. So Rails.cache.redis.ping would likely work.

The weird thing about that is in test and development you might need to check for NullStore, which I think would require:

• up_controller#databases will now possibly have different behavior based on RAILS_ENV
• You might need to stub Rails.cache in the up_controller test to return a mock cache instance

Both of these things feel a little bit icky, though maybe there is a way to do this without thee two things

Edit: I went ahead and modified the code to use Rails.cache.redis when RedisCacheStore is available, and noop otherwise.

I also noticed the development was using memory_store rather than redis_store, yet the redis service is run on docker up, so it seems like it makes sense to use the redis cache in development too. Curious as to your thoughts on that

[nickjj]

Redis will always be there in dev / prod, it's defined here:

docker-rails-example/config/application.rb

Lines 21 to 24 in 3f5e55c

	config.cache_store = :redis_cache_store, {
	url: ENV.fetch("REDIS_URL") { "redis://redis:6379/1" },
	namespace: "cache"
	}

I don't think we need to do any defensive programming to ensure it's available. That would mean reverting your changes in development.rb and not needing the private method in the up controller. The initializer could go away too. We could just reach in and use Rails.cache.redis.ping as needed anywhere in the code base.

Using Rails.cache.redis executes this: https://github.com/rails/rails/blob/de53ba56cab69fb9707785a397a59ac4aaee9d6f/activesupport/lib/active_support/cache/redis_cache_store.rb#L155

I'm not really sure what the trade offs are here but I have a hunch Rails defining an ability to access the Redis client is probably better than setting up a global? What do you think?

[astronaut-wannabe]

I saw the definition in application.rb, however when I run ./run rails dev:cache followed by docker-compose up, I find Rails.cache is returning memory_cache_store when I break on a debugger in the up_controller. Maybe I am doing something wrong to get development to run redis as the cache?

I removed the private method and everything, but this makes ./run test blow up due to Rails.env.cache returning a NullCacheStore in test

I'm not really sure what the trade offs are here but I have a hunch Rails defining an ability to access the Redis client is probably better than setting up a global? What do you think?

I agree with you. I think it makes a lot of sense to use Rails.cache.redis.ping in the up_controller. In a way you are conceptually health-checking more things now. Both that redis can be reached, and that the Rail's cache is properly wired up to use redis

I don't think there is an immediate tradeoff for the up_controller use case. I think maybe going through the Rails cache might be a bad thing if the app you are building uses redis a lot outside of the standard sidekiq/action cable/rails.cache uses cases

If you needed to heavily use redis directly, you might need to run your "business logic redis" and "just a cache redis" into separate redis services. Which could be a pain if you've decided you needed to do that after building out a lot of code using Rails.cache.redis. However, that hypothetical seems out-of-scope for this template

[nickjj]

Ah right, tests. That makes things a bit more complicated. I don't think we should have special code paths in our application to account for tests. Not checking Redis in tests would also be bad. I wonder what the implications would be to use Redis in our tests.

As for dev:cache, maybe that's related to https://github.com/nickjj/docker-rails-example/blob/main/config/environments/development.rb#L21. I don't really use this feature to be honest.

[nickjj]

Hi,

I see you closed your PR recently. I heard Rails 7.1+ will support Redis 5+ so I think this issue should be addressed sooner than later.

Here's what I have in an uncommit branch which is working:

# config/initializers/redis.rb
module RedisClient
  class << self
    def current
      @redis ||= Redis.new(url: ENV.fetch("REDIS_URL") { "redis://redis:6379/1" })
    end
  end
end

Then you can call RedisClient.current.ping in the up controller or anywhere you need it.

Thinking about shipping this, do you have any last minute comments before it goes in?

[nickjj]

I ended up rolling with it. It's commit to master at 790a6b3.