Handling adding and removing object paths from a single tree about dbus-rs

[diwic]

(although this requires an unsafe conversion of &m.tree to a &mut m.tree, it's a least possible) (This should not cause issues as long as it's always a different ObjectPath we're adding or removing than the objectpath/interface our method was invoked on)

It is never allowed to directly convert a & to a &mut. This is because there are LLVM annotations on & that indicates the pointer is read-only (edit: and on &mut that it's not aliased). This allows LLVM to do some interesting optimisations which, obviously, can cause random behaviour if you in fact break them. More information

[diwic]

@agrover

here's a slightly longer way of doing the same thing, but probably easier to understand:

// we want to add "my_new_path" to "tree"
let mut temp = Tree::new();
mem::swap(&mut temp, &mut tree);
let mut newtree = temp.add(my_new_path);
mem::swap(&mut newtree, &mut tree);

Nothing is ever added to the empty tree, it's just used to be temporarily stored in tree while we change the tree.

[agrover]

Proposed implementation of tree/objectpath.rs Tree add_o_ref:

    pub fn add_o_ref<I: Into<Arc<ObjectPath<M, D>>>>(&mut self, s: I)
                                                     -> Arc<ObjectPath<M, D>> {
        let m = s.into();
        self.paths.insert(m.name.clone(), m.clone());
        m
    }

[diwic]

Well, I'm not happy about sending a &mut Tree in, because if you could, you could remove all object paths, which could include the one you're currently calling a method on. That Rust protects you from being able to write that code is more a good than a bad thing IMO.

I believe the way to do this is in two steps: The method would just add an "add" or "remove" instruction in a TODO list. Back outside tree::run (edit: or tree::handle) you have full mutable access to the tree, where you can carry out the instruction.

If you want to wait to send the reply back to the client until after the instruction has been fully carried out, you can also store the method_return message in the TODO list (and return an empty vec back in the method handler).

[diwic]

I believe your suggested add_o_ref can be done today as:

*tree = mem::replace(&mut tree, Tree::new()).add(my_new_path);

[tasleson]

@diwic Maybe it's not a good API design choice, but for a different project that I've worked on I exposed a dbus job object for long running operations. The job object has properties and methods , one of which is to remove itself when the client no longer needs it. Thus in this case it was required that the dbus library have the ability to remove a dbus object from one of the methods on the object itself.

[diwic]

@tasleson I don't mind that the API exposes a function to remove its own object path (or interface, etc). But when we implement such functionality, we need to do this in a safe manner, avoiding dangling pointers.

IMO, the TODO list approach is the best one. The other option would be to do refcounting, but then every time you call into a method you need to up the reference count to the object path, interface, method etc, and down the refcount afterwards. While that would not be much of a performance hit compared to sending messages over D-Bus in general, it still feels like an unnecessary complication for something that happens quite rarely or not at all for most D-Bus applications.

[agrover]

@diwic: I'm afraid I still need more help understanding your use of mem::replace. Assuming we have obtained a mut ref of Tree, we want to add an objectpath to the existing tree -- your snippet appears to replace the tree entirely with a new tree with just the new object path, no?

[diwic]

That said, using mem::replace is still unergonomic compared just calling a method. So I just added one. Enjoy :-)

[diwic]

So, the add_o_ref method has been added (renamed to "insert"), the &mut Tree suggestion is unlikely to happen and I suggest to use a TODO list instead as a more "rustic" approach to the problem.

With that, is there anything else in this issue that needs fixing / clarification?

[agrover]

Much thanks @diwic. I think insert and remove plus the TODO list approach could work well (and avoid undefined behavior), we'll give that a try!