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