From c0aaf32d5823ec9255366b56e2f95df86a8d65d4 Mon Sep 17 00:00:00 2001 From: wangchenguang Date: Tue, 18 Aug 2026 01:34:48 +0800 Subject: [PATCH] fix(channel): Reject reinitialization to keep SocketMap references balanced Reject re-initialization once Channel::Init() has succeeded. This ensures a Channel instance only inserts into SocketMap at most once and preserves its options and signature intact, guaranteeing that ~Channel() always balances the insertion without requiring extra state tracking. Failed inits before the first successful initialization can still be retried. --- src/brpc/channel.cpp | 16 +++ test/brpc_channel_unittest.cpp | 213 ++++++++++++++++++++++++--------- 2 files changed, 175 insertions(+), 54 deletions(-) diff --git a/src/brpc/channel.cpp b/src/brpc/channel.cpp index 83fc37b077..0a6e121b28 100644 --- a/src/brpc/channel.cpp +++ b/src/brpc/channel.cpp @@ -254,6 +254,10 @@ int Channel::InitChannelOptions(const ChannelOptions* options) { int Channel::Init(const char* server_addr_and_port, const ChannelOptions* options) { + if (_server_id != INVALID_SOCKET_ID || _lb != NULL) { + LOG(ERROR) << "Channel=" << this << " has already been initialized"; + return -1; + } GlobalInitializeOrDie(); butil::EndPoint point; const AdaptiveProtocolType& ptype = (options ? options->protocol : _options.protocol); @@ -287,6 +291,10 @@ int Channel::Init(const char* server_addr_and_port, int Channel::Init(const char* server_addr, int port, const ChannelOptions* options) { + if (_server_id != INVALID_SOCKET_ID || _lb != NULL) { + LOG(ERROR) << "Channel=" << this << " has already been initialized"; + return -1; + } GlobalInitializeOrDie(); butil::EndPoint point; const AdaptiveProtocolType& ptype = (options ? options->protocol : _options.protocol); @@ -358,6 +366,10 @@ int Channel::InitSingle(const butil::EndPoint& server_addr_and_port, const char* raw_server_address, const ChannelOptions* options, int raw_port) { + if (_server_id != INVALID_SOCKET_ID || _lb != NULL) { + LOG(ERROR) << "Channel=" << this << " has already been initialized"; + return -1; + } GlobalInitializeOrDie(); if (InitChannelOptions(options) != 0) { return -1; @@ -410,6 +422,10 @@ int Channel::Init(const char* ns_url, // Treat ns_url as server_addr_and_port return Init(ns_url, options); } + if (_server_id != INVALID_SOCKET_ID || _lb != NULL) { + LOG(ERROR) << "Channel=" << this << " has already been initialized"; + return -1; + } GlobalInitializeOrDie(); if (InitChannelOptions(options) != 0) { return -1; diff --git a/test/brpc_channel_unittest.cpp b/test/brpc_channel_unittest.cpp index 6f4540d6a2..73b04887d1 100644 --- a/test/brpc_channel_unittest.cpp +++ b/test/brpc_channel_unittest.cpp @@ -2316,6 +2316,46 @@ TEST_F(ChannelTest, init_as_single_server) { } } +TEST_F(ChannelTest, reject_reinitialization_after_successful_init) { + butil::EndPoint first_endpoint; + butil::EndPoint second_endpoint; + ASSERT_EQ(0, str2endpoint("127.0.0.1:59347", &first_endpoint)); + ASSERT_EQ(0, str2endpoint("127.0.0.1:59348", &second_endpoint)); + + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init(first_endpoint, NULL)); + ASSERT_EQ(-1, channel.Init(first_endpoint, NULL)); + ASSERT_EQ(-1, channel.Init(second_endpoint, NULL)); + ASSERT_EQ(-1, channel.Init("unknown://unknown", "rr", NULL)); + } + + brpc::SocketId id; + EXPECT_NE(0, brpc::SocketMapFind(brpc::SocketMapKey(first_endpoint), &id)); + EXPECT_NE(0, brpc::SocketMapFind(brpc::SocketMapKey(second_endpoint), &id)); +} + +TEST_F(ChannelTest, retry_init_after_failed_init) { + butil::EndPoint endpoint; + ASSERT_EQ(0, str2endpoint("127.0.0.1:59349", &endpoint)); + + { + brpc::Channel channel; + brpc::ChannelOptions invalid_options; + invalid_options.client_host = "not a valid client host"; + ASSERT_EQ(-1, channel.Init(endpoint, &invalid_options)); + EXPECT_EQ(brpc::INVALID_SOCKET_ID, channel._server_id); + + brpc::ChannelOptions valid_options; + ASSERT_EQ(0, channel.Init(endpoint, &valid_options)); + EXPECT_NE(brpc::INVALID_SOCKET_ID, channel._server_id); + EXPECT_EQ(endpoint, channel._server_address); + } + + brpc::SocketId id; + EXPECT_NE(0, brpc::SocketMapFind(brpc::SocketMapKey(endpoint), &id)); +} + TEST_F(ChannelTest, init_using_unknown_naming_service) { brpc::Channel channel; ASSERT_EQ(-1, channel.Init("unknown://unknown", "unknown", NULL)); @@ -2391,73 +2431,138 @@ TEST_F(ChannelTest, parse_hostname) { brpc::ChannelOptions opt; opt.succeed_without_server = false; opt.protocol = brpc::PROTOCOL_HTTP; - brpc::Channel channel; - ASSERT_EQ(-1, channel.Init("", 8888, &opt)); - ASSERT_EQ("", channel._service_name); - ASSERT_EQ(-1, channel.Init("", &opt)); - ASSERT_EQ("", channel._service_name); - - ASSERT_EQ(0, channel.Init("http://127.0.0.1", 8888, &opt)); - ASSERT_EQ("127.0.0.1:8888", channel._service_name); - ASSERT_EQ(0, channel.Init("http://127.0.0.1:8888", &opt)); - ASSERT_EQ("127.0.0.1:8888", channel._service_name); - - ASSERT_EQ(0, channel.Init("localhost", 8888, &opt)); - ASSERT_EQ("localhost:8888", channel._service_name); - ASSERT_EQ(0, channel.Init("localhost:8888", &opt)); - ASSERT_EQ("localhost:8888", channel._service_name); - - ASSERT_EQ(0, channel.Init("http://www.baidu.com", &opt)); - ASSERT_EQ("www.baidu.com", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", &opt)); - ASSERT_EQ("www.baidu.com:80", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com", 80, &opt)); - ASSERT_EQ("www.baidu.com:80", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", &opt)); - ASSERT_EQ("www.baidu.com:8888", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com", 8888, &opt)); - ASSERT_EQ("www.baidu.com:8888", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com", "rr", &opt)); - ASSERT_EQ("www.baidu.com", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", "rr", &opt)); - ASSERT_EQ("www.baidu.com:80", channel._service_name); - ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", "rr", &opt)); - ASSERT_EQ("www.baidu.com:8888", channel._service_name); + { + brpc::Channel channel; + ASSERT_EQ(-1, channel.Init("", 8888, &opt)); + ASSERT_EQ("", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(-1, channel.Init("", &opt)); + ASSERT_EQ("", channel._service_name); + } + + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://127.0.0.1", 8888, &opt)); + ASSERT_EQ("127.0.0.1:8888", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://127.0.0.1:8888", &opt)); + ASSERT_EQ("127.0.0.1:8888", channel._service_name); + } + + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("localhost", 8888, &opt)); + ASSERT_EQ("localhost:8888", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("localhost:8888", &opt)); + ASSERT_EQ("localhost:8888", channel._service_name); + } + + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com", &opt)); + ASSERT_EQ("www.baidu.com", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", &opt)); + ASSERT_EQ("www.baidu.com:80", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com", 80, &opt)); + ASSERT_EQ("www.baidu.com:80", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", &opt)); + ASSERT_EQ("www.baidu.com:8888", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com", 8888, &opt)); + ASSERT_EQ("www.baidu.com:8888", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com", "rr", &opt)); + ASSERT_EQ("www.baidu.com", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com:80", "rr", &opt)); + ASSERT_EQ("www.baidu.com:80", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("http://www.baidu.com:8888", "rr", &opt)); + ASSERT_EQ("www.baidu.com:8888", channel._service_name); + } opt.mutable_ssl_options()->verify.verify_mode = brpc::VerifyMode::VERIFY_PEER; opt.mutable_ssl_options()->verify.verify_depth = 1; opt.mutable_ssl_options()->verify.ca_file_path = "cert1.crt"; - ASSERT_EQ(0, channel.Init("https://www.baidu.com", &opt)); - ASSERT_EQ("www.baidu.com", channel._service_name); + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com", &opt)); + ASSERT_EQ("www.baidu.com", channel._service_name); #if defined(USE_MESALINK) || \ (!defined(OPENSSL_IS_BORINGSSL) && OPENSSL_VERSION_NUMBER < 0x10002000L) - ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty()); + ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty()); #else - ASSERT_EQ("www.baidu.com", - channel._options.ssl_options().verify.expected_peer_name); + ASSERT_EQ("www.baidu.com", + channel._options.ssl_options().verify.expected_peer_name); #endif - ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", &opt)); - ASSERT_EQ("www.baidu.com:443", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", &opt)); + ASSERT_EQ("www.baidu.com:443", channel._service_name); #if defined(USE_MESALINK) || \ (!defined(OPENSSL_IS_BORINGSSL) && OPENSSL_VERSION_NUMBER < 0x10002000L) - ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty()); + ASSERT_TRUE(channel._options.ssl_options().verify.expected_peer_name.empty()); #else - ASSERT_EQ("www.baidu.com", - channel._options.ssl_options().verify.expected_peer_name); + ASSERT_EQ("www.baidu.com", + channel._options.ssl_options().verify.expected_peer_name); #endif - ASSERT_EQ(0, channel.Init("https://www.baidu.com", 443, &opt)); - ASSERT_EQ("www.baidu.com:443", channel._service_name); - ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", &opt)); - ASSERT_EQ("www.baidu.com:1443", channel._service_name); - ASSERT_EQ(0, channel.Init("https://www.baidu.com", 1443, &opt)); - ASSERT_EQ("www.baidu.com:1443", channel._service_name); - ASSERT_EQ(0, channel.Init("https://www.baidu.com", "rr", &opt)); - ASSERT_EQ("www.baidu.com", channel._service_name); - ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", "rr", &opt)); - ASSERT_EQ("www.baidu.com:443", channel._service_name); - ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", "rr", &opt)); - ASSERT_EQ("www.baidu.com:1443", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com", 443, &opt)); + ASSERT_EQ("www.baidu.com:443", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", &opt)); + ASSERT_EQ("www.baidu.com:1443", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com", 1443, &opt)); + ASSERT_EQ("www.baidu.com:1443", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com", "rr", &opt)); + ASSERT_EQ("www.baidu.com", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com:443", "rr", &opt)); + ASSERT_EQ("www.baidu.com:443", channel._service_name); + } + { + brpc::Channel channel; + ASSERT_EQ(0, channel.Init("https://www.baidu.com:1443", "rr", &opt)); + ASSERT_EQ("www.baidu.com:1443", channel._service_name); + } const char *address_list[] = { "10.127.0.1:1234",