From b4fe02fb30bae86b62e32bc816803a72259cf0d4 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Sat, 30 Sep 2023 01:27:05 +0200 Subject: [PATCH 1/3] Fixed issue #335 by making at least one connection attempt --- clickhouse/client.cpp | 5 ++- clickhouse/columns/string.cpp | 2 +- ut/client_ut.cpp | 84 ++++++++++++++++++++++++++++++++--- ut/roundtrip_column.h | 7 +++ 4 files changed, 89 insertions(+), 9 deletions(-) diff --git a/clickhouse/client.cpp b/clickhouse/client.cpp index ca57190b..c9a3c134 100644 --- a/clickhouse/client.cpp +++ b/clickhouse/client.cpp @@ -385,14 +385,15 @@ void Client::Impl::ResetConnectionEndpoint() { } void Client::Impl::CreateConnection() { - for (size_t i = 0; i < options_.send_retries;) + // i <= 1 to try at least once + for (size_t i = 0; i <= 1 || i < options_.send_retries;) { try { ResetConnectionEndpoint(); return; } catch (const std::system_error&) { - if (++i == options_.send_retries) + if (++i >= options_.send_retries) { throw; } diff --git a/clickhouse/columns/string.cpp b/clickhouse/columns/string.cpp index 173c024f..62ec464b 100644 --- a/clickhouse/columns/string.cpp +++ b/clickhouse/columns/string.cpp @@ -173,7 +173,7 @@ ColumnString::ColumnString(const std::vector& data) for (const auto & s : data) { AppendUnsafe(s); } -}; +} ColumnString::ColumnString(std::vector&& data) : ColumnString() diff --git a/ut/client_ut.cpp b/ut/client_ut.cpp index d894cc36..03236f27 100644 --- a/ut/client_ut.cpp +++ b/ut/client_ut.cpp @@ -1,17 +1,36 @@ #include +#include "clickhouse/base/socket.h" #include "readonly_client_test.h" #include "connection_failed_client_test.h" +#include "ut/utils_comparison.h" #include "utils.h" +#include "ut/roundtrip_column.h" +#include "ut/value_generators.h" #include +#include #include +#include #include #include using namespace clickhouse; + +template +std::shared_ptr createTableWithOneColumn(Client & client, const std::string & table_name, const std::string & column_name) +{ + auto col = std::make_shared(); + const auto type_name = col->GetType().GetName(); + + client.Execute("DROP TEMPORARY TABLE IF EXISTS " + table_name + ";"); + client.Execute("CREATE TEMPORARY TABLE IF NOT EXISTS " + table_name + "( " + column_name + " " + type_name + " )"); + + return col; +} + // Use value-parameterized tests to run same tests with different client // options. class ClientCase : public testing::TestWithParam { @@ -27,13 +46,9 @@ class ClientCase : public testing::TestWithParam { template std::shared_ptr createTableWithOneColumn(Block & block) { - auto col = std::make_shared(); - const auto type_name = col->GetType().GetName(); - - client_->Execute("DROP TEMPORARY TABLE IF EXISTS " + table_name + ";"); - client_->Execute("CREATE TEMPORARY TABLE IF NOT EXISTS " + table_name + "( " + column_name + " " + type_name + " )"); + auto col = ::createTableWithOneColumn(*client_, table_name, column_name); - block.AppendColumn("test_column", col); + block.AppendColumn(column_name, col); return col; } @@ -1352,3 +1367,60 @@ INSTANTIATE_TEST_SUITE_P(ResetConnectionClientTest, ResetConnectionTestCase, .SetRetryTimeout(std::chrono::seconds(1)) } )); + +struct CountingSocketFactoryAdapter : public SocketFactory { + struct Counters + { + size_t connect_count = 0; + + void Reset() { + *this = Counters(); + } + }; + + SocketFactory & socket_factory; + Counters& counters; + + CountingSocketFactoryAdapter(SocketFactory & socket_factory, Counters& counters) + : socket_factory(socket_factory) + , counters(counters) + {} + + std::unique_ptr connect(const ClientOptions& opts, const Endpoint& endpoint) { + ++counters.connect_count; + return socket_factory.connect(opts, endpoint); + } + + void sleepFor(const std::chrono::milliseconds& duration) { + return socket_factory.sleepFor(duration); + } + +}; + +TEST(SimpleClientTest, issue_335) { + auto vals = MakeStrings(); + auto col = std::make_shared(vals); + + CountingSocketFactoryAdapter::Counters counters; + std::unique_ptr wrapped_socket_factory = std::make_unique(); + std::unique_ptr socket_factory = std::make_unique(*wrapped_socket_factory, counters); + + Client client(ClientOptions(LocalHostEndpoint) + .SetSendRetries(0), // <<=== crucial for reproducing https://github.com/ClickHouse/clickhouse-cpp/issues/335 + std::move(socket_factory)); + + EXPECT_EQ(1u, counters.connect_count); + EXPECT_TRUE(CompareRecursive(vals, *RoundtripColumnValuesTyped(client, col))); + + counters.Reset(); + + client.ResetConnection(); + EXPECT_EQ(1u, counters.connect_count); + EXPECT_TRUE(CompareRecursive(vals, *RoundtripColumnValuesTyped(client, col))); + + counters.Reset(); + + client.ResetConnectionEndpoint(); + EXPECT_EQ(1u, counters.connect_count); + EXPECT_TRUE(CompareRecursive(vals, *RoundtripColumnValuesTyped(client, col))); +} diff --git a/ut/roundtrip_column.h b/ut/roundtrip_column.h index 30097997..6204d8ad 100644 --- a/ut/roundtrip_column.h +++ b/ut/roundtrip_column.h @@ -1,9 +1,16 @@ #pragma once #include +#include namespace clickhouse { class Client; } clickhouse::ColumnRef RoundtripColumnValues(clickhouse::Client& client, clickhouse::ColumnRef expected); + +template +auto RoundtripColumnValuesTyped(clickhouse::Client& client, std::shared_ptr expected_col) +{ + return RoundtripColumnValues(client, expected_col)->template As(); +} From ece5e9b4dfc2f9b8d971190b6a8ed8930461b223 Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Sat, 30 Sep 2023 02:14:35 +0200 Subject: [PATCH 2/3] Better fix + updated tests --- clickhouse/client.cpp | 8 +++--- ut/client_ut.cpp | 57 +++++++++++++++++++++++++++++-------------- 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/clickhouse/client.cpp b/clickhouse/client.cpp index c9a3c134..6f7ea213 100644 --- a/clickhouse/client.cpp +++ b/clickhouse/client.cpp @@ -385,15 +385,17 @@ void Client::Impl::ResetConnectionEndpoint() { } void Client::Impl::CreateConnection() { - // i <= 1 to try at least once - for (size_t i = 0; i <= 1 || i < options_.send_retries;) + // make sure to try to connect to each endpoint at least once even if `options_.send_retries` is 0 + const size_t max_attempts = (options_.send_retries ? options_.send_retries : 1); + for (size_t i = 0; i < max_attempts;) { try { + // Try to connect to each endpoint before throwing exception. ResetConnectionEndpoint(); return; } catch (const std::system_error&) { - if (++i >= options_.send_retries) + if (++i >= max_attempts) { throw; } diff --git a/ut/client_ut.cpp b/ut/client_ut.cpp index 03236f27..f0b3a108 100644 --- a/ut/client_ut.cpp +++ b/ut/client_ut.cpp @@ -1369,25 +1369,20 @@ INSTANTIATE_TEST_SUITE_P(ResetConnectionClientTest, ResetConnectionTestCase, )); struct CountingSocketFactoryAdapter : public SocketFactory { - struct Counters - { - size_t connect_count = 0; - void Reset() { - *this = Counters(); - } - }; + using ConnectRequests = std::vector>; SocketFactory & socket_factory; - Counters& counters; + ConnectRequests & connect_requests; - CountingSocketFactoryAdapter(SocketFactory & socket_factory, Counters& counters) + CountingSocketFactoryAdapter(SocketFactory & socket_factory, ConnectRequests& connect_requests) : socket_factory(socket_factory) - , counters(counters) + , connect_requests(connect_requests) {} std::unique_ptr connect(const ClientOptions& opts, const Endpoint& endpoint) { - ++counters.connect_count; + connect_requests.emplace_back(opts, endpoint); + return socket_factory.connect(opts, endpoint); } @@ -1395,32 +1390,58 @@ struct CountingSocketFactoryAdapter : public SocketFactory { return socket_factory.sleepFor(duration); } + size_t GetConnectRequestsCount() const { + return connect_requests.size(); + } + }; TEST(SimpleClientTest, issue_335) { + // Make sure Client connects to server even with ClientOptions.SetSendRetries(0) auto vals = MakeStrings(); auto col = std::make_shared(vals); - CountingSocketFactoryAdapter::Counters counters; + CountingSocketFactoryAdapter::ConnectRequests connect_requests; std::unique_ptr wrapped_socket_factory = std::make_unique(); - std::unique_ptr socket_factory = std::make_unique(*wrapped_socket_factory, counters); + std::unique_ptr socket_factory = std::make_unique(*wrapped_socket_factory, connect_requests); Client client(ClientOptions(LocalHostEndpoint) .SetSendRetries(0), // <<=== crucial for reproducing https://github.com/ClickHouse/clickhouse-cpp/issues/335 std::move(socket_factory)); - EXPECT_EQ(1u, counters.connect_count); + EXPECT_EQ(1u, connect_requests.size()); EXPECT_TRUE(CompareRecursive(vals, *RoundtripColumnValuesTyped(client, col))); - counters.Reset(); + connect_requests.clear(); client.ResetConnection(); - EXPECT_EQ(1u, counters.connect_count); + EXPECT_EQ(1u, connect_requests.size()); EXPECT_TRUE(CompareRecursive(vals, *RoundtripColumnValuesTyped(client, col))); - counters.Reset(); + connect_requests.clear(); client.ResetConnectionEndpoint(); - EXPECT_EQ(1u, counters.connect_count); + EXPECT_EQ(1u, connect_requests.size()); EXPECT_TRUE(CompareRecursive(vals, *RoundtripColumnValuesTyped(client, col))); } + +TEST(SimpleClientTest, issue_335_reconnects_count) { + // Make sure that client attempts to connect to each endpoint at least once. + CountingSocketFactoryAdapter::ConnectRequests connect_requests; + std::unique_ptr wrapped_socket_factory = std::make_unique(); + std::unique_ptr socket_factory = std::make_unique(*wrapped_socket_factory, connect_requests); + + const auto endpoints = { + Endpoint{"foo-invalid-hostname", 1234}, + Endpoint{"bar-invalid-hostname", 4567}, + }; + + EXPECT_ANY_THROW( + Client(ClientOptions() + .SetEndpoints(endpoints) + .SetSendRetries(0), // <<=== crucial for reproducing https://github.com/ClickHouse/clickhouse-cpp/issues/335 + std::move(socket_factory)); + ); + + EXPECT_EQ(endpoints.size(), connect_requests.size()); +} From dc76dac084027dfebe3a66cded00db44b02d5cdd Mon Sep 17 00:00:00 2001 From: Vasily Nemkov Date: Sat, 30 Sep 2023 02:30:08 +0200 Subject: [PATCH 3/3] Fixed build for older compilers + more detailed tests --- ut/client_ut.cpp | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/ut/client_ut.cpp b/ut/client_ut.cpp index f0b3a108..a9dd8190 100644 --- a/ut/client_ut.cpp +++ b/ut/client_ut.cpp @@ -12,12 +12,18 @@ #include #include +#include #include #include #include using namespace clickhouse; +namespace clickhouse { +std::ostream & operator << (std::ostream & ostr, const Endpoint & endpoint) { + return ostr << endpoint.host << ":" << endpoint.port; +} +} template std::shared_ptr createTableWithOneColumn(Client & client, const std::string & table_name, const std::string & column_name) @@ -1431,7 +1437,7 @@ TEST(SimpleClientTest, issue_335_reconnects_count) { std::unique_ptr wrapped_socket_factory = std::make_unique(); std::unique_ptr socket_factory = std::make_unique(*wrapped_socket_factory, connect_requests); - const auto endpoints = { + const std::vector endpoints = { Endpoint{"foo-invalid-hostname", 1234}, Endpoint{"bar-invalid-hostname", 4567}, }; @@ -1444,4 +1450,14 @@ TEST(SimpleClientTest, issue_335_reconnects_count) { ); EXPECT_EQ(endpoints.size(), connect_requests.size()); + // make sure there was an attempt to connect to each endpoint at least once. + for (const auto & endpoint : endpoints) + { + auto p = std::find_if(connect_requests.begin(), connect_requests.end(), [&endpoint](const auto & connect_request) { + return connect_request.second == endpoint; + }); + + EXPECT_TRUE(connect_requests.end() != p) + << "\tThere was no attempt to connect to endpoint " << endpoint; + } }