Hiredis+sentinel don't re-use connections after a failover

Fix: #257

The `hiredis` C library isn't capable of reconnecting to a different
server, only to the same server.

For classic client it's not a big deal, but with sentinel, a failover
may have happened, hence the host or port may have changed.

Author: byroot

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Unreleased
 
+- hiredis-client: Properly reconnect to the new leader after a sentinel failover.
+
 # 0.26.0
 
 - Add `RedisClient::Error#final?` and `#retriable?` to allow middleware to filter out non-final errors.

hiredis-client/lib/redis_client/hiredis_connection.rb

@@ -131,6 +131,7 @@ def write_multi(commands)
     private
 
     def connect
+      @server_key = @config.server_key
       _connect(@config.path, @config.host, @config.port, @config.ssl_context)
     rescue SystemCallError => error
       host = @path || "#{@host}:#{@port}"

lib/redis_client/config.rb

@@ -186,7 +186,7 @@ def server_url
 
     include Common
 
-    attr_reader :host, :port, :path
+    attr_reader :host, :port, :path, :server_key
 
     def initialize(
       url: nil,
@@ -220,6 +220,8 @@ def initialize(
         @host = host || DEFAULT_HOST
         @port = Integer(port || DEFAULT_PORT)
       end
+
+      @server_key = [@path, @host, @port].freeze
     end
   end
 end

lib/redis_client/connection_mixin.rb

@@ -9,9 +9,7 @@ def initialize(config)
       @pending_reads = 0
       @retry_attempt = nil
       @config = config
-      @path = @config.path
-      @host = @config.host
-      @port = @config.port
+      @server_key = nil
     end
 
     def reconnect
@@ -25,7 +23,7 @@ def close
     end
 
     def revalidate
-      if @pending_reads > 0 && !config_changed?
+      if @pending_reads > 0 || @server_key != @config.server_key
         close
         false
       else
@@ -102,11 +100,5 @@ def connection_error(message)
       error._set_retry_attempt(@retry_attempt)
       error
     end
-
-    private
-
-    def config_changed?
-      !(@path == @config.path && @host == @config.host && @port == @config.port)
-    end
   end
 end

lib/redis_client/ruby_connection.rb

@@ -109,6 +109,7 @@ def measure_round_trip_delay
     private
 
     def connect
+      @server_key = @config.server_key
       socket = if @config.path
         UNIXSocket.new(@config.path)
       else

lib/redis_client/sentinel_config.rb

@@ -82,6 +82,10 @@ def reset
       end
     end
 
+    def server_key
+      config.server_key
+    end
+
     def host
       config.host
     end

test/redis_client/connection_test.rb

@@ -205,6 +205,15 @@ def test_reconnect_reuse
       end
     end
 
+    def test_reconnect_config_change
+      assert_equal "PONG", @redis.call("PING")
+      @redis.close
+      @redis.instance_variable_set(:@config, RedisClient.config(**tcp_config, port: 1))
+      assert_raises RedisClient::CannotConnectError do
+        @redis.call("PING")
+      end
+    end
+
     def test_circuit_breaker
       circuit_breaker = CircuitBreaker.new(error_threshold: 3, success_threshold: 2, error_timeout: 1)
       @redis = new_client(circuit_breaker: circuit_breaker)

test/sentinel/sentinel_test.rb

@@ -147,9 +147,11 @@ def test_master_failover_ready
       stub(@config, :sentinel_client, ->(_config) { sentinel_client_mock }) do
         client = @config.new_client
         assert_equal "PONG", client.call("PING")
+        initial_server_key = @config.server_key
 
         Toxiproxy[Servers::REDIS.name].down do
           assert_equal "PONG", client.call("PING")
+          refute_equal initial_server_key, @config.server_key
         end
       end
     ensure