New error message handling is broken when using sentinels

[jlledom]

Release 0.21.0 introduced a new error message handling: https://github.com/redis-rb/redis-client/pull/178

However, this blocks the execution when trying to connect to an invalid sentinel client. Check this example:

# frozen_string_literal: true

require "bundler/inline"

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

  gem 'redis-client'#, '0.20.0'
end

def main
  redis_config = RedisClient.sentinel(
    name: "mymaster",
    sentinels: [
      { host: "127.0.0.1", port: 26380 },
      { host: "127.0.0.1", port: 26381 },
    ],
    role: :master
  )
  redis = redis_config.new_pool(timeout: 0.5, size: 5)

  while true
    begin
      sleep 1
      result = redis.call("PING") # => "PONG"
      puts result
    rescue StandardError => e
      puts e.message
      retry
    end
  end
end

main

When using 0.20.0. This is the result:

$ ruby redis-client-sentinels-inavlid.rb 
Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
Using bundler 2.2.25
Using connection_pool 2.4.1
Using redis-client 0.20.0
No sentinels available
No sentinels available
No sentinels available
No sentinels available
...

And this is the result using 0.21.0:

$ ruby redis-client-sentinels-inavlid.rb 
Fetching gem metadata from https://rubygems.org/..
Resolving dependencies...
Using bundler 2.2.25
Using connection_pool 2.4.1
Using redis-client 0.21.0
Traceback (most recent call last):

After taking a quick look at the code, I'd say this happens because the new HasConfig#message method calls to config.server_url:

https://github.com/redis-rb/redis-client/blob/93c343a52c452e916d6f10e1b403469cc4c72116/lib/redis_client.rb#L87-L91

Which calls to host:

https://github.com/redis-rb/redis-client/blob/93c343a52c452e916d6f10e1b403469cc4c72116/lib/redis_client/config.rb#L125-L138

Which, if I'm not wrong, ends up calling to resolve_master, and trying to connect again, thus raising the error again.

https://github.com/redis-rb/redis-client/blob/93c343a52c452e916d6f10e1b403469cc4c72116/lib/redis_client/sentinel_config.rb#L138-L151

[byroot]

FYI @stanhu

[stanhu]

Is the simplest solution here to catch ConnectionError in message and give up?

[stanhu]

Alternatively, we could use a different method or use an argument to config that avoids the call to resolve_master or resolve_replica.

[jlledom]

@stanhu IMO catching errors in message is the way to go. Because showing the URL in the message is not a vital feature. If we can have it, cool, but if something fails it's ok to just return super, not a big deal. I would catch any error, not only ConnectionError

[casperisfine]

I don't like the rescue idea, I think we should just not call anything that could fail from here.

So we probably need a version of server_url that just return something else if it wasn't resolved for a sentinel client.

[casperisfine]

@stanhu if you want to fix it it's all good, but if you don't have time let me know and I'll find the time.

[stanhu]

@casperisfine I'll fix this.

[stanhu]

So we probably need a version of server_url that just return something else if it wasn't resolved for a sentinel client.

The issue is that config calls resolve_master or resolve_replica, so we need some config object that doesn't attempt that.

[stanhu]

I think https://github.com/redis-rb/redis-client/pull/183 should solve this.

[stanhu]

It looks like this was fixed in v0.21.1. Thanks for the quick review and release!