dbus::Connection is neither Sync nor Send

[arcnmx]

I'm not familiar with libdbus or its semantics, but it seems unusual that it does not at least allow migration of the connection across threads via Send. Is the lack of both of these bounds intentional?

[diwic]

Cells are used internally so it cannot be Sync. I think it could be Send though, it's just me being overly cautious probably.

@albel727 You're good at these things, do you remember some weird edge case which makes it impossible for Connection to implement Send?

[marcelbuesing]

I started making it Sync + Send for my use case, reading dbus messages via tokio. Since I did not want to use the single thread Tokio current_thread::Runtime, but wanted to use a threadpool using runtime::Runtime I needed the AMessageStream from dbus_tokio::AConnection to be Send.
Added this unsafe impl Send for IConnection{} and then tried to make it sound using a Mutex in AConnection+ADriver for the Connection.
This kind of saved me from having to deal with thread safety in the dbus crate. The current problems in my fork are:

• anything reading/sending messages not using dbus-tokio is currently unsound, since the lower parts are actually still not really Sync or Send.
• AConnection.messages takes ownership of self
• Haven't fixed the sending message part yet since I did not need it yet and I'm right now not sure how to deal with the ownership problem with messages if one would want to send and receive messages via a shared Connection instance.

Another advantage though in the fork is that a client now no longer needs to pass a Handle or Runtime reference to AConnection::new. Especially the mutable Runtime reference I feel is difficult in a larger project.

So this:

    let conn = Rc::new(Connection::get_private(::dbus::BusType::Session).unwrap());
    let mut rt = Runtime::new().unwrap();
    let aconn = AConnection::new(conn.clone(), CoreHandle::current(), &mut rt).unwrap();

    let f = aconn.messages().unwrap().map_err(|_| ()).for_each(|msg| {
        println!("fsignal was: {:?}", msg);
        Ok(())
    });
    rt.block_on(f).unwrap();

becomes this:

    let conn = Connection::get_private(::dbus::BusType::Session).unwrap();
    c.add_match("type=signal,sender=org.freedesktop.DBus").unwrap();
    let aconn = AConnection::new(conn);

    let f = aconn.messages().unwrap().map_err(|_| ()).for_each(|msg| {
        println!("signal was: {:?}", msg);
        Ok(())
    });

    let mut rt = Runtime::new().unwrap();
    rt.block_on(f).unwrap();

Tokio Client Example

I hope I did not hijack this issue since this is mostly related to dbus-tokio. Anyway I would be grateful for help making this fork ready to be merged.

[diwic]

@marcelbuesing

So I had a look at how easy it would be to make IConnection Send a while ago. Apart from the actual ffi pointer, I believe I've made the mistake to add other non-Send stuff in there, like MessageCallback and MsgHandlerList. And requiring these to be Send would be a breaking change, and perhaps also limiting for all single-threaded uses of the dbus crate.

I'm not sure how to resolve this trade-off so that people can stuff Rcs in their callbacks and have a single-threaded efficient dbus server, while others can have a Sendable connection (and use arcs in their callbacks if they need to)...

Asked question here: https://www.reddit.com/r/rust/comments/ahpelv/to_send_or_not_to_send_thats_the_question/

[diwic]

@marcelbuesing
So I had a look at your fork but I found it difficult to know where to start. Also I don't have the in-depth knowledge of Tokio that I probably should have in order to determine whether all of this is the correct way of doing things.

But if there are some small no-brainers in there - which don't depend on other changes - I could merge them individually before discussing the bigger architectural changes?

[diwic]

Ok, so actually I'm suspecting that the best course of action here is to make a new Connection with all the lessons learned from the past. And that works better with both async in general and Send + Sync. I haven't got very far today (have a look at connection2.rs) but at least it's something.

@marcelbuesing @arcnmx what functions would you need on a connection to unblock your work?