Allow owned ObjectServer instances #58

Closed zeenix closed 1 year ago

In GitLab by @whot on Jul 24, 2020, 08:50

Apologies, this is rather a beginner question. I'm failing at trying to set up an ObjectServer - my requirement is that everything is contained within a context struct (this will be a library with a internal dbus backend). So effectively I need:

struct Context {
   connection: zbus::Connection,
   object_server: zbus::ObjectServer,
}

Since the ObjectServer::new() takes a reference to the Connection, Rust screams at me no matter what I try. The naive implementation was of course

impl Context {
  fn new() -> Self {
     let c = Connection::new_session();
     let o = ObjectServer::new(&c);
     Context {c, o}
  }
}

Which won't work because c gets moved. And all other 50 tries of lifetimes, RefCells, etc. attempts all failed too so at this point I'm lost. The only example for ObjectServer I could find has a static lifetime so it doesn't cover this use-case. I'd appreciate any help here in how to store this in a struct. Thanks.

Hi Peter,

Currently on vacation without a computer so can't directly help you but you could ask the nice folks in #beginners channel on Rust Discord, the Rust forum or #rust on gnome IRC/matrix. Otherwise I can try and help you out next week when I'm back. 😊

I'd say that we really should provide owned version of ObjectServer as well so this is super easy.

In GitLab by @elmarco on Jul 24, 2020, 10:25

Ah the famous self-referential struct lifetime issue...

Here, it summarizes the possible solutions today, including with Pin. I haven't tried it yet: https://users.rust-lang.org/t/how-do-i-create-self-referential-structures-with-pin/24982/11

Thanks @elmarco. We need to provide the owned API for sure. I'll look into that when I'm back. It's hopefully easy. 😊

In GitLab by @elmarco on Jul 24, 2020, 10:55

Do you mean that ObjectServer should own the connection? That's probably an API break if that's templated. And design-wise, it looks wrong to me...

In GitLab by @whot on Jul 24, 2020, 12:35

@elmarco's link helped, thanks. I have a working solution, though I'm not sure I fully understand what I'm doing here:

pub struct Context<'a> {
    c: Connection,
    o: Option<ObjectServer<'a>>,
}

impl<'a> Context<'a> {
    pub fn new() -> Pin<Box<Self>> {
        let connection = Connection::new_session().unwrap();
        let res = Self {
            c: connection,
            o: None,
        };

        let mut boxed = Box::pin(res);
        let cref = NonNull::from(&boxed.c);

        zbus::fdo::DBusProxy::new(unsafe {&*cref.as_ptr()}).unwrap().request_name(
            "org.zbus.MyGreeter",
            zbus::fdo::RequestNameFlags::ReplaceExisting.into(),
        ).unwrap();

        let mut object_server = ObjectServer::new(unsafe {&*cref.as_ptr()});
        let quit = Rc::new(RefCell::new(false));
        let interface = Example::new(quit.clone());
        object_server.at(&ObjectPath::from_str_unchecked("/org/zbus/path"), interface).unwrap();
        boxed.o = Some(object_server);
        boxed
    }
}

AFAIU:

Option<ObjectServer> is needed because the Connection must be in the pinned box before we get the ref, so we need the objectserver field as None initially
Once it's in the Pin, we get the ref for the inside field and unsafely de-ref it to pass it to the ObjectServer (and the fdo::DbusProxy...)
Then we can move the ObjectServer into the pinned box and return from that
Whether the <'a> is correct or makes a difference beyond what the compiler wanting it I cannot say.

Now, whether that's a developer-friendly API... :wink:

In GitLab by @elmarco on Jul 24, 2020, 13:16

fairly advanced code, @GuillaumeGomez could you comment/help? thanks! :)

Yes but we can avoid an API break by providing an owned version of ObjectServer that can deref to ObjectServer. Similar to what we do for Value.

In worst case we break the API (as a last resort) but this is a very common use case that shouldn't require user to learn and use some very advanced Rust tricks.

In GitLab by @elmarco on Jul 24, 2020, 13:31

Fundamentally, the problem lies outside zbus, and is a general Rust problem, thus there should be a Rust solution to it. I don't really get how having a complementary owning API would help. If all Rust API that take references had supplementary APIs to take owned version instead, that would be atrocious. Yet, I think that's what you suggest here.

In GitLab by @elmarco on Jul 24, 2020, 13:33

Oh, I think I understand what you mean by derefing to ObjectServer. Ok, maybe, as a helper/supplementary API. Is this a design pattern in Rust??

In GitLab by @whot on Jul 25, 2020, 01:14

as a naïve suggestion: would changing the API to take an Rc<ObjectServer> be sufficient? We only need immutable ones but we need that in multiple places so as a first guess, Rc may be enough.

Not naive at all actually and that's exactly what it should have been like in the first place. ObjectServer keeps everything owned except for connection. Borrowing is meant for temporary sharing of data. You want Rc or Arc if you want to share data in contexts of structs that user would want to keep around.

I've been contemplating some solutions, and one of them would be exactly this and I'm sure we can achieve this without an API break (we'll have to deprecate new though). However I don't have access to a computer at the moment to try any of them out. Stay tuned, I'll try to give you a solution next week. 😊

In GitLab by @whot on Jul 26, 2020, 11:56

I appreciate it, but at least from my side there's no hurry :)

Of different ideas I thought of, this would be the easiest I think and it allows us to not deprecate (or break) any API: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=2f214a15c3d3ae2cbac66a5eabef7023

In GitLab by @elmarco on Jul 26, 2020, 22:54

yes, that's about what I imagined you would propose. I would really like to hear @GuillaumeGomez or someone with a long rust background to see if this is a common idiom/pattern, or if we can just leave that off to the user-side.

yes, that's about what I imagined you would propose

I've a few other ideas but all of them involved either breaking the API or at least deprecating new.

I would really like to hear @GuillaumeGomez or someone with a long rust background to see if this is a common idiom/pattern, or if we can just leave that off to the user-side.

No, we're most definitely are not leaving this to the user. I know enough Rust to know that borrowing is temporary and if you want to share data more permanently, you use Rc or Arc. If it wasn't for breaking/changing the API, I would definitely want to change to use of either of these containers. Opinion of @GuillaumeGomez is most welcome of course but I doesn't seem like they're interested in offering it. :)

I must say, I'm completely baffled how you insisted on owned (only) API in places, where unowned API was more important but have the completely opposite opinion where owned API will make a lot of sense. E.g it's a lot more likely that the return value of a method call is only temporarily needed, while the proxy instance is more long lived (and kept around).

In GitLab by @elmarco on Jul 26, 2020, 23:23

I think you misread my point-of-view, I strive for simplicity and convenience. If you can make it work with borrowed value, it's superior to owned values in my book too. Now, having an API that supports both owned and unowned looks very suspicious, potentially uncoherent with other parts of the API and an anti-pattern to me...

In GitLab by @GuillaumeGomez on Jul 27, 2020, 24:05

Kinda busy, so we can talk directly about it if you want but I don't think I'll have enough time to go through comments before a few weeks.

In GitLab by @whot on Jul 27, 2020, 04:21

I've a few other ideas but all of them involved either breaking the API or at least deprecating new.

But is there anything wrong with deprecating it? AFAICT the current approach doesn't really work well for anything but the most basic example programs and there's bound to be someone else who runs into the same issue. Deprecating it is the best signal that using this approach is not the best option.

I don't think the ObjectServer owning the Connection is a good idea either - you'll run into similar lifetime issues with e.g. 'zbus::fdo::DBusProxy'. And possibly others in this crate. Arc or Rc seems to be the default approach for where you need to pass the same thing into multiple entities. Though AIUI it'll be immutable that way which may be a problem later. That's already the case with the reference though so nothing changes, I think.

In GitLab by @elmarco on Jul 27, 2020, 07:06

@whot Holding T and &T in the same struct is not so common in Rust. I understand the desire for providing 1 object to C bindings, but it's certainly not what most Rust program would need. Imho, the problem is not specific to zbus, and I don't think we should have Rc/Arc in our API. (or it will surely become a much more complicated and unsafe API). Fwiw, I think that's also one part of the dbus-rs complexity, it tries to handle all the various kind of ownerships in a templating manner, it makes me puke. The Rust API I usually use certainly don't have any of this, they let you deal with this problem yourself, because your are not really supposed to have them in pure/clean Rust designed programs.

But is there anything wrong with deprecating it? AFAICT the current approach doesn't really work well for anything but the most basic example programs and there's bound to be someone else who runs into the same issue. Deprecating it is the best signal that using this approach is not the best option.

@whot No, I'm not saying we don't need to support owned use case, just that the best would be not to deprecate or (especially) break the API while doing so. It all depends on what kind of solution we can come up with. Deprecating might be the best after all. I need to implement one or two of the solutions before we could decide.

I don't think the ObjectServer owning the Connection is a good idea either

Yeah, I'm aware and hence why I was thinking of supporting both.

you'll run into similar lifetime issues with e.g. 'zbus::fdo::DBusProxy'.

I was planning on creating a solution that goes for both Proxy and ObjectServer.

Arc or Rc seems to be the default approach for where you need to pass the same thing into multiple entities.

That is correct. Keep in mind that the enum-based solution I proposed can easily add support for Rc<Connection> and Arc<Connection> w/o breaking API. However, I think if we go for this solution, we should just go with Rc<Connection> (no Connection) at first and if in future we really have a use-case for Connection to be used from multiple threads, we can add Arc.

@elmarco

the problem is not specific to zbus,

The problem is with our API that we didn't think hard enough of the typical uses and ownership models.

and I don't think we should have Rc/Arc in our API.

There is nothing zbus-specific about those and they are used exactly for sharing an instance of a type between different parts of the code. They are a good solution here.

wiw, I think that's also one part of the dbus-rs complexity, it tries to handle all the various kind of ownerships in a templating manner, it makes me puke.

Last I checked (an year ago), it had the exact same issue as we're trying to solve here: It put unonwed data in it's struct and it made it very hard for me to keep the structs around. So IMO it's the other way around.

In any case, I've another solution that doesn't involve using Rc/Arc in the API at least. Stay tuned!

mentioned in commit 35d2c02bce86efa5427328d54049a841a441d731

mentioned in commit ad56b85f5d3ca4c9c130553af9a1eea82dd216e0

mentioned in commit e1b0831b1eadbf4aa787871f6ad5babbb56b1087

mentioned in commit 50d64a1b80db1e422557cf3cac8e5faf84ca59d1

@whot Could you give a test to !139 branch?

mentioned in commit 293fe1b892c44b666a2e258fdc0cbda63d58e3ac

mentioned in commit a77a515e0b76bb0f8a0a9b221cb1c6b5291aa652

mentioned in commit 4448d472b36a34fbacb9a04d03b953acdd0fe525

mentioned in commit 5db11af9b6ac291e8a4b4ce5e46adc4eaa3653d6

mentioned in commit 5a9c2bbc1fe20bde5b88a7265b7298ae5d1e217a

mentioned in commit 9d456ada53968b1435ce6a33d325722ef17f8b7d

mentioned in commit aec82f1545be7b882c98327b5bf65e15a94a2d2d

mentioned in commit 5e85968a9d2818d66f3cd66ff966f6fc7e8cac7e

@whot I tested against a modified version of your test code and it works with !139 :

use zbus::Connection;
use zbus::ObjectServer;

struct Context<'a> {
   connection: Connection,
   object_server: ObjectServer<'a>,
}

impl<'a> Context<'a> {
  fn new() -> Self {
     let connection = Connection::new_session().unwrap();
     let object_server = ObjectServer::new(&connection);

     Context {connection, object_server}
  }
}

Also so does this:

use zbus::Connection;
use zbus::ObjectServer;

struct Context {
   connection: Connection,
   object_server: ObjectServer<'static>,
}

impl Context {
  fn new() -> Self {
     let connection = Connection::new_session().unwrap();
     let object_server = ObjectServer::new(&connection);

     Context {connection, object_server}
  }
}

I'll merge the MR now.

closed via merge request !139

mentioned in commit c46cbe11e0a6a17538e83b03dcf0e960fa997036

In GitLab by @whot on Jul 31, 2020, 07:44

thanks!

unfortunately I've been pulled into something else now so I might not get to actually use this for a while :(

dbus2 / zbus

Allow owned ObjectServer instances #58