DEV: Upgrade the Redis gem to v5.4

Gemfile

@@ -33,9 +33,7 @@ gem "mail"
 gem "mini_mime"
 gem "mini_suffix"
 
-# config/initializers/006-mini_profiler.rb depends upon the RedisClient#call.
-# Rework this when upgrading to redis client 5.0 and above.
-gem "redis", "< 5.0"
+gem "redis"
 
 # This is explicitly used by Sidekiq and is an optional dependency.
 # We tell Sidekiq to use the namespace "sidekiq" which triggers this

Gemfile.lock

@@ -427,7 +427,8 @@ GEM
     rdoc (6.12.0)
       psych (>= 4.0.0)
     redcarpet (3.6.1)
-    redis (4.8.1)
+    redis (5.4.0)
+      redis-client (>= 0.22.0)
     redis-client (0.24.0)
       connection_pool
     redis-namespace (1.11.0)
@@ -753,7 +754,7 @@ DEPENDENCIES
   rbtrace
   rchardet
   redcarpet
-  redis (< 5.0)
+  redis
   redis-namespace
   rinku
   rotp
@@ -1004,7 +1005,7 @@ CHECKSUMS
   rchardet (1.9.0) sha256=26889486cdd83b378652baf7603f71d93e431bb11bc237b4cd8c65151af4a590
   rdoc (6.12.0) sha256=7d6f706e070bffa5d18a448f24076cbfb34923a99c1eab842aa18e6ca69f56e0
   redcarpet (3.6.1) sha256=d444910e6aa55480c6bcdc0cdb057626e8a32c054c29e793fa642ba2f155f445
-  redis (4.8.1) sha256=387ee086694fffc9632aaeb1efe4a7b1627ca783bf373320346a8a20cd93333a
+  redis (5.4.0) sha256=798900d869418a9fc3977f916578375b45c38247a556b61d58cba6bb02f7d06b
   redis-client (0.24.0) sha256=ee65ee39cb2c38608b734566167fd912384f3c1241f59075e22858f23a085dbb
   redis-namespace (1.11.0) sha256=e91a1aa2b2d888b6dea1d4ab8d39e1ae6fac3426161feb9d91dd5cca598a2239
   regexp_parser (2.10.0) sha256=cb6f0ddde88772cd64bff1dbbf68df66d376043fe2e66a9ef77fcb1b0c548c61

app/models/concerns/cached_counting.rb

@@ -73,7 +73,7 @@ module CachedCounting
       iterations += 1
     end
   rescue => ex
-    if Redis::CommandError === ex && ex.message =~ /READONLY/
+    if Redis::ReadOnlyError === ex
       # do not warn for Redis readonly mode
     elsif PG::ReadOnlySqlTransaction === ex
       # do not warn for PG readonly mode

app/models/global_setting.rb

@@ -50,8 +50,8 @@ class GlobalSetting
         end
         token
       end
-  rescue Redis::CommandError => e
-    @safe_secret_key_base = SecureRandom.hex(64) if e.message =~ /READONLY/
+  rescue Redis::ReadOnlyError
+    @safe_secret_key_base = SecureRandom.hex(64)
   end
 
   def self.load_defaults
@@ -209,9 +209,11 @@ class GlobalSetting
         c[:port] = redis_port if redis_port
 
         if get_redis_replica_host && get_redis_replica_port && defined?(RailsFailover)
-          c[:replica_host] = get_redis_replica_host
-          c[:replica_port] = get_redis_replica_port
-          c[:connector] = RailsFailover::Redis::Connector
+          c[:client_implementation] = RailsFailover::Redis::Client
+          c[:custom] = {
+            replica_host: get_redis_replica_host,
+            replica_port: get_redis_replica_port,
+          }
         end
 
         c[:username] = redis_username if redis_username.present?
@@ -234,9 +236,11 @@ class GlobalSetting
         c[:port] = message_bus_redis_port if message_bus_redis_port
 
         if get_message_bus_redis_replica_host && get_message_bus_redis_replica_port
-          c[:replica_host] = get_message_bus_redis_replica_host
-          c[:replica_port] = get_message_bus_redis_replica_port
-          c[:connector] = RailsFailover::Redis::Connector
+          c[:client_implementation] = RailsFailover::Redis::Client
+          c[:custom] = {
+            replica_host: get_message_bus_redis_replica_host,
+            replica_port: get_message_bus_redis_replica_port,
+          }
         end
 
         c[:username] = message_bus_redis_username if message_bus_redis_username.present?

config/initializers/006-mini_profiler.rb

@@ -95,7 +95,7 @@ if defined?(Rack::MiniProfiler) && defined?(Rack::MiniProfiler::Config)
       end
     end
 
-  Rack::MiniProfiler.counter_method(Redis::Client, :call) { "redis" }
+  Rack::MiniProfiler.counter_method(Redis::Client, :call_v) { "redis" }
 end
 
 if ENV["PRINT_EXCEPTIONS"]

lib/discourse.rb

@@ -1035,8 +1035,7 @@ module Discourse
       Rails.logger.warn(warning)
       begin
         Discourse.redis.without_namespace.setex(redis_key, 3600, "x")
-      rescue Redis::CommandError => e
-        raise unless e.message =~ /READONLY/
+      rescue Redis::ReadOnlyError
       end
     end
     warning
@@ -1048,7 +1047,7 @@ module Discourse
     GlobalSetting
       .redis_config
       .dup
-      .except(:connector, :replica_host, :replica_port)
+      .except(:client_implementation, :custom)
       .tap { |config| config.merge!(db: config[:db].to_i + 1) unless old }
   end
 

lib/discourse_redis.rb

@@ -27,13 +27,9 @@ class DiscourseRedis
 
   def self.ignore_readonly
     yield
-  rescue Redis::CommandError => ex
-    if ex.message =~ /READONLY/
-      Discourse.received_redis_readonly!
-      nil
-    else
-      raise ex
-    end
+  rescue Redis::ReadOnlyError
+    Discourse.received_redis_readonly!
+    nil
   end
 
   # prefix the key with the namespace
@@ -214,7 +210,7 @@ class DiscourseRedis
   end
 
   def reconnect
-    @redis._client.reconnect
+    @redis.close
   end
 
   def namespace_key(key)

lib/discourse_updates.rb

@@ -223,7 +223,7 @@ module DiscourseUpdates
     end
 
     def bump_last_viewed_feature_date(user_id, feature_date)
-      Discourse.redis.hset(last_viewed_feature_dates_for_users_key, user_id.to_s, feature_date)
+      Discourse.redis.hset(last_viewed_feature_dates_for_users_key, user_id.to_s, feature_date.to_s)
     end
 
     def clean_state

lib/distributed_mutex.rb

@@ -39,7 +39,7 @@ class DistributedMutex
     @using_global_redis = true if !redis
     @redis = redis || Discourse.redis
     @mutex = Mutex.new
-    @validity = validity
+    @validity = validity.to_i
   end
 
   # NOTE wrapped in mutex to maintain its semantics

lib/method_profiler.rb

@@ -8,21 +8,21 @@ class MethodProfiler
         .map do |method_name|
           recurse_protection = ""
           recurse_protection = <<~RUBY if no_recurse
-          return #{method_name}__mp_unpatched(*args, &blk) if @mp_recurse_protect_#{method_name}
+          return #{method_name}__mp_unpatched(*args, **kwargs, &blk) if @mp_recurse_protect_#{method_name}
           @mp_recurse_protect_#{method_name} = true
         RUBY
 
           <<~RUBY
       unless defined?(#{method_name}__mp_unpatched)
         alias_method :#{method_name}__mp_unpatched, :#{method_name}
-        def #{method_name}(*args, &blk)
+        def #{method_name}(*args, **kwargs, &blk)
           unless prof = Thread.current[:_method_profiler]
-            return #{method_name}__mp_unpatched(*args, &blk)
+            return #{method_name}__mp_unpatched(*args, **kwargs, &blk)
           end
           #{recurse_protection}
           begin
             start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
-            #{method_name}__mp_unpatched(*args, &blk)
+            #{method_name}__mp_unpatched(*args, **kwargs, &blk)
           ensure
             data = (prof[:#{name}] ||= {duration: 0.0, calls: 0})
             data[:duration] += Process.clock_gettime(Process::CLOCK_MONOTONIC) - start
@@ -44,14 +44,14 @@ class MethodProfiler
         .map do |method_name|
           recurse_protection = ""
           recurse_protection = <<~RUBY if no_recurse
-          return #{method_name}__mp_unpatched_debug_sql(*args, &blk) if @mp_recurse_protect_#{method_name}
+          return #{method_name}__mp_unpatched_debug_sql(*args, **kwargs, &blk) if @mp_recurse_protect_#{method_name}
           @mp_recurse_protect_#{method_name} = true
         RUBY
 
           <<~RUBY
       unless defined?(#{method_name}__mp_unpatched_debug_sql)
         alias_method :#{method_name}__mp_unpatched_debug_sql, :#{method_name}
-        def #{method_name}(*args, &blk)
+        def #{method_name}(*args, **kwargs, &blk)
           #{recurse_protection}
 
           query = args[0]
@@ -63,7 +63,7 @@ class MethodProfiler
 
           begin
             start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
-            #{method_name}__mp_unpatched_debug_sql(*args, &blk)
+            #{method_name}__mp_unpatched_debug_sql(*args, **kwargs, &blk)
           ensure
             duration = Process.clock_gettime(Process::CLOCK_MONOTONIC) - start
 
@@ -144,7 +144,8 @@ class MethodProfiler
           :sql,
         )
 
-        MethodProfiler.patch(Redis::Client, %i[call call_pipeline], :redis)
+        MethodProfiler.patch(Redis::Client, %i[call_v], :redis)
+        MethodProfiler.patch(RedisClient::RubyConnection, %i[call_pipelined], :redis)
 
         MethodProfiler.patch(Net::HTTP, [:request], :net, no_recurse: true)
 

lib/middleware/anonymous_cache.rb

@@ -317,7 +317,8 @@ module Middleware
 
         if status == 200 && cache_duration
           if GlobalSetting.anon_cache_store_threshold > 1
-            count = REDIS_STORE_SCRIPT.eval(Discourse.redis, [cache_key_count], [cache_duration])
+            count =
+              REDIS_STORE_SCRIPT.eval(Discourse.redis, [cache_key_count], [cache_duration.to_i])
 
             # technically lua will cast for us, but might as well be
             # prudent here, hence the to_i

lib/presence_channel.rb

@@ -151,7 +151,7 @@ class PresenceChannel
     mutex_value = SecureRandom.hex
     result =
       retry_on_mutex_error do
-        PresenceChannel.redis_eval(:leave, redis_keys, [name, user_id, client_id, nil, mutex_value])
+        PresenceChannel.redis_eval(:leave, redis_keys, [name, user_id, client_id, "", mutex_value])
       end
 
     if result == 1

lib/rate_limiter.rb

@@ -50,7 +50,7 @@ class RateLimiter
     @type = type
     @key = build_key(type)
     @max = max
-    @secs = secs
+    @secs = secs.to_i
     @global = global
     @aggressive = aggressive
     @error_code = error_code
@@ -60,7 +60,7 @@ class RateLimiter
     # override the default values if staff user, and staff specific max is passed
     if @user&.staff? && !@apply_limit_to_staff && @staff_limit[:max].present?
       @max = @staff_limit[:max]
-      @secs = @staff_limit[:secs]
+      @secs = @staff_limit[:secs].to_i
     end
   end
 
@@ -131,23 +131,15 @@ class RateLimiter
     else
       true
     end
-  rescue Redis::CommandError => e
-    if e.message =~ /READONLY/
-      # TODO,switch to in-memory rate limiter
-    else
-      raise
-    end
+  rescue Redis::ReadOnlyError
+    # TODO,switch to in-memory rate limiter
   end
 
   def rollback!
     return if RateLimiter.disabled?
     redis.lpop(prefixed_key)
-  rescue Redis::CommandError => e
-    if e.message =~ /READONLY/
-      # TODO,switch to in-memory rate limiter
-    else
-      raise
-    end
+  rescue Redis::ReadOnlyError
+    # TODO,switch to in-memory rate limiter
   end
 
   def remaining
@@ -162,7 +154,6 @@ class RateLimiter
   private
 
   def rate_limiter_allowed?(now)
-    lua, lua_sha = nil
     eval_helper = @aggressive ? PERFORM_LUA_AGGRESSIVE : PERFORM_LUA
 
     eval_helper.eval(redis, [prefixed_key], [now, @secs, @max]) == 0

spec/lib/discourse_redis_spec.rb

@@ -2,7 +2,7 @@
 
 RSpec.describe DiscourseRedis do
   it "ignore_readonly returns nil from a pure exception" do
-    result = DiscourseRedis.ignore_readonly { raise Redis::CommandError.new("READONLY") }
+    result = DiscourseRedis.ignore_readonly { raise Redis::ReadOnlyError }
     expect(result).to eq(nil)
   end
 
@@ -53,7 +53,7 @@ RSpec.describe DiscourseRedis do
       end
 
       it "should noop pipelined commands against a readonly redis" do
-        redis.without_namespace.expects(:pipelined).raises(Redis::CommandError.new("READONLY"))
+        redis.without_namespace.expects(:pipelined).raises(Redis::ReadOnlyError)
 
         set, incr = nil
 
@@ -69,7 +69,7 @@ RSpec.describe DiscourseRedis do
       end
 
       it "should noop multi commands against a readonly redis" do
-        redis.without_namespace.expects(:multi).raises(Redis::CommandError.new("READONLY"))
+        redis.without_namespace.expects(:multi).raises(Redis::ReadOnlyError)
 
         val =
           redis.multi do |transaction|
@@ -153,7 +153,7 @@ RSpec.describe DiscourseRedis do
       it "should noop a readonly redis" do
         expect(Discourse.recently_readonly?).to eq(false)
 
-        redis.without_namespace.expects(:set).raises(Redis::CommandError.new("READONLY"))
+        redis.without_namespace.expects(:set).raises(Redis::ReadOnlyError)
 
         redis.set("key", 1)
 

spec/lib/distributed_mutex_spec.rb

@@ -39,7 +39,7 @@ RSpec.describe DistributedMutex do
   end
 
   it "allows the validity of the lock to be configured" do
-    mutex = DistributedMutex.new(key, validity: 2)
+    mutex = DistributedMutex.new(key, validity: 2.seconds)
 
     mutex.synchronize do
       expect(Discourse.redis.ttl(key)).to be <= 3

spec/lib/presence_channel_spec.rb

@@ -66,7 +66,7 @@ RSpec.describe PresenceChannel do
   end
 
   it "does not raise error when getting channel config under readonly" do
-    PresenceChannel.redis.stubs(:set).raises(Redis::CommandError.new("READONLY")).once
+    PresenceChannel.redis.stubs(:set).raises(Redis::ReadOnlyError).once
     channel = PresenceChannel.new("/test/public1")
     expect(channel.user_ids).to eq([])
   end

spec/models/global_setting_spec.rb

@@ -87,21 +87,29 @@ RSpec.describe GlobalSetting do
 
   describe ".redis_config" do
     describe "when replica config is not present" do
-      it "should not set any connector" do
-        expect(GlobalSetting.redis_config[:connector]).to eq(nil)
+      it "does not set any custom client nor replica config" do
+        expect(GlobalSetting.redis_config[:client_implementation]).to be_nil
+        expect(GlobalSetting.redis_config[:custom]).to be_nil
       end
     end
 
     describe "when replica config is present" do
-      before { GlobalSetting.reset_redis_config! }
+      before do
+        GlobalSetting.reset_redis_config!
+        GlobalSetting.expects(:redis_replica_port).returns(6379).at_least_once
+        GlobalSetting.expects(:redis_replica_host).returns("0.0.0.0").at_least_once
+      end
 
       after { GlobalSetting.reset_redis_config! }
 
-      it "should set the right connector" do
-        GlobalSetting.expects(:redis_replica_port).returns(6379).at_least_once
-        GlobalSetting.expects(:redis_replica_host).returns("0.0.0.0").at_least_once
-
-        expect(GlobalSetting.redis_config[:connector]).to eq(RailsFailover::Redis::Connector)
+      it "sets the right config" do
+        expect(GlobalSetting.redis_config).to include(
+          client_implementation: RailsFailover::Redis::Client,
+          custom: {
+            replica_host: "0.0.0.0",
+            replica_port: 6379,
+          },
+        )
       end
     end
   end