Copilot commented on code in PR #3691:
URL: https://github.com/apache/brpc/pull/3691#discussion_r4425850870
##########
src/brpc/versioned_ref_with_id.h:
##########
@@ -628,18 +648,24 @@ int VersionedRefWithId<T>::Dereference() {
template<typename T>
void VersionedRefWithId<T>::Revive(int32_t at_least_nref) {
+ AdditionalRefStatus expect = ADDITIONAL_REF_RELEASED_ON_FAILURE;
+ if (!_additional_ref_status.compare_exchange_strong(
+ expect, ADDITIONAL_REF_REVIVING,
+ butil::memory_order_relaxed,
+ butil::memory_order_relaxed)) {
+ CHECK_EQ(ADDITIONAL_REF_RELEASED, expect);
Review Comment:
Revive() assumes the only possible CAS-failure state is
ADDITIONAL_REF_RELEASED, but compare_exchange_strong writes the observed
current value into `expect`. If Revive() can be called when the status is
ADDITIONAL_REF_USING (or still ADDITIONAL_REF_REVIVING due to a concurrent
reviver), this CHECK will crash the process. Consider handling
non-RELEASED_ON_FAILURE states as a no-op (e.g., return when expect is
ADDITIONAL_REF_RELEASED or ADDITIONAL_REF_USING, and optionally yield/retry
when expect is ADDITIONAL_REF_REVIVING), rather than CHECKing.
##########
test/brpc_channel_unittest.cpp:
##########
@@ -3977,6 +3996,31 @@ class CountingSubChannel : public brpc::Channel {
};
int CountingSubChannel::s_alive = 0;
+TEST_F(ChannelTest, selective_channel_removal_prevents_revival) {
+ brpc::SelectiveChannel channel;
+ ASSERT_EQ(0, channel.Init("rr", nullptr));
+ brpc::SelectiveChannel::ChannelHandle handle;
+ ASSERT_EQ(0, channel.AddChannel(new CountingSubChannel, &handle));
+ ASSERT_EQ(1, CountingSubChannel::s_alive);
+ brpc::SocketUniquePtr ptr;
+ ASSERT_EQ(0, brpc::Socket::Address(handle.id, &ptr));
+
+ // Keep the HC reference but disable automatic scheduling: the retained
+ // pointer below simulates the health check task resuming after removal.
+ ptr->_is_hc_related_ref_held = false;
+ EXPECT_EQ(0, ptr->SetFailed());
+ ptr->_is_hc_related_ref_held = true;
+ channel.RemoveAndDestroyChannel(handle);
+ EXPECT_FALSE(ptr->HCEnabled());
+ ptr->Revive(1);
+ EXPECT_TRUE(ptr->Failed());
+ EXPECT_EQ(1, ptr->nref());
+ // Also clean up the leaked additional reference if this test regresses.
Review Comment:
The test directly mutates an internal socket field
(`_is_hc_related_ref_held`) to simulate HC scheduling behavior. This tightly
couples the test to private/internal implementation details and makes future
refactors riskier. Prefer using a public test hook (e.g., a helper API to
pause/resume HC scheduling) or a fixture-level mechanism to disable scheduling
without reaching into internal fields.
##########
test/brpc_channel_unittest.cpp:
##########
@@ -2576,6 +2576,25 @@ TEST_F(ChannelTest, init_using_unknown_naming_service) {
ASSERT_EQ(-1, channel.Init("unknown://unknown", "unknown", nullptr));
}
+TEST_F(ChannelTest, selective_channel_invalid_load_balancer) {
+ GFLAGS_NAMESPACE::FlagSaver flag_saver;
+#if !BRPC_WITH_GLOG
+ // Invalid configuration must return an error even when fatal logs abort.
+ ASSERT_FALSE(GFLAGS_NAMESPACE::SetCommandLineOption(
+ "crash_on_fatal_log", "true").empty());
+#endif
Review Comment:
This test configures `crash_on_fatal_log` (and the comment references fatal
logs), but the PR also changes SharedLoadBalancer::Init() to LOG(ERROR) instead
of LOG(FATAL). That makes the comment and the flag toggling
misleading/redundant. Suggest updating the comment to reflect the new behavior
and removing the flag manipulation unless some remaining initialization path
still produces fatal logs that this test is intended to guard against.
--
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]
