chenBright commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4355658306
##########
src/brpc/selective_channel.cpp:
##########
@@ -193,23 +197,42 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
sub_chan->ownership = subopt.ownership;
SocketId sock_id;
SocketOptions options;
- options.user = sub_chan;
+ // Deliberately do NOT attach sub_chan as SocketUser on creation: failure
+ // of get_resource() runs no recycling callback at all, while failure of
+ // OnCreated() recycles synchronously. Keeping _user unset until the
+ // socket is fully created makes cleanup a single call-site rule.
options.health_check_interval_s = FLAGS_channel_check_interval;
if (Socket::Create(options, &sock_id) != 0) {
- delete sub_chan;
+ // The socket was never user-attached, so no BeforeRecycle callback
+ // runs on any creation-failure path. Dispose sub_chan here: deletes
+ // chan iff OWNS_CHANNEL, then sub_chan (same as the callback does).
+ sub_chan->BeforeRecycle(nullptr);
Review Comment:
Could we use `butil::MakeScopeGuard` to call
`sub_chan->BeforeRecycle(nullptr)` on early returns? Creating the guard after
duplicate detection and initializing `sub_chan` would centralize cleanup and
protect future failure paths.
The guard should be dismissed immediately after `ptr->set_user(sub_chan)`,
when ownership transfers to the socket. Subsequent failures are already handled
by socket recycling, so keeping the guard active until `AddChannel` returns
successfully would cause duplicate cleanup.
--
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]
