From c65498c4baa3cd4d1182a5d7eec23fb2e6bed4bf Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 15 May 2024 09:54:05 -0700
Subject: [PATCH] Ensure BLPOP/BRPOP returns nil instead of raising
 ReadTimeoutError

In https://github.com/redis/redis-rb/issues/1279, we discovered that
`redis-rb` properly returned `nil` when `timeout` was reached with no
key present, but when connecting to Redis Sentinels, the client raised
a `ReadTimeoutTimeout` error.

This occurred because of a subtle difference in how `RedisClient`
(from `redis-rb`) and `Redis::Client` (from `redis-client`) behaved.
The former, which is used with standalone Redis, returned `nil`
because the socket read timeout was incremented to the command timeout
value (https://github.com/redis/redis-rb/pull/1175). The latter did
not have this, so the socket read timeout would get triggered before
the actual Redis timeout hit.

To make the behavior consistent, increment the configured read timeout
to the command timeout.

Closes https://github.com/redis/redis-rb/issues/1279
---
 lib/redis_client/connection_mixin.rb | 13 +++++++++++--
 test/shared/redis_client_tests.rb    | 26 ++++++++++++++++++++++----
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/lib/redis_client/connection_mixin.rb b/lib/redis_client/connection_mixin.rb
index 2de6b20..0ad25b7 100644
--- a/lib/redis_client/connection_mixin.rb
+++ b/lib/redis_client/connection_mixin.rb
@@ -28,7 +28,7 @@ def revalidate
     def call(command, timeout)
       @pending_reads += 1
       write(command)
-      result = read(timeout)
+      result = read(connection_timeout(timeout))
       @pending_reads -= 1
       if result.is_a?(Error)
         result._set_command(command)
@@ -49,7 +49,7 @@ def call_pipelined(commands, timeouts, exception: true)
 
       size.times do |index|
         timeout = timeouts && timeouts[index]
-        result = read(timeout)
+        result = read(connection_timeout(timeout))
         @pending_reads -= 1
 
         # A multi/exec command can return an array of results.
@@ -73,5 +73,14 @@ def call_pipelined(commands, timeouts, exception: true)
         results
       end
     end
+
+    def connection_timeout(timeout)
+      return timeout unless timeout && timeout > 0
+
+      # Can't use the command timeout argument as the connection timeout
+      # otherwise it would be very racy. So we add the regular read_timeout on top
+      # to account for the network delay.
+      timeout + config.read_timeout
+    end
   end
 end
diff --git a/test/shared/redis_client_tests.rb b/test/shared/redis_client_tests.rb
index 4a63f07..d802603 100644
--- a/test/shared/redis_client_tests.rb
+++ b/test/shared/redis_client_tests.rb
@@ -448,12 +448,30 @@ def test_blocking_call_timeout
     assert_equal "OK", @redis.call("SET", "foo", "bar")
   end
 
+  def test_blocking_call_long_timeout
+    client = new_client(timeout: 0.5)
+    assert_nil client.blocking_call(0.5, "BRPOP", "list", "0.5")
+    assert_nil client.blocking_call_v(0.5, ["BRPOP", "list", "0.5"])
+
+    result = client.pipelined do |pipeline|
+      pipeline.blocking_call(0.5, "BRPOP", "list", "0.5")
+      pipeline.blocking_call_v(0.5, ["BRPOP", "list", "0.5"])
+    end
+
+    assert_equal([nil, nil], result)
+
+    result = client.multi do |pipeline|
+      pipeline.call("BRPOP", "list", "0.5")
+      pipeline.call_v(["BRPOP", "list", "0.5"])
+    end
+
+    assert_equal([nil, nil], result)
+  end
+
   def test_blocking_call_timeout_retries
-    redis = new_client(reconnect_attempts: [3.0])
+    redis = new_client(timeout: 0.5, reconnect_attempts: [3.0])
     start = Process.clock_gettime(Process::CLOCK_MONOTONIC)
-    assert_raises RedisClient::ReadTimeoutError do
-      redis.blocking_call(0.1, "BRPOP", "list", "0.1")
-    end
+    assert_nil redis.blocking_call(0.1, "BRPOP", "list", "0.1")
     duration = Process.clock_gettime(Process::CLOCK_MONOTONIC) - start
     assert duration < 0.5 # if we retried we'd have waited much long
   end