Skip to content
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

:blpop returns an exception when using sentinels #1279

Closed
jlledom opened this issue May 14, 2024 · 3 comments · Fixed by redis-rb/redis-client#197
Closed

:blpop returns an exception when using sentinels #1279

jlledom opened this issue May 14, 2024 · 3 comments · Fixed by redis-rb/redis-client#197

Comments

@jlledom
Copy link

jlledom commented May 14, 2024

The documentation says :blpop should return nil when timed out. However when connecting to sentinels, timing out raises a RedisClient::ReadTimeoutError.

I wrote this script to reproduce it:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem 'redis'
  gem 'hiredis-client'
end

config = {
  url: 'redis://redis-master',
  name: 'redis-master',
  sentinels: [
    { host: 'localhost', port: 26379},
    { host: 'localhost', port: 26380},
    { host: 'localhost', port: 26381}
  ],
}
client = Redis.new(config)

while true
  begin
    sleep 1
    result = client.blpop('foo', timeout: 1)
    puts result || 'No result'
  rescue RedisClient::ReadTimeoutError => e
    puts e.message
    retry
  end
end

The problem comes from the hiredis-client gem which actually raises an exception for sentinels but returns nil for single redis instances. So this is probably affecting other blocking commands.

@jlledom jlledom changed the title :blop returns an exception when using sentinels :blpop returns an exception when using sentinels May 14, 2024
@stanhu
Copy link

stanhu commented May 15, 2024

How timely. I just ran into this issue as well, but I'm not using the hiredis-client gem:

/Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/buffered_io.rb:214:in `block in fill_buffer': Waited 2 seconds (RedisClient::ReadTimeoutError)
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/buffered_io.rb:197:in `loop'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/buffered_io.rb:197:in `fill_buffer'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/buffered_io.rb:187:in `ensure_remaining'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/buffered_io.rb:152:in `getbyte'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/resp3.rb:113:in `parse'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/resp3.rb:50:in `load'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection.rb:98:in `block in read'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection/buffered_io.rb:114:in `with_timeout'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/ruby_connection.rb:98:in `read'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/connection_mixin.rb:31:in `call'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client.rb:360:in `block (2 levels) in blocking_call_v'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client/middlewares.rb:16:in `call'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client.rb:359:in `block in blocking_call_v'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client.rb:699:in `ensure_connected'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-client-0.22.1/lib/redis_client.rb:358:in `blocking_call_v'
        from /Users/stanhu/.asdf/installs/ruby/3.2.3/lib/ruby/gems/3.2.0/gems/redis-5.2.0/lib/redis.rb:160:in `block in send_blocking_command'

@stanhu
Copy link

stanhu commented May 15, 2024

Notice how _client is different depending on whether Sentinels are in use:

irb(main):001> require 'redis'
=> true
irb(main):002> redis_sentinel = Redis.new(url: 'redis://mymaster', name: 'mymaster', sentinels: [ { host: 'localhost', port: 26379 }])
=> #<Redis client v5.2.0 for redis://127.0.0.1:6381>
irb(main):003> redis_sentinel._client
=> #<RedisClient redis://127.0.0.1:6381>
irb(main):004> redis = Redis.new(url: 'redis://localhost:6381')
=> #<Redis client v5.2.0 for redis://localhost:6381>
irb(main):005> redis._client
=> #<Redis::Client redis://localhost:6381>

In the Redis Sentinel case, RedisClient from the redis-client library is used, but in the standalone Redis case, Redis::Client is used:

def config(**kwargs)
super(protocol: 2, **kwargs)
end
def sentinel(**kwargs)
super(protocol: 2, **kwargs, client_implementation: ::RedisClient)
end

The latter increments the timeout in Redis::Client#blocking_call_v:

def blocking_call_v(timeout, command, &block)
if 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

However, this doesn't happen in RedisClient#blocking_call_v: https://github.com/redis-rb/redis-client/blob/a2f16fc7dbdaff3f8d3d396e8ec7c3826d2f9fcc/lib/redis_client.rb#L355-L364

I've validated that when I added a similar timeout mechanism to RedisClient, the issue appears to go away.

@byroot What do you think? Should _client be consistent?

stanhu added a commit to stanhu/redis-client that referenced this issue May 15, 2024
In redis/redis-rb#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 (redis/redis-rb#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. To avoid code duplication, turn
`CommandBuilder` into a class that holds the `read_timeout` and use
to determine the socket timeout.

Closes redis/redis-rb#1279
stanhu added a commit to stanhu/redis-client that referenced this issue May 15, 2024
In redis/redis-rb#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 (redis/redis-rb#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. To avoid code duplication, turn
`CommandBuilder` into a class that holds the `read_timeout` and use it
to determine the socket timeout.

Closes redis/redis-rb#1279
stanhu added a commit to stanhu/redis-client that referenced this issue May 15, 2024
In redis/redis-rb#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 (redis/redis-rb#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. To avoid code duplication, turn
`CommandBuilder` into a class that holds the `read_timeout` and use it
to determine the socket timeout.

Closes redis/redis-rb#1279
stanhu added a commit to stanhu/redis-client that referenced this issue May 15, 2024
In redis/redis-rb#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 (redis/redis-rb#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 redis/redis-rb#1279
stanhu added a commit to stanhu/redis-client that referenced this issue May 15, 2024
In redis/redis-rb#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 (redis/redis-rb#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 redis/redis-rb#1279
stanhu added a commit to stanhu/redis-client that referenced this issue May 15, 2024
In redis/redis-rb#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 (redis/redis-rb#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 redis/redis-rb#1279
@stanhu
Copy link

stanhu commented May 15, 2024

redis-rb/redis-client#197 should fix this.

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 a pull request may close this issue.

2 participants