zbus::Interface rework for thread-safety and async

[zeenix]

In GitLab by @danieldg on Sep 11, 2021, 22:53

1. Remove the error returns from all zbus::Interface methods, have them handle sending the reply on their own. Exception: introspection.

2. Remove all &mut self functions from zbus::Interface but change the code generation to produce a struct instead of only a trait impl:

struct Foo {
   hat: i32,
   hair: u64,
}

#[dbus_interface(name = "org.example.Foo", struct = ExampleFoo)]
impl Foo {
    fn split_hair(&mut self) {
        self.hair = self.hair * 2 - 1;
    }

    fn fuzziness(&self) -> u64 {
        if self.hat > 0 {
            0
        } else {
            self.hair
        }
    }

    async fn get_hat(&mut self, #[zbus(connection)] conn: &Connection) -> fdo::Result<bool> {
        let hats = AsyncHatStoreProxy::new(conn).await?;
        if let Some(hat) = hats.get_hat().await? {
            self.hat = hat;
            return Ok(true);
        } 
        Ok(false)
    }
}

//////////////// GENERATES ////////////////
struct ExampleFoo(RwLock<Foo>);
impl Interface for ExampleFoo {
    fn call(&self, ...) -> BoxFuture<'_, ()> {
      async move {
        match method_name {
            "SplitHair" => send_reply(self.0.write().await.split_hair()),
            "Fuzziness" => send_reply(self.0.read().await.fuzziness()),
            "GetHat" => send_reply(self.0.write().await.get_hat(conn).await),
            _ => send_reply(Err(MethodNotFound)),
        }
      }
    }
    fn get(&self, name: &str) -> BoxFuture<'_, ()> { ... }
    fn set(&self, name: &str, value: &Value) -> BoxFuture<'_, ()> { ... }
}
impl From<Foo> for ExmapleFoo { ... }

[zeenix]

The locking is an inner detail. Don't think it should be exposed to dbus_interface user.

[zeenix]

In GitLab by @danieldg on Sep 14, 2021, 24:55

You could easily consider it an inner detail of the Interface implementation instead, which would both simplify the ObjectServer and avoid some overhead (call vs call_mut) and allow some less-common use cases (property getters can't currently take &mut self atm, which requires oncecell/mutex for a lazy init instead of just Option).

[zeenix]

which would both simplify the ObjectServer and avoid some overhead (call vs call_mut)

That'd be nice but not at the cost of exposing a bit of strange (IMO) API to the user.

allow some less-common use cases (property getters can't currently take &mut self atm, which requires oncecell/mutex for a lazy init instead of just Option).

As you recognize yourself, this is a niche case so if user must use inner mutability here (there are other options too, like keep the state in another task/thread and communicating with channels), I'm fine with that.

[zeenix]

allow some less-common use cases (property getters can't currently take &mut self atm, which requires oncecell/mutex for a lazy init instead of just Option).

As you recognize yourself, this is a niche case so if user must use inner mutability here (there are other options too, like keep the state in another task/thread and communicating with channels), I'm fine with that.

Oh and we can easily switch to getter getting &mut, or have user choose which one (just like we do for method calls).

[zeenix]

In GitLab by @danieldg on Sep 15, 2021, 24:29

That'd be nice but not at the cost of exposing a bit of strange (IMO) API to the user.

I would say the call and call_mut interface is a strange API that exposes (at a design level) the internal RwLock.

Requiring ObjectServer to contain an RwLock just so the user can write &mut self methods seems backwards from how rust usually does things: if the trait could be invoked in a shared manner (like Interface) then everything possible takes &self and you use an internal RwLock (or Mutex, or some fancier thing like ArcSwap) for shared state. This could be hidden by the dbus_interface macro to allow the user to create &mut self methods there (so existing code mostly works the same) but allow more flexibility by the manual implementation (which is also an advanced API).

Adjusted the original post to remove the rwlock part from the user API and make it a detail, and retain the 'async_trait' style returns.

[zeenix]

That'd be nice but not at the cost of exposing a bit of strange (IMO) API to the user.

I would say the call and call_mut interface is a strange API that exposes (at a design level) the internal RwLock.

I can agree here but Interface trait isn't something most (99% at least) users even look at, let alone implement it. So it being a bit awkward is not worth worrying much about. We actually wanted to hide it in the beginning but then for a few reasons didn't. dbus_interface macro is what matters.

Requiring ObjectServer to contain an RwLock just so the user can write &mut self methods seems backwards from how rust usually does things: if the trait could be invoked in a shared manner (like Interface) then everything possible takes &self and you use an internal RwLock (or Mutex, or some fancier thing like ArcSwap) for shared state.

That's more work for the user so if we can handle it ourselves, why not make it super simple for the user? Keep in mind that mutable state in Interface is not at all a niche case here.

This could be hidden by the dbus_interface macro to allow the user to create &mut self methods there (so existing code mostly works the same)

But with your proposal, they'll have to do extra magic that they need to remember (which nobody will and they'll have to constantly check the docs, especially with attribute macros still not very well supported by rust-analyzer) and it doesn't save them much of headache compared to if they just handle mutability themselves. Just getting &self and &mut self is super easy and trivial to remember OTOH.

Having said that, I don't think we need to provide such an API to the user if we decide to remove the inner lock from the ObjectServer. They'll just get &self and we document to use the usual rust API for inner mutability or channels etc. I need to sleep over this a bit to decide though. :)

---

The issue InterfaceDeref and InterfaceDerefMut from my WIP async ObjectServer branch being awkward, that you mentioned on the Matrix channel, is something I can very much agree on. However, they're only needed when using the ObjectServer::with methods so not sure it's that big a problem.

[zeenix]

In GitLab by @danieldg on Sep 29, 2021, 02:20

This is mostly resolved by !388 since that makes it easier to turn everything into wrappers around Arc<RwLock<SharedState>> or manage locks manually. Doing this in generated code is likely more confusing than helpful.