Copilot commented on code in PR #3433:
URL: https://github.com/apache/brpc/pull/3433#discussion_r3850092841
##########
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));
+ }
Review Comment:
This test now asserts that reinitialization after a successful Init() must
fail (-1). That expectation conflicts with the PR description’s claim that
reinitialization remains supported and should correctly replace the
direct-server state; please align the test expectation with the intended API
contract (either implement safe reinit, or update the PR description/docs to
state Init is single-shot).
##########
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;
+ }
Review Comment:
This adds a hard guard that rejects any subsequent Channel::Init() once the
channel has been initialized. That contradicts the PR description (which says
reinitialization remains supported and should replace direct-server state) and
also changes previously tested behavior (parse_hostname previously re-used a
Channel for multiple Init calls). If reinitialization is still intended to be
supported, the fix likely needs to safely swap state (and release the old
SocketMapKey) rather than returning -1 here; otherwise the PR description/docs
should be updated to reflect the breaking behavior change.
##########
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));
Review Comment:
These SocketMap assertions can become vacuous/false-positive because they
never verify that SocketMapFind(SocketMapKey(first_endpoint)) succeeds while
the channel is alive. If the effective SocketMapKey includes a non-zero
ChannelSignature (e.g., due to non-default gflags/options), this lookup would
fail even if the socket entry leaked. Consider asserting SocketMapFind succeeds
(and matches channel._server_id) immediately after the successful Init, before
the channel goes out of scope.
This issue also appears on line 2355 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
