Allow objects to be added/removed from method calls #220

Closed zeenix closed 1 year ago

In GitLab by @danieldg on Sep 29, 2021, 03:26

refi64[m]12:

Hey so I'm trying to use zbus for a personal project (on the main branch for that sweet, sweet async server support), but I think I hit a bit of a minor roadblock? It seems there's no way to get a mutable ObjectServer inside of a handler (using #[zbus(object_server)] gives me a &ObjectServer), meaning I can't export a new object from within a handler. In my case, the method is supposed to return the new object handle, so offloading that via tokio::spawn and calling object_server_mut from there or something isn't really an option. Would there be any chance of adding a #[zbus(object_server_mut)] option or similar? (I could send an MR for it if needed.)

I think the easiest solution is to make ObjectServer::at a &self method.

I actually did some work for this some weeks ago but then abondoned the effort cause it wasn't as trivial as one would think. The main issue here is that we've the node tree to manipulate.

In GitLab by @danieldg on Sep 30, 2021, 01:06

You should just be able to lock the node tree as a whole to modify it. The node tree should already be unlocked by the dispatch due to the Arc (though I haven't actually looked at that code... if it doesn't already, it would need to clone the Arc instead of just grabbing a reference).

right. I didn't mean to imply it's impossible or even very hard, just not as trivial as it sounds. :)

In GitLab by @refi_64 on Oct 1, 2021, 01:36

So after playing with this for a bit, it indeed became much more complex than I anticipated, but not for the reasons I thought it would be :sweat_smile:

In essence, right now I just wrapped the node tree in an RwLock, then said lock is acquired and held for the duration of all node operations. This almost works, but... Currently, given InterfaceDeref/Mut<'a, I>, the lifetime 'a is the lifetime of the ObjectServer instance. However, since the interface value inside is contained within the node tree, and the node tree is now accessed via the lock, the lifetime 'a becomes the lifetime of the lock.

This is workable for with/_mut, since I can just make sure the InterfaceDeref instance doesn't escape the callback. However, of course there's also get_interface/_mut, which returns the InterfaceDeref instance, thus the lifetime needs to extend past the function in this case.

The simplest idea: since the interfaces are in their own locked arcs, we could technically just detach their lifetimes completely, storing the interface's Arc inside the InterfaceDeref struct. However, this would be self-referential enough for Rust to complain, and the only way to work around it would be to use ouroboros. Most other variants of this idea still run into the same issue.

Tokio does have a rather nice OwnedRwLockRead/WriteGuard which I believe would solve this, but that's not available in async_lock.

In GitLab by @danieldg on Oct 1, 2021, 01:58

Hmm, you shouldn't need any kind of self-reference to do the lookup under a lock, clone the Arc, drop the lock, and then pack that in the InterfaceDeref. The same tactic should be usable during the dispatch. Do you have the code that was causing you problems? It sounds like you have what I think would work when you talk about detaching the Interface and the ObjectServer, but I can't think why this would be a self-reference.

In GitLab by @refi_64 on Oct 1, 2021, 03:36

@danieldg From what I can tell it still doesn't work: since Arc::deref() returns a reference tied to the Arc's lifetime, that reference, and thus the lock guard resulting from it, go invalid once the Arc goes out of scope, i.e. the guard needs to be actively borrowing the arc in order for it to be valid. We'd have to include the Arc somehow in order for the guard to remain valid, but now there's a field inside the struct references another field inside the same struct, hence the circle.

This is what I mean: consider this playground link, which is how the code is now. In here, the iface guard of course references node. But, that reference is only because the lifetime of the Arc the guard is from is tied to node. If I .clone() it first, now the Arc's lifetime is tied to the stack...but the guard is still tied to its originating Arc:

error[E0515]: cannot return value referencing local variable `locked_iface`
  --> src/lib.rs:26:9
   |
25 |         let iface = locked_iface.read().await;
   |                     ------------ `locked_iface` is borrowed here
26 |         InterfaceDeref { iface, phantom: PhantomData }
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returns a value referencing data owned by the current function

For more information about this error, try `rustc --explain E0515`.
error: could not compile `playground` due to previous error

TLDR there isn't really a clean way of having the guard not tied to the lifetime of some Arc, and if we put the Arc itself in InterfaceDeref, we get the cycle. (This of course works right now because we never clone the Arc, so its lifetime is that of the ObjectServer, and thus the guard is tied to the ObjectServer as well, so it all "just works".)

(I will admit I could be missing something major here! I'm still relatively new to Rust...but I've been throwing ideas in my head for a while now, and nothing really seems to stick.)

EDIT: Second playground link was incorrect, fixed.

Right. As I said, it's not going to be trivial at all, if possible. There is also the issue of possible deadlocks resulting from interface method modifying the node they're currently being called on, to consider. So I'm wondering that maybe we need a different approach here.

mentioned in commit 4c0cdef7d3da9cc05814479878397975031d715a

dbus2 / zbus-old

Allow objects to be added/removed from method calls #220