Skip to content

[fix](brpc) Make secondary package aliases non-owning #65777

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: master
Choose a base branch
from
+14 −21
Open

[fix](brpc) Make secondary package aliases non-owning #65777

Show file tree
Hide file tree
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
23 changes: 6 additions & 17 deletions thirdparty/build-thirdparty.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2144,37 +2144,26 @@ build_lance_c() {
exit 1
fi

local required_rust_version="1.91.0"
local cargo_env=(
"CARGO_BUILD_JOBS=${PARALLEL}"
"CARGO_TARGET_DIR=${PWD}/${BUILD_DIR}"
"PROTOC=${TP_INSTALL_DIR}/bin/protoc"
)
if command -v rustup >/dev/null 2>&1 && [[ -z "${RUSTUP_TOOLCHAIN}" ]]; then
if rustup toolchain list | grep -q '^1.91.0'; then
cargo_env+=("RUSTUP_TOOLCHAIN=1.91.0")
if ! rustup toolchain list | grep -Eq '^1\.91\.0([[:space:]-]|$)'; then
rustup toolchain install "${required_rust_version}" --profile minimal

@github-actions github-actions Bot Jul 19, 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] Respect an explicitly selected Cargo before invoking rustup

cargo_bin may come from LANCE_C_CARGO/CARGO, but this branch runs whenever any unrelated rustup is on PATH. A network-restricted builder with an exact /opt/rust-1.91/bin/cargo (and matching RUSTC) plus rustup containing only stable now attempts rustup toolchain install 1.91.0 and exits under set -e before checking the valid custom toolchain; LANCE_C_CARGO_OFFLINE=ON is only applied later. Please auto-manage rustup only for the default/rustup Cargo path and validate explicit tool overrides directly.

fi
cargo_env+=("RUSTUP_TOOLCHAIN=${required_rust_version}")
fi

local required_rust_version="1.91.0"
local cargo_version
if ! cargo_version="$(env "${cargo_env[@]}" "${cargo_bin}" --version | awk '{print $2}')"; then
echo "failed to get cargo version for lance-c. Install Rust ${required_rust_version} or set LANCE_C_CARGO/RUSTUP_TOOLCHAIN."
exit 1
fi
if ! awk -v required="${required_rust_version}" -v actual="${cargo_version}" 'BEGIN {
split(required, r, ".");
split(actual, a, ".");
for (i = 1; i <= 3; i++) {
if ((a[i] + 0) > (r[i] + 0)) {
exit 0;
}
if ((a[i] + 0) < (r[i] + 0)) {
exit 1;
}
}
exit 0;
}'; then
echo "lance-c requires Rust/Cargo ${required_rust_version} or newer, but found ${cargo_version}."
if [[ "${cargo_version}" != "${required_rust_version}" ]]; then

@github-actions github-actions Bot Jul 19, 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] Pin the compiler that triggers this failure

The reported ethnum failure is produced by rustc, but this gate validates only cargo --version. Cargo 1.91.0 can still execute a newer compiler through inherited RUSTC/CARGO_BUILD_RUSTC (or a standalone Cargo paired with a newer rustc on PATH), so that setup passes here and reaches the same E0512 path. Please select and version-check the actual compiler Cargo will use, then pass the paired 1.91.0 toolchain consistently into the build.

echo "lance-c requires Rust/Cargo ${required_rust_version}, but found ${cargo_version}."
echo "Install Rust ${required_rust_version} or set LANCE_C_CARGO/RUSTUP_TOOLCHAIN."
exit 1
fi
Expand Down
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;
Comment thread
gavinchou marked this conversation as resolved.
+ secondary_mp.own_method_status = false;

@github-actions github-actions Bot Jul 17, 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 alias-key collision.

+ _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 17, 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 removal paths, the service while these secondary map entries remain. A later Find* returns dangling state, and Start() dereferences the freed status/service. Please track and 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