Merge pull request #255 from redis-rb/middleware-final-errors

Add `RedisClient::Error#final?` to segregate retriable errors in middlewares

Author: byroot

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
 
+- Add `RedisClient::Error#final?` and `#retriable?` to allow middleware to filter out non-final errors.
 - Fix precedence of `db: nil` initialization parameter.
 
   ```ruby

README.md

@@ -459,6 +459,29 @@ RedisClient.register(MyGlobalRedisInstrumentation)
 
 redis_config = RedisClient.config(custom: { tags: { "environment": Rails.env }})
 ```
+
+### Instrumenting Errors
+
+It is important to note that when `reconnect_attempts` is enabled, all network errors are reported to the middlewares,
+even the ones that will be retried.
+
+In many cases you may want to ignore retriable errors, or report them differently:
+
+```ruby
+module MyGlobalRedisInstrumentation
+  def call(command, redis_config)
+    super
+  rescue RedisClient::Error => error
+    if error.final?
+      # Error won't be retried.
+    else
+      # Error will be retried.
+    end
+    raise
+  end
+end
+```
+
 ### Timeouts
 
 The client allows you to configure connect, read, and write timeouts.

hiredis-client/lib/redis_client/hiredis_connection.rb

@@ -100,19 +100,21 @@ def read(timeout = nil)
         end
       end
     rescue SystemCallError, IOError => error
-      raise ConnectionError.with_config(error.message, config)
+      raise connection_error(error.message)
     rescue Error => error
       error._set_config(config)
+      error._set_retry_attempt(@retry_attempt)
       raise error
     end
 
     def write(command)
       _write(command)
       flush
     rescue SystemCallError, IOError => error
-      raise ConnectionError.with_config(error.message, config)
+      raise connection_error(error.message)
     rescue Error => error
       error._set_config(config)
+      error._set_retry_attempt(@retry_attempt)
       raise error
     end
 
@@ -122,9 +124,10 @@ def write_multi(commands)
       end
       flush
     rescue SystemCallError, IOError => error
-      raise ConnectionError.with_config(error.message, config)
+      raise connection_error(error.message)
     rescue Error => error
       error._set_config(config)
+      error._set_retry_attempt(@retry_attempt)
       raise error
     end
 

lib/redis_client.rb

@@ -99,8 +99,27 @@ def message
     end
   end
 
+  module Retriable
+    def _set_retry_attempt(retry_attempt)
+      @retry_attempt = retry_attempt
+    end
+
+    def retry_attempt
+      @retry_attempt || 0
+    end
+
+    def retriable?
+      !!@retry_attempt
+    end
+
+    def final?
+      !@retry_attempt
+    end
+  end
+
   class Error < StandardError
     include HasConfig
+    include Retriable
 
     def self.with_config(message, config = nil)
       error = new(message)
@@ -706,6 +725,7 @@ def ensure_connected(retryable: true)
     close if !config.inherit_socket && @pid != PIDCache.pid
 
     if @disable_reconnection
+      @raw_connection.retry_attempt = nil
       if block_given?
         yield @raw_connection
       else
@@ -717,6 +737,7 @@ def ensure_connected(retryable: true)
       preferred_error = nil
       begin
         connection = raw_connection
+        connection.retry_attempt = config.retriable?(tries) ? tries : nil
         if block_given?
           yield connection
         else
@@ -744,6 +765,7 @@ def ensure_connected(retryable: true)
       connection = ensure_connected
       begin
         @disable_reconnection = true
+        @raw_connection.retry_attempt = nil
         yield connection
       rescue ConnectionError, ProtocolError
         close

lib/redis_client/config.rb

@@ -140,6 +140,10 @@ def new_client(**kwargs)
         @client_implementation.new(self, **kwargs)
       end
 
+      def retriable?(attempt)
+        @reconnect_attempts && @reconnect_attempts[attempt]
+      end
+
       def retry_connecting?(attempt, _error)
         if @reconnect_attempts
           if (pause = @reconnect_attempts[attempt])

lib/redis_client/connection_mixin.rb

@@ -2,8 +2,11 @@
 
 class RedisClient
   module ConnectionMixin
+    attr_accessor :retry_attempt
+
     def initialize
       @pending_reads = 0
+      @retry_attempt = nil
     end
 
     def reconnect
@@ -33,6 +36,7 @@ def call(command, timeout)
       if result.is_a?(Error)
         result._set_command(command)
         result._set_config(config)
+        result._set_retry_attempt(@retry_attempt)
         raise result
       else
         result
@@ -61,6 +65,7 @@ def call_pipelined(commands, timeouts, exception: true)
         elsif result.is_a?(Error)
           result._set_command(commands[index])
           result._set_config(config)
+          result._set_retry_attempt(@retry_attempt)
           first_exception ||= result
         end
 
@@ -82,5 +87,15 @@ def connection_timeout(timeout)
       # to account for the network delay.
       timeout + config.read_timeout
     end
+
+    def protocol_error(message)
+      ProtocolError.with_config(message, config)
+    end
+
+    def connection_error(message)
+      error = ConnectionError.with_config(message, config)
+      error._set_retry_attempt(@retry_attempt)
+      error
+    end
   end
 end

lib/redis_client/ruby_connection.rb

@@ -75,7 +75,7 @@ def write(command)
       begin
         @io.write(buffer)
       rescue SystemCallError, IOError, OpenSSL::SSL::SSLError => error
-        raise ConnectionError.with_config(error.message, config)
+        raise connection_error(error.message)
       end
     end
 
@@ -87,7 +87,7 @@ def write_multi(commands)
       begin
         @io.write(buffer)
       rescue SystemCallError, IOError, OpenSSL::SSL::SSLError => error
-        raise ConnectionError.with_config(error.message, config)
+        raise connection_error(error.message)
       end
     end
 
@@ -100,7 +100,7 @@ def read(timeout = nil)
     rescue RedisClient::RESP3::UnknownType => error
       raise RedisClient::ProtocolError.with_config(error.message, config)
     rescue SystemCallError, IOError, OpenSSL::SSL::SSLError => error
-      raise ConnectionError.with_config(error.message, config)
+      raise connection_error(error.message)
     end
 
     def measure_round_trip_delay

test/redis_client/middlewares_test.rb

@@ -69,6 +69,66 @@ def test_multi_instrumentation
       ]
     end
 
+    def test_final_errors
+      client = new_client(reconnect_attempts: 1)
+      simulate_network_errors(client, ["PING"]) do
+        assert_equal("PONG", client.call("PING"))
+      end
+
+      calls = TestMiddleware.calls.select { |type, _| type == :call }
+      assert_equal 2, calls.size
+
+      call = calls[0]
+      assert_equal :error, call[1]
+      assert_equal ["PING"], call[2]
+      refute_predicate call[3], :final?
+
+      call = calls[1]
+      assert_equal :success, call[1]
+      assert_equal ["PING"], call[2]
+
+      TestMiddleware.calls.clear
+
+      client = new_client(reconnect_attempts: 1)
+      simulate_network_errors(client, ["PING", "PING"]) do
+        assert_raises ConnectionError do
+          client.call("PING")
+        end
+      end
+
+      calls = TestMiddleware.calls.select { |type, _| type == :call }
+      assert_equal 2, calls.size
+
+      call = calls[0]
+      assert_equal :error, call[1]
+      assert_equal ["PING"], call[2]
+      refute_predicate call[3], :final?
+
+      call = calls[1]
+      assert_equal :error, call[1]
+      assert_equal ["PING"], call[2]
+      assert_predicate call[3], :final?
+
+      TestMiddleware.calls.clear
+
+      client = new_client(reconnect_attempts: 1)
+      simulate_network_errors(client, ["PING"]) do
+        assert_raises ConnectionError do
+          client.call_once("PING")
+        end
+      end
+
+      calls = TestMiddleware.calls.select { |type, _| type == :call }
+      assert_equal 1, calls.size
+
+      call = calls[0]
+      assert_equal :error, call[1]
+      assert_equal ["PING"], call[2]
+      assert_predicate call[3], :final?
+
+      TestMiddleware.calls.clear
+    end
+
     module DummyMiddleware
       def call(command, _config, &_)
         command

test/support/client_test_helper.rb

@@ -21,7 +21,7 @@ def write(command)
     def read(*)
       @fail_now ||= false
       if @fail_now
-        raise ::RedisClient::ConnectionError, "simulated failure"
+        raise connection_error("simulated failure")
       end
 
       super