New error message handling is broken when using sentinels

[jlledom]

Release 0.21.0 introduced a new error message handling: #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:

redis-client/lib/redis_client.rb

Lines 87 to 91 in 93c343a

	def message
	return super unless config
	"#{super} (#{config.server_url})"
	end

Which calls to host:

redis-client/lib/redis_client/config.rb

Lines 125 to 138 in 93c343a

	def server_url
	if path
	url = "unix://#{path}"
	if db != 0
	url = "#{url}?db=#{db}"
	end
	else
	url = "redis#{'s' if ssl?}://#{host}:#{port}"
	if db != 0
	url = "#{url}/#{db}"
	end
	end
	url
	end

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

redis-client/lib/redis_client/sentinel_config.rb

Lines 138 to 151 in 93c343a

	def resolve_master
	each_sentinel do |sentinel_client|
	host, port = sentinel_client.call("SENTINEL", "get-master-addr-by-name", @name)
	next unless host && port
	refresh_sentinels(sentinel_client)
	return Config.new(host: host, port: Integer(port), **@client_config)
	end
	rescue ConnectionError
	raise ConnectionError, "No sentinels available"
	else
	raise ConnectionError, "Couldn't locate a replica for role: #{@name}"
	end

[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 #183 should solve this.

[stanhu]

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