Skip to content

branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777 #65818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gavinchou wants to merge 1 commit into
base: branch-4.0
Choose a base branch
from
+8 −4
Open

branch-4.0: [fix](brpc) Make secondary package aliases non-owning #65777 #65818

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions thirdparty/patches/brpc-1.4.0-secondary-package-name.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ index 2087cbcf..7aede561 100644
Tabbed* tabbed = dynamic_cast<Tabbed*>(service);
for (int i = 0; i < sd->method_count(); ++i) {
const google::protobuf::MethodDescriptor* md = sd->method(i);
@@ -1282,6 +1290,14 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
@@ -1282,6 +1290,16 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
mp.method = md;
mp.status = new MethodStatus;
_method_map[md->full_name()] = mp;
Expand All @@ -27,17 +27,21 @@ index 2087cbcf..7aede561 100644
+ secondary_method_name.append(secondary_full_name);
+ secondary_method_name.push_back('.');
+ secondary_method_name.append(md->name());
+ _method_map[secondary_method_name] = mp;
+ MethodProperty secondary_mp = mp;

@github-actions github-actions Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Add a runtime regression for this ownership path

cloud/test/meta_server_test.cpp still forces config::secondary_package_name = "" specifically to avoid the brpc::Server destructor crash, so no test executes this changed non-owning branch. A patch-application/build check cannot prove the lifetime behavior. Please run a distinct non-empty alias with an owned service, verify both names resolve, and prove clear/destruction reclaims the shared service/status exactly once; also cover the equal-package ownership case above.

+ secondary_mp.own_method_status = false;

@github-actions github-actions Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve ownership when the alias equals the primary package

secondary_package_name is unrestricted. For MetaService, setting it to the current package doris.cloud makes the derived service and method keys identical to the primary keys. FlatMap::operator[] then overwrites the just-inserted owning entries with these non-owning copies, so RemoveService()/ClearServices() no longer delete the MethodStatus objects or the SERVER_OWNS_SERVICE allocation. Please reject or skip an alias equal to the primary package before assigning the non-owning copies.

+ _method_map[secondary_method_name] = secondary_mp;
+ }
if (is_idl_support && sd->name() != sd->full_name()/*has ns*/) {
MethodProperty mp2 = mp;
mp2.own_method_status = false;
@@ -1306,6 +1322,9 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
@@ -1306,6 +1324,11 @@ int Server::AddServiceInternal(google::protobuf::Service* service,
is_builtin_service, svc_opt.ownership, service, NULL };
_fullname_service_map[sd->full_name()] = ss;
_service_map[sd->name()] = ss;
+ if (!secondary_full_name.empty()) {
+ _fullname_service_map[secondary_full_name] = ss;
+ ServiceProperty secondary_ss = ss;
+ secondary_ss.ownership = SERVER_DOESNT_OWN_SERVICE;
+ _fullname_service_map[secondary_full_name] = secondary_ss;
+ }
if (is_builtin_service) {
++_builtin_service_count;
Expand Down
Loading