two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4237046253
##########
test/brpc_channel_unittest.cpp:
##########
@@ -3740,4 +3740,97 @@ TEST_F(RateLimitedBackupPolicyTest,
OnRPCEndDrivesRatioDownAndReAllows) {
ASSERT_TRUE(p->DoBackup(nullptr));
}
+// Issue #1408: SelectiveChannel::AddChannel must take over (or not take over)
+// the sub channel consistently w.r.t. option.ownership, even on failure, so
+// that callers don't leak or double-free.
+class CountingSubChannel : public brpc::Channel {
+public:
+ static int s_alive;
+ CountingSubChannel() { ++s_alive; }
+ ~CountingSubChannel() override { --s_alive; }
+};
+int CountingSubChannel::s_alive = 0;
+
+TEST_F(ChannelTest, selective_channel_add_failure_ownership) {
Review Comment:
Agreed the gap is real. These branches (Socket::Create failure, address < 0,
HC disabled, AddServer duplicate) depend on Socket internals and are
effectively unreachable for schan's fake socket (fd=-1, no connect/auth);
Socket provides no deterministic failure-injection point, so covering them
would require a test hook in the hot core path — high cost/risk and
inconsistent with the codebase.
Correctness here rests on a single invariant: sub_chan is owned solely by
the fake Socket, with SubChannel::BeforeRecycle as the only deletion site — no
failure branch manually deletes it. This is enforced by review. The unit test
covers externally triggerable failures (nullptr, duplicate) and the success
path, validating the OWNS/DOESNT_OWN rule.
--
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]
