You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "two-headBoy (via GitHub)" <gi...@apache.org> on 2026/10/10 07:17:09 UTC
[PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
two-headBoy opened a new pull request, #3595:
URL: https://github.com/apache/brpc/pull/3595
### What problem does this PR solve?
Issue Number: resolve #1408
Problem Summary:
- `SelectiveChannel::AddChannel` 各失败路径对用户 sub channel 的所有权处理不一致：有的泄漏（OWNS 时未删用户 channel / 已创建的 fake Socket 未回收），有的行为不统一，调用方无法预判，容易泄漏或 double free。
### What is changed and the side effects?
Changed:
- 统一所有权语义为「OWNS_CHANNEL 一律接管、DOESNT_OWN_CHANNEL 一律不接管」，以首次成功注册为唯一事实源。
- `Socket::Create` 失败时按 ownership 手动回收；其余失败路径对 fake Socket 调`SetFailed()` +`ReleaseHCRelatedReference()` ，由`SubChannel::BeforeRecycle` 按 ownership 清理；重复 channel 返回 -1 且不改变任何东西。补充注释并新增单测。
> 注：`Socket::Create` 失败、address 返回 < 0、健康检查未启用、`AddServer` 因`ServerId` 重复失败等内部路径依赖 Socket 内部状态，单测环境难以稳定复刻，正确性主要靠代码审查保证；单测覆盖外部可触发的失败（nullptr、重复）及成功路径，间接验证规则一致性。
Side effects:
- Performance effects: 无，仅修正失败路径回收，不影响成功热路径。
- Breaking backward compatibility: 无破坏性变更，仅把失败路径从「可能泄漏」修正为「按 ownership 正确回收」。
---
### Check List:
- Please make sure your changes are compilable.
- When providing us with a new feature, it is best to add related tests.
- Please follow [Contributor Covenant Code of Conduct](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4246989885
##########
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.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3595:
URL: https://github.com/apache/brpc/pull/3595
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4237267945
##########
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.
Review Comment:
This new failure cleanup makes the existing caller in `src/brpc/partition_channel.cpp:313-317` unsafe: it uses the default `OWNS_CHANNEL` option and then executes `delete pchan` when `AddChannel` returns `-1`. Any failure here that reaches `SubChannel::BeforeRecycle` now deletes `pchan` first, so that handler double-frees it. Update the caller as part of this ownership-contract change (and ensure it can distinguish the duplicate case, where ownership is intentionally not consumed).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4305442834
##########
src/brpc/selective_channel.h:
##########
@@ -72,6 +72,14 @@ class SelectiveChannel : public ChannelBase/*non-copyable*/ {
// Add a sub channel, which will be deleted along with schan or explicitly
// by RemoveAndDestroyChannel.
// On success, handle is set with the key for removal.
+ //
+ // Ownership (single source of truth: the FIRST successful registration):
+ // - OWNS_CHANNEL: schan owns sub_channel; caller must not delete it.
+ // - DOESNT_OWN_CHANNEL: caller owns sub_channel and must delete it.
+ // On failure the same rule applies to the registration being created: schan
Review Comment:
[replied by brpc-oncall robot] This failure contract is not honored when `SelectiveChannel::AddChannel` is called before `Init()`: selective_channel.cpp:564-566 just logs and returns -1 without deleting an `OWNS_CHANNEL` sub_channel, so under the documented rule the caller will not delete it either and the channel leaks (a caller keeping the old behavior still deletes it). Please either consume ownership on that path too, or narrow the wording, e.g. "If the schan has not been initialized, AddChannel returns -1 and does not take ownership." That path is externally triggerable, so it can be pinned down in the new unit test.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4246986491
##########
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4244983837
##########
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:
[replied by brpc-oncall robot] This second `AddressFailedAsWell` cannot succeed and the block is a no-op: `Socket::Create` just returned 0 for this id (the socket still holds the additional reference taken in `Create`), so this branch is not entered in practice; and once `AddressFailedAsWell` returns -1 the slot is free/version-mismatched, so repeating the call with the same id also returns -1 and `fail_ptr` is never set. If the branch is believed reachable, the leak it is meant to fix is not fixed by this code; otherwise please drop it (and the comment) rather than leaving dead code that looks like it handles the ownership handoff.
##########
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:
[replied by brpc-oncall robot] In addition to the double-delete of `EpollOutRequest`'s request and the `Acceptor::StartAccept()` re-entrant `_map_mutex` lock, running `BeforeRecycle` in this state is unsafe by itself: at this point the socket is only half-constructed, so after the callback `Socket::BeforeRecycled()` continues with `_transport->Release()` on a still-null `unique_ptr` (the transport is created at line ~734, after this check) and `bthread_id_list_destroy(&_id_wait_list)` before `bthread_id_list_init()` has run. So even removing the caller's redundant `delete req` would not make this failure path work; `_user` should not be attached here (or `BeforeRecycled` must be made safe for a partially constructed `Socket`).
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4237946273
##########
src/brpc/socket.cpp:
##########
@@ -722,6 +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";
+ // This is the only failure point before _user is attached below; set it
+ // now so the synchronous recycle runs SocketUser::BeforeRecycle and the
+ // user is cleaned up instead of leaking.
+ _user = options.user;
SetFailed(ENOMEM, "%s", "Fail to init IOEvent");
return -1;
Review Comment:
`Socket::Create` invokes `OnCreated` and, when it returns nonzero, calls `SetFailed()` again (`versioned_ref_with_id.h:421-426`). Calling `SetFailed` here can release the only reference and recycle this socket before `Create` regains control; its subsequent `SetFailed` may then operate on a recycled/reused slot. Attach `_user` here, but return `-1` and let `Create` perform the single failure transition so `BeforeRecycle` still cleans up the sub-channel safely.
This issue also appears in the following locations of the same file:
- line 728
- line 728
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4242271120
##########
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:
`VersionedRefWithId::Create()` can return when `get_resource()` fails, before entering `OnCreated()`. No recycling callback runs in that case. Removing delete `sub_chan` therefore introduces a wrapper-object leak under both ownership modes. The removed PartitionChannel cleanup also leaves that caller without a fallback.
##########
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:
There are two high-priority issues:
1. When `IOEvent::Init()` fails, assigning `_user `causes recycling to invoke `EpollOutRequest::BeforeRecycle()`, which deletes the request. After `Create()` returns, `Socket::Connect()` deletes the same request again at line 1353.
```c++
if (Create(options, &connect_id) != 0) {
LOG(FATAL) << "Fail to create Socket";
delete req;
return -1;
}
```
2. `Acceptor::StartAccept()` holds `_map_mutex` while calling `Socket::Create()`. The newly enabled failure callback synchronously enters `Acceptor::BeforeRecycle()`, which attempts to acquire the same non-recursive mutex. This prevents startup from returning an error.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4246997456
##########
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.)
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4237099060
##########
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;
+ // Socket recycles itself on failure, already running
+ // SubChannel::BeforeRecycle (deletes chan iff OWNS, then sub_chan).
+ // Do NOT delete sub_chan/chan here: that double-frees.
Review Comment:
`Socket::Create` does not always run `SubChannel::BeforeRecycle` on failure. `Socket::OnCreated` can fail at `_io_event.Init()` before `_user = options.user` (`socket.cpp:723-749`), and `VersionedRefWithId::Create` then recycles a socket whose `_user` is null. This leaks `sub_chan` and, for `OWNS_CHANNEL`, the caller's channel. The creation API or this handoff needs to make ownership consumption observable/consistent before removing manual 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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4236943429
##########
src/brpc/selective_channel.cpp:
##########
@@ -197,6 +201,12 @@ int ChannelBalancer::AddChannel(ChannelBase* sub_channel,
options.health_check_interval_s = FLAGS_channel_check_interval;
if (Socket::Create(options, &sock_id) != 0) {
+ // sub_chan is not attached to any Socket, recycle it directly.
+ // sub_channel is deleted only when OWNS_CHANNEL, consistent with the
+ // ownership contract.
+ if (sub_chan->ownership == OWNS_CHANNEL) {
+ delete sub_chan->chan;
+ }
Review Comment:
This can access and delete an already-recycled `SubChannel`. `Socket::OnCreated` assigns `options.user` before several later failure points (`socket.cpp:749-840`), and `VersionedRefWithId::Create` responds to those failures by calling `SetFailed()`, which can synchronously run `Socket::BeforeRecycled` and `SubChannel::BeforeRecycle`. For `OWNS_CHANNEL`, that callback has already deleted both objects before `Socket::Create` returns, so these lines cause a use-after-free/double-free. Cleanup needs one authoritative owner across both failures before `_user` is attached and failures after it is attached.
##########
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:
The added test only exercises duplicate rejection (which exits before creating a `SubChannel`) and successful registration; it does not exercise any of the newly changed `Socket::Create`, address, health-check, or `AddServer` failure cleanup. Consequently it cannot detect regressions such as double-freeing an owned channel during creation failure. Please add a deterministic failure-injection seam and cover at least the creation-failure ownership handoff for both ownership modes.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4306146150
##########
src/brpc/selective_channel.h:
##########
@@ -72,6 +72,14 @@ class SelectiveChannel : public ChannelBase/*non-copyable*/ {
// Add a sub channel, which will be deleted along with schan or explicitly
// by RemoveAndDestroyChannel.
// On success, handle is set with the key for removal.
+ //
+ // Ownership (single source of truth: the FIRST successful registration):
+ // - OWNS_CHANNEL: schan owns sub_channel; caller must not delete it.
+ // - DOESNT_OWN_CHANNEL: caller owns sub_channel and must delete it.
+ // On failure the same rule applies to the registration being created: schan
Review Comment:
Done — took the first option: AddChannel now consumes an OWNS_CHANNEL sub_channel even before Init(), so the failure contract is uniform everywhere. Both directions are covered in the new unit test.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4237219097
##########
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;
+ // Socket recycles itself on failure, already running
+ // SubChannel::BeforeRecycle (deletes chan iff OWNS, then sub_chan).
+ // Do NOT delete sub_chan/chan here: that double-frees.
Review Comment:
Fixed by attaching`_user` in the single`OnCreated` failure point that precedes it (`_io_event.Init` ), so the synchronous recycle runs`BeforeRecycle` on every creation-failure path. Chosen deliberately over a per-`SubChannel` observation flag: this adds zero per-object bytes (sockets are allocated at high volume — one per connection plus per-server fake sockets in load balancers), keeps the change confined to the one leaking branch , and hardens the cleanup contract for all`SocketUser` s rather than patching schan alone.`SubChannel` stays unchanged in size;`selective_channel` simply stops manually deleting on`Socket::Create` failure.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "two-headBoy (via GitHub)" <gi...@apache.org>.
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.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Fix inconsistent ownership handling in SelectiveChannel::AddChannel on failure (#1408) (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3595:
URL: https://github.com/apache/brpc/pull/3595#issuecomment-6342106966
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org