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

Fix: #254
Fix: #119
Ref: #119

Middlewares witness all network errors, but currently have no way of
knowing whether the error is final or is about to be retried.

In many case, you do want to distinguish the two because a low number
of transcient network errors can be expected.

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

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]
+      refute_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