From 1befb5a8c3f2af4063502416ba8c66a77dfa83c7 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 19 Sep 2025 14:31:01 +0200 Subject: [PATCH] Add `RedisClient::Error#final?` to segregate retriable errors in middlewares. Fix: https://github.com/redis-rb/redis-client/pull/254 Fix: https://github.com/redis-rb/redis-client/issues/119 Ref: https://github.com/redis-rb/redis-client/issues/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. --- CHANGELOG.md | 1 + README.md | 23 +++++++ .../lib/redis_client/hiredis_connection.rb | 9 ++- lib/redis_client.rb | 22 +++++++ lib/redis_client/config.rb | 4 ++ lib/redis_client/connection_mixin.rb | 15 +++++ lib/redis_client/ruby_connection.rb | 6 +- test/redis_client/middlewares_test.rb | 60 +++++++++++++++++++ test/support/client_test_helper.rb | 2 +- 9 files changed, 135 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f77e0e8..c4a290f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index cdcf575..1fa96db 100644 --- a/README.md +++ b/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. diff --git a/hiredis-client/lib/redis_client/hiredis_connection.rb b/hiredis-client/lib/redis_client/hiredis_connection.rb index b666a43..ee6a8a2 100644 --- a/hiredis-client/lib/redis_client/hiredis_connection.rb +++ b/hiredis-client/lib/redis_client/hiredis_connection.rb @@ -100,9 +100,10 @@ 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 @@ -110,9 +111,10 @@ 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 diff --git a/lib/redis_client.rb b/lib/redis_client.rb index 5149778..20ef0ca 100644 --- a/lib/redis_client.rb +++ b/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 diff --git a/lib/redis_client/config.rb b/lib/redis_client/config.rb index 0032f2f..6205368 100644 --- a/lib/redis_client/config.rb +++ b/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]) diff --git a/lib/redis_client/connection_mixin.rb b/lib/redis_client/connection_mixin.rb index 0ad25b7..021c66a 100644 --- a/lib/redis_client/connection_mixin.rb +++ b/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 diff --git a/lib/redis_client/ruby_connection.rb b/lib/redis_client/ruby_connection.rb index 56a98ce..70585bc 100644 --- a/lib/redis_client/ruby_connection.rb +++ b/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 diff --git a/test/redis_client/middlewares_test.rb b/test/redis_client/middlewares_test.rb index 1b317e1..127a3d1 100644 --- a/test/redis_client/middlewares_test.rb +++ b/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 diff --git a/test/support/client_test_helper.rb b/test/support/client_test_helper.rb index a9ea061..7f207c3 100644 --- a/test/support/client_test_helper.rb +++ b/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