Skip to content

zb: make object destruction more robust #1553

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
Schmarni-Dev wants to merge 1 commit into
base: main
Choose a base branch
from
Open

zb: make object destruction more robust #1553

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
42 changes: 32 additions & 10 deletions zbus/src/object_server/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,24 +191,46 @@ impl ObjectServer {
let path = path.try_into().map_err(Into::into)?;
let mut root = self.root.write().await;
let (node, manager_path) = root.get_child_mut(&path, false);
let manager_path = manager_path.map(|p| p.to_owned());
let node = node.ok_or(Error::InterfaceNotFound)?;
if !node.remove_interface(I::name()) {
return Err(Error::InterfaceNotFound);
}
if let Some(manager_path) = manager_path {
if let Some(manager_path) = manager_path.clone() {
let ctxt = SignalEmitter::new(&self.connection(), manager_path.clone())?;
ObjectManager::interfaces_removed(&ctxt, path.clone(), (&[I::name()]).into()).await?;
}
if node.is_empty() {
let mut path_parts = path.rsplit('/').filter(|i| !i.is_empty());
let last_part = path_parts.next().unwrap();
let ppath = ObjectPath::from_string_unchecked(
path_parts.fold(String::new(), |a, p| format!("/{p}{a}")),
);
root.get_child_mut(&ppath, false)
.0
.unwrap()
.remove_node(last_part);
fn gen_paths(paths: &mut Vec<(ObjectPath<'_>, String)>, path: ObjectPath<'_>) {
if path.is_empty() {
return;
}
let mut path_parts = path.rsplit('/').filter(|i| !i.is_empty());
let last_part = path_parts.next().unwrap();
let ppath = ObjectPath::from_string_unchecked(
path_parts.fold(String::new(), |a, p| format!("/{p}{a}")),
);
paths.push((ppath.clone(), last_part.to_owned()));
gen_paths(paths, ppath);
}
Comment on lines +204 to +215

@zeenix zeenix Oct 21, 2025

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.

That's potentially a lot of allocations. I'm confident we don't need them. We can parse the path as we traverse the hierarchy.

let mut paths = Vec::new();
gen_paths(&mut paths, path);
for (ppath, last_part) in paths {
let path = ObjectPath::from_string_unchecked(format!("{ppath}/{last_part}"));
let node = root.get_child_mut(&path, false).0.unwrap();
if !node.is_empty() {
break;
}
let interfaces = node.interfaces().cloned().collect::<Vec<_>>();

@zeenix zeenix Oct 21, 2025

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.

why are we cloning here?

if let Some(manager_path) = manager_path.clone() {
let ctxt = SignalEmitter::new(&self.connection(), manager_path.clone())?;
Comment on lines +225 to +226

@zeenix zeenix Oct 21, 2025

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.

Why do we keep cloning the manager_path? 🤔 Pretty sure we don't need to do that for if let ... statements at least.

ObjectManager::interfaces_removed(&ctxt, path, interfaces.into()).await?;
}
root.get_child_mut(&ppath, false)
.0
.unwrap()
.remove_node(&last_part);
}
return Ok(true);
}
Ok(false)
Expand Down
8 changes: 6 additions & 2 deletions zbus/src/object_server/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,16 @@ impl Node {
}

pub(super) fn is_empty(&self) -> bool {
!self.interfaces.keys().any(|k| {
(!self.interfaces.keys().any(|k| {
*k != Peer::name()
&& *k != Introspectable::name()
&& *k != Properties::name()
&& *k != ObjectManager::name()
})
})) && self.children.is_empty()
}
Comment on lines -104 to +110

@zeenix zeenix Oct 21, 2025

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.

Looks like this change is about fixing the logic of "is this node empty" while the other changes aren't related to this fix? If so, could we please split the commit? It will also make it easier for me to review the changes.

@zeenix zeenix Oct 21, 2025

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.

Looks like this change is about fixing the logic of "is this node empty" while the other changes aren't related to this fix?

Right, reading up the PR description, the other changes seems to be related but still a separate change: removing all empty nodes above the node being removed.


pub(super) fn interfaces(&self) -> impl Iterator<Item = &InterfaceName<'static>> {
self.interfaces.keys()
}

pub(super) fn remove_node(&mut self, node: &str) -> bool {
Expand Down
Loading