Handle timeouts to blocking_v specially via a custom client class

[KJTsanaktsidis]

try_send already handles the case where the call is a blocking one specially, but try_delegate does not. This diff detects whether or not a ::RedisClient::ReadTimeoutError is for a blocking call at the source, and wraps it in a special subclass so we can differentiate it.

This is done for two reasons:

• One, it means that we don't retry when a blocking command like BLPOP takes too long and is called through the #try_delegate codepath, and also don't unnecessarily refresh the cluster topology in this case either
• More importantly though, it means that the error handling between try_send and try_delegate is now identical, and we can refactor this into a single helper function in a subsequent PR.

[supercaracal]

I think this handling is better and elegant. Also, I'd say that we would be better to add this handling to redis-client side. What is your thoughts on this?

https://github.com/redis-rb/redis-client/blob/bda8fd604eda6abd87eb4274178fb3977a400a3e/lib/redis_client.rb#L313-L351

[KJTsanaktsidis]

Thanks for prompting me to dig into this. So I had a closer look... I don't think we should rescue from ::RedisClient::TimeoutError at all.

When using a command like BLPOP, there are two timeouts: The one which is "how long will i wait on this socket before raising a ReadTimeout error", and the one which is "how long will I ask the Redis server to block before it returns nil".

redis-client lets you specify the two directly

irb(main):013:0> rcl = RedisClient.config(url: 'redis://127.0.0.1:6381').new_client
=> #<RedisClient redis://127.0.0.1:6381/0>
# If the sent-to-redis timeout is lower than the socket timeout, you get `nil` on timeout
irb(main):014:0> rcl.blocking_call_v(2, ['BLPOP', 'hello', '1'])
=> nil
# If the sent-to-redius timeout is higher than the socket timeout, you get the error
irb(main):015:0> rcl.blocking_call_v(1, ['BLPOP', 'hello', '2'])
/Users/ktsanaktsidis/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/redis-client-0.18.0/lib/redis_client/ruby_connection/buffered_io.rb:140:in `block in fill_buffer': Waited 1 seconds (RedisClient::ReadTimeoutError)

But obviously you should be waiting on the socket for longer than you're asking Redis to wait. redis-rb handles this directly itself here: https://github.com/redis/redis-rb/blob/935f64f18d7c37386e9b01db71683f8f3b868cd2/lib/redis/client.rb#L95-L106

So, when using blocking commands in redis-rb, you always get nil, not a timeout error:

irb(main):016:0> redis = Redis.new(url: 'redis://127.0.0.1:6381')
=> #<Redis client v5.0.8 for redis://127.0.0.1:6381/0>
irb(main):017:0> redis.blpop 'hello', timeout: 2
=> nil

Thus, I think we should actually just not ignore timeouts on blocking commands at all; all timeout errors are bad timeout errors, and:

• People using redis-cluster-client directly should make sure they pass a higher socket timeout to blocking_call_v than redis-level timeout, or
• People using redis-clustering should have this done for them automagically like redis-rb does

WDYT? I'll open a PR which does this and you can merge that one & close this one if you agree.

[supercaracal]

It's exactly as you said. I think it would be better to delete the error handling. I'd like to merge the latter pull request.

[supercaracal]

• Delete special handling of blocking command timeouts #306