Skip to content

Delete special handling of blocking command timeouts #306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

KJTsanaktsidis
Copy link
Contributor

@KJTsanaktsidis KJTsanaktsidis commented Dec 14, 2023

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

Also, I had to delete the circuit breaker tests. In reality, this is totally broken, since we respond to literally any connection error by entirely dumping all Redis connections and starting from scratch. The new connections won't have their circuit breaker counts carried over. I think we might need to make cluster-client maintain it's own circuit breakers per-node-key; this is something I can look at in the future perhaps.

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

Also, I had to delete the circuit breaker tests. In reality, this is
totally broken, since we respond to _literally any_ connection error by
entirely dumping all Redis connections and starting from scratch. The
new connections won't have their circuit breaker counts carried over.
@supercaracal
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants