From ea24b5bf5e47ca92708c45da54e8b775fd319e64 Mon Sep 17 00:00:00 2001 From: Peter Raboud Date: Wed, 8 Sep 2021 23:07:33 -0400 Subject: [PATCH] Ensure command details are populated for Cluster clients Previously, if a clustered client is unable to query any of the nodes for command details during setup, it'd initialize successfully, but without any command details. (This is a bit of a corner case - in order to hit this case, at least one node needs to be reachable when querying nodes + slots, but all nodes would need to be unreachable immediately afterwards when querying command details.) In this case, the client would be unable to determine which node a given command should be routed to. Because the client falls back to routing commands to a random node, and following redirects as necessary, this wouldn't cause any overt problems, but leaves the client following unnecessary redirects for the rest of the lifespan of the client. This patch changes the client to require that the initial fetch of command details succeeds (as is already done with slot and node detail fetching), and raises a CannotConnectError if that's not possible. Users should already handle this error, and retry creating the client later, or use some other suitable failure handling mechanism. --- lib/redis/cluster/command_loader.rb | 13 ++++++------- test/cluster_client_key_hash_tags_test.rb | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/lib/redis/cluster/command_loader.rb b/lib/redis/cluster/command_loader.rb index 574212d35..99673b6fb 100644 --- a/lib/redis/cluster/command_loader.rb +++ b/lib/redis/cluster/command_loader.rb @@ -10,22 +10,21 @@ module CommandLoader module_function def load(nodes) - details = {} - nodes.each do |node| - details = fetch_command_details(node) - details.empty? ? next : break + begin + return fetch_command_details(node) + rescue CannotConnectError, ConnectionError, CommandError + next # can retry on another node + end end - details + raise CannotConnectError, 'Redis client could not connect to any cluster nodes' end def fetch_command_details(node) node.call(%i[command]).map do |reply| [reply[0], { arity: reply[1], flags: reply[2], first: reply[3], last: reply[4], step: reply[5] }] end.to_h - rescue CannotConnectError, ConnectionError, CommandError - {} # can retry on another node end private_class_method :fetch_command_details diff --git a/test/cluster_client_key_hash_tags_test.rb b/test/cluster_client_key_hash_tags_test.rb index 8ff4d301a..81e87165f 100644 --- a/test/cluster_client_key_hash_tags_test.rb +++ b/test/cluster_client_key_hash_tags_test.rb @@ -6,8 +6,8 @@ class TestClusterClientKeyHashTags < Minitest::Test include Helper::Cluster - def build_described_class - option = Redis::Cluster::Option.new(cluster: ['redis://127.0.0.1:7000']) + def build_described_class(urls = ['redis://127.0.0.1:7000']) + option = Redis::Cluster::Option.new(cluster: urls) node = Redis::Cluster::Node.new(option.per_node_key) details = Redis::Cluster::CommandLoader.load(node) Redis::Cluster::Command.new(details) @@ -85,4 +85,15 @@ def test_whether_the_command_effect_is_readonly_or_not assert_equal false, described_class.should_send_to_slave?([:info]) end end + + def test_cannot_build_details_from_bad_urls + assert_raises(Redis::CannotConnectError) do + build_described_class(['redis://127.0.0.1:7006']) + end + end + + def test_builds_details_from_a_mix_of_good_and_bad_urls + described_class = build_described_class(['redis://127.0.0.1:7006', 'redis://127.0.0.1:7000']) + assert_equal 'dogs:1', described_class.extract_first_key(%w[get dogs:1]) + end end