Merge pull request #260 from redis-rb/hiredis-reconnect

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

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

@@ -39,11 +39,8 @@ def initialize(ca_file: nil, ca_path: nil, cert: nil, key: nil, hostname: nil)
       end
     end
 
-    attr_reader :config
-
     def initialize(config, connect_timeout:, read_timeout:, write_timeout:)
-      super()
-      @config = config
+      super(config)
       self.connect_timeout = connect_timeout
       self.read_timeout = read_timeout
       self.write_timeout = write_timeout
@@ -134,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 = @config.path || "#{@config.host}:#{@config.port}"

lib/redis_client.rb

@@ -786,7 +786,7 @@ def raw_connection
   def connect
     @pid = PIDCache.pid
 
-    if @raw_connection
+    if @raw_connection&.revalidate
       @middlewares.connect(config) do
         @raw_connection.reconnect
       end

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

@@ -3,10 +3,13 @@
 class RedisClient
   module ConnectionMixin
     attr_accessor :retry_attempt
+    attr_reader :config
 
-    def initialize
+    def initialize(config)
       @pending_reads = 0
       @retry_attempt = nil
+      @config = config
+      @server_key = nil
     end
 
     def reconnect
@@ -20,7 +23,7 @@ def close
     end
 
     def revalidate
-      if @pending_reads > 0
+      if @pending_reads > 0 || @server_key != @config.server_key
         close
         false
       else

lib/redis_client/ruby_connection.rb

@@ -40,11 +40,8 @@ def ssl_context(ssl_params)
 
     SUPPORTS_RESOLV_TIMEOUT = Socket.method(:tcp).parameters.any? { |p| p.last == :resolv_timeout }
 
-    attr_reader :config
-
     def initialize(config, connect_timeout:, read_timeout:, write_timeout:)
-      super()
-      @config = config
+      super(config)
       @connect_timeout = connect_timeout
       @read_timeout = read_timeout
       @write_timeout = write_timeout
@@ -112,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