two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4246993883
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,19 +201,33 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
options.health_check_interval_s = FLAGS_channel_check_interval;
if (Socket::Create(options, &sock_id) != 0) {
- delete sub_chan;
+ // On every creation-failure path Socket recycles synchronously and
runs
+ // SubChannel::BeforeRecycle, which deletes chan iff OWNS_CHANNEL and
+ // then deletes sub_chan. Do NOT delete here: that double-frees.
LOG(ERROR) << "Fail to create fake socket for sub channel";
return -1;
}
SocketUniquePtr ptr;
int rc = Socket::AddressFailedAsWell(sock_id, &ptr);
if (rc < 0) {
LOG(ERROR) << "Fail to address SocketId=" << sock_id;
+ // The fake Socket was created (holding the HC-related reference) but
+ // cannot be addressed. Recycle it so that sub_chan, and sub_channel
+ // when OWNS_CHANNEL, are deleted in SubChannel::BeforeRecycle.
+ SocketUniquePtr fail_ptr;
+ if (Socket::AddressFailedAsWell(sock_id, &fail_ptr) >= 0) {
Review Comment:
Correct — that second`AddressFailedAsWell` can never succeed (`Create` just
returned 0 holding the additional reference; once the first call returns -1 the
slot is version-mismatched, so the repeat also returns -1 and`fail_ptr` is
never set). Removed the dead block and its comment.`rc < 0` means the slot
version advanced past`id_ver+1` , i.e. the socket was already`SetFailed`
/recycled, so`BeforeRecycle` has already cleaned up`sub_chan` ; the branch now
just logs and returns.
##########
src/brpc/socket.cpp:
##########
@@ -722,7 +722,10 @@ int Socket::Create(const SocketOptions& options, SocketId*
id) {
int Socket::OnCreated(const SocketOptions& options) {
if (_io_event.Init((void*)id()) != 0) {
LOG(ERROR) << "Fail to init IOEvent";
- SetFailed(ENOMEM, "%s", "Fail to init IOEvent");
+ // VersionedRefWithId::Create() performs the failure transition after
+ // OnCreated() returns an error; attach the user before returning so
+ // recycling invokes SocketUser::BeforeRecycle.
+ _user = options.user;
Review Comment:
Agreed, and this is the key insight that drove the fix. Attaching`_user`
here is wrong not just because of the two caller symptoms but
because`BeforeRecycled` would run on a half-constructed socket (`_transport`
not created,`_id_wait_list` not initialized). Rather than make`BeforeRecycled`
safe for partial construction (a separate, larger change), the`_user`
attachment is dropped from the failure path entirely — it is assigned only at
the end of`OnCreated` on success. That also resolves your "cannot be decided at
the call site" point: with`_user` attached only on success,`Socket::Create`
failure never triggers`BeforeRecycle` , so the contract becomes uniform —
callers always own cleanup when`Create` fails — and`AddChannel` can use a
single unconditional cleanup path with no ambiguity. (Making`BeforeRecycled`
itself robust to a partially-constructed socket is left out of scope as you
suggested.)
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,19 +201,33 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
options.health_check_interval_s = FLAGS_channel_check_interval;
if (Socket::Create(options, &sock_id) != 0) {
Review Comment:
Right —`get_resource()` returning before`OnCreated` means no callback runs,
so`sub_chan` (and`chan` under`OWNS_CHANNEL` ) would leak. Restored caller-owned
cleanup in`AddChannel` : when`Socket::Create` fails we now delete the
wrapper`sub_chan` ourselves, and`sub_channel` iff ownership is`OWNS_CHANNEL` .
This is sound because, per the socket change,`BeforeRecycle` never runs on any
create-failure path, so there is no double-free.`PartitionChannel` uses the
default`OWNS_CHANNEL` and relies on schan for cleanup (its redundant`delete
pchan` on failure was already removed), so it has a correct fallback via schan.
##########
src/brpc/socket.cpp:
##########
@@ -722,7 +722,10 @@ int Socket::Create(const SocketOptions& options, SocketId*
id) {
int Socket::OnCreated(const SocketOptions& options) {
if (_io_event.Init((void*)id()) != 0) {
LOG(ERROR) << "Fail to init IOEvent";
- SetFailed(ENOMEM, "%s", "Fail to init IOEvent");
+ // VersionedRefWithId::Create() performs the failure transition after
+ // OnCreated() returns an error; attach the user before returning so
+ // recycling invokes SocketUser::BeforeRecycle.
+ _user = options.user;
Review Comment:
Both valid, thanks. Fixed by no longer attaching`_user` on the failure path
at all:`_user = options.user` is moved to the very end of`OnCreated` , so it is
set only on the fully-constructed success path. No`OnCreated` failure (IOEvent
init,`bthread_id_create` ,`id_wait_list` init, or`get_resource` failing
before`OnCreated` runs) attaches the user, so recycling never
invokes`SocketUser::BeforeRecycle` on a failed create. This removes
the`EpollOutRequest` double-delete (`Connect` 's`delete req` becomes the sole
cleanup) and the re-entrant`_map_mutex` acquisition in`Acceptor::StartAccept` —
and incidentally the same latent hazards on the`bthread_id` /`id_wait_list`
failure sub-paths.
--
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]
