Skip to content

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

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.1
Choose a base branch
from
+8 −4
Open

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

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 branch

The Cloud lifecycle test currently forces config::secondary_package_name = "" specifically to avoid brpc::Server::~Server() crashing, so no test executes this changed non-owning branch. The patch/download checks prove only that the third-party patch applies. Please enable a distinct non-empty alias in a test, assert both service/method names resolve, and destroy or ClearServices() an owning server; the removal and equal-package cases above should also be covered.

+ 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 its current package doris.cloud makes secondary_full_name equal sd->full_name() and the derived method keys equal md->full_name(). FlatMap::operator[] then overwrites the just-inserted owning entries with these non-owning copies, so ClearServices/RemoveService no longer deletes the MethodStatus or the SERVER_OWNS_SERVICE allocation. Reject or skip an alias equal to the primary name before assigning it, and preflight any other occupied alias key.

+ _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;

@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.

[P1] Erase these aliases when removing the service

Making the alias non-owning avoids the destructor double free, but BRPC 1.4.0 RemoveMethodsOf erases only md->full_name() (plus the IDL no-namespace alias), and RemoveService erases only sd->full_name()/sd->name(). Therefore AddService with a secondary package followed by RemoveService—or an AddServiceInternal rollback—deletes the primary MethodStatus and, on owning paths, the service while these secondary entries remain. A later Find* returns dangling state, and Start() dereferences the freed status/service. Please erase the secondary method and service keys on every removal/rollback before deleting the shared objects.

+ }
if (is_builtin_service) {
++_builtin_service_count;
Expand Down
Loading