Skip to content

Ensure BLPOP/BRPOP returns nil instead of raising ReadTimeoutError

Stan Hu requested to merge sh-patch-redis-client into master

What does this MR do and why?

In https://github.com/redis/redis-rb/issues/1279, we discovered that when using BLPOP or BRPOP 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.

This commit includes the patch in https://github.com/redis-rb/redis-client/pull/197 to fix this issue until an official redis-client patch release can be made.

Relates to https://gitlab.com/gitlab-com/dev-sub-department/section-dev-request-for-help/-/issues/250

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

  1. Setup a Redis Sentinel and Redis server with these config files:

sentinel.conf

port 26379
dir "/tmp"
sentinel monitor mymaster 127.0.0.1 6381 2
sentinel down-after-milliseconds mymaster 5000

redis.conf

port 6381
  1. Run these servers:
redis-sentinel sentinel.conf & 
redis-server redis.conf &
  1. In master, attempt to run RAILS_ENV=test bin/rails console. You should see a ReadTimeoutError:
[1] pry(main)> Gitlab::Redis::SharedState.with { |redis| redis.blpop('key-does-not-exist', timeout: 5) }
RedisClient::ReadTimeoutError: Waited 5 seconds
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:214:in `block in fill_buffer'
  1. In your config/redis.yml, add this to the test section:
test:
  shared_state:
    url: 'redis://mymaster'
    name: 'mymaster'
    sentinels:
      - host: 'localhost'
        port: 26379
  1. Repeat step 3. You should now see a nil response:
[1] pry(main)> Gitlab::Redis::SharedState.with { |redis| redis.blpop('key-does-not-exist', timeout: 5) }
=> nil
Edited by Stan Hu

Merge request reports