dbus2 / zbus-old

Rust D-Bus crate.
https://gitlab.freedesktop.org/dbus/zbus
Other
49 stars 13 forks source link

glib mainloop integration? #205

Closed zeenix closed 1 year ago

zeenix commented 3 years ago

In GitLab by @swsnr on Aug 28, 2021, 17:21

I use a zbus ObjectServer inside an application based on Glib's mainloop. Up to zbus 2.0.0-beta.3 I could just plug the file descriptor of zbus::Connection into glib with

pub fn source_add_connection_local<F: FnMut(&zbus::Message) + 'static>(
    connection: zbus::Connection,
    mut on_message: F,
) -> SourceId {
    let fd = connection.as_raw_fd();
    glib::source::unix_fd_add_local(
        fd,
        glib::IOCondition::IN | glib::IOCondition::PRI,
        move |_, condition| {
            debug!("Connection {file_descriptor} entered IO condition {:?}");
            match connection.receive_message() {
                Ok(message) => on_message(&message),
                Err(err) => error!(
                    log,
                    "Failed to process message on connection {}: {:#}", fd, err,
                ),
            }
            glib::Continue(true)
        },
    )
}

In the on_message callback I'd simply dispatch the message to the object server.

As of zbus-2.0.0-beta.4 this no longer works; for some reason the file descriptor never even enters the IN condition. In other words I never see the debug! log from above. I tried to make sense of the diff between beta.3 and beta.4, but it looks as if beta.4 migrated to async connections and I'm somewhat unfamiliar with async Rust.

Am I missing something obvious? What's the recommended way to integrate a zbus object server into an external event loop?

zeenix commented 3 years ago

Hi, I'm not sure either which change broke this code for you but since zbus now receives the message from the socket directly in an internal thread I'm not even sure the as_raw_fd API can be used/relied upon. I actually wanted to remove that but @elmarco needed it so we kept it so maybe he can shed some light here?

As for integration with glib, you actually don't need to create a source or use fd. Like with other async run times, all you need to do currently is to continuously call ObjectServer::try_handle_next (you can do that in a separate thread for example). Having said that, I'm currently working on removing the need to do that so hopefully that will be in 2.0.0-beta.7. i-e You'd then just setup the ObjectServer and keep it around and that's it.

zeenix commented 3 years ago

In GitLab by @elmarco on Aug 29, 2021, 23:17

Hi, I'm not sure either which change broke this code for you but since zbus now receives the message from the socket directly in an internal thread I'm not even sure the as_raw_fd API can be used/relied upon. I actually wanted to remove that but @elmarco needed it so we kept it so maybe he can shed some light here?

I relied on similar integration as @lunaryorn described, so I will likely hit the same issue when I upgrade zbus. Maybe the thread shouldn't be running betweehn try_handle_next() calls, so existing single-threaded loop code keep working with a FD watch.

zeenix commented 3 years ago

In GitLab by @swsnr on Aug 30, 2021, 20:31

[…] (you can do that in a separate thread for example)

I know but that's precisely what I was trying to avoid :grimacing:

I need to run some Gio code in response to incoming DBus method calls, and I'm not sure the thread safety of that code, so I figured it would be easiest to just keep everything on Glib's mainloop thread. If I put everything onto a separate thread then I'd have to care for thread safety and perhaps do some back and forth messaging to get everything back to the thread where it belongs.

I'd probably also have to open multiple zbus connections instead of reusing the same connection for the server side and the client side of some messages I need to send to DBus.

All in all it'd make things more complicated… :disappointed:


Speaking of the object server, while trying to work around this issue I found zbus' object server a little inflexible, because it couples message dispatch with sending return values.

If there had been an "object dispatcher" extracted from the object server to which I'd hand a zbus message to dispatch to the registered objects, and get e.g. a Future<Option> in return with the response message to send back in case the message was dispatched, I'd have had more flexibility in splitting up message receival and sending and handling of messages and could've worked around this better.

zeenix commented 3 years ago

In GitLab by @swsnr on Sep 4, 2021, 12:07

I've continued to experiment with the asynchronous API and realized that the glib crate lets me run futures on a glib MainContext, so I've ended up with this:

/// Run an object server on the given connection.
///
/// Continuously polls the connection for new messagesand dispatches them to `server`.
pub async fn run_server(mut connection: zbus::azync::Connection, mut server: zbus::ObjectServer) {
    while let Some(result) = connection.next().await {
        match result {
            Ok(message) => match server.dispatch_message(&message) {
                Ok(true) => trace!("Message dispatched to object server: {:?} ", message),
                Ok(false) => warn!("Message not handled by object server: {:?}", message),
                Err(error) => error!(
                    "Failed to dispatch message {:?} on object server: {}",
                    message, error
                ),
            },
            Err(error) => error!("Failed to receive message from bus connection: {:?}", error),
        }
    }
}

fn main() {
    let context = glib::MainContext::default();
    context.push_thread_default();

    let mainloop = glib::MainLoop::new(Some(&context), false);

    let connection =
        zbus::Connection::session().with_context(|| "Failed to connect to session bus")?;

    info!("Registering all search providers");
    let mut object_server = zbus::ObjectServer::new(&connection);
    register_search_providers(&connection, &mut object_server)?;

    info!("All providers registered, acquiring {}", BUSNAME);
    let object_server = object_server
        .request_name(BUSNAME)
        .with_context(|| format!("Failed to request bus name {}", BUSNAME))?;

    info!("Name acquired, starting server and main loop");
    context.spawn_local(run_server(connection.inner().clone(), object_server));

    mainloop.run();
}

In other words I'm executing a future to continuously receive and dispatch messages on the default MainContext.

Is this a good idea? I'm not very familiar with Rust's futures, and even less so with glib and the async zbus API, so I'm not sure… I do like the code though since it's much less magic and easier to understand than the FD-based IO dispatch, and feels "higher level" :blush:

It does seem to work though; messages get handled and replied to just fine.

zeenix commented 3 years ago

In GitLab by @swsnr on Sep 4, 2021, 12:12

The only thing I don't really like here is that ObjectServer still requires a synchronous connection to sent its replies to. If the object server would just dispatch and return the message to be sent back to the caller instead it wouldn't require access to the connection anymore and I could actually implement message handling as a single stream from the connection through the object server back to the connection sink.

Would you accept a patch which extracts something like an "ObjectDispatcher" from ObjectServer? For my use case this would absolutely satisfy the "asynchronous server interface" feature :slight_smile:

zeenix commented 3 years ago

If the object server would just dispatch and return the message to be sent back to the caller instead it wouldn't require access to the connection anymore and I could actually implement message handling as a single stream from the connection through the object server back to the connection sink.

Reading this, makes me even more certain that you probably don't want to use ObjectServer then and implement your own dispatch mechanism. You could still make use of dbus_interface macro to make your code simpler, but that would need moving of replying code from dbus_interface to ObjectServer (the Interface trait methods will need to return the response message). That's a change I'm very much willing to consider.

zeenix commented 3 years ago

In GitLab by @swsnr on Sep 5, 2021, 10:27

[…] implement your own dispatch mechanism.

Oh, that escalated quickly :smile:

Doing that would probably double the size of my code, to a point where it'd likely be simpler to bite the bullet and use the not very Rusty GDBus server interface :shrug:

All I want is for zbus to integrate into an external event loop so that my code remains single threaded and simple; I would not like to run two event loops inside my program and channel data between those, just because zbus insists on having its own loop.

I understand your intend to make object server more "automatic", and I think that's a good thing for self-contained services which do not rely on any other event loop, but I believe it makes programs with preexisting event loops (e.g. every GUI application and every glib-based application out there) harder.

If you could just decouple dispatch to interface implementations from message sending and receiving zbus would compose well with external event loops with just a few lines of code. If you couple those even more to a point where explicit dispatch to interface implementations is no longer possible zbus no longer composes with external event loops at all. I don't think that this would be beneficial :shrug:

zeenix commented 3 years ago

I understand your intend to make object server more "automatic", and I think that's a good thing for self-contained services which do not rely on any other event loop, but I believe it makes programs with preexisting event loops (e.g. every GUI application and every glib-based application out there) harder.

I think there is a misunderstanding here. This proposed new "automatic" way would actually work out of the box with all event loops/runtimes. It also makes things super easy for simple implementations: create a connection, declare your interface impl, hook it up with the connection's object server and you're mostly done. As pointed out in the GNOME discourse discussion, you only need channels if you need communication between your interface(s) and your main logic.

If you don't want threads to be launched behind your back, ConnectionBuilder already provides a way to disable that but then you'll need to ensure that you tick the executor (which you get a ref to with Connection::executor()) yourself, which you can do in an idle function that you can register with glib's main loop.

zeenix commented 3 years ago

In GitLab by @swsnr on Sep 6, 2021, 21:20

I think there is a misunderstanding here.

Perhaps we have a different understanding of what "working with" means :slight_smile:

This proposed new "automatic" way would actually work out of the box with all event loops/runtimes.

From my point of view it wouldn't work "with" external event loops, it'd just work alongside them. It simply creates a second event loop with a second execution thread. And then I'd manually have to move stuff across threads to communicate between those two event loops, in whatever way. That's not "super easy" to me.

Whereas currently I can make zbus dispatch messages on the main thread, with the code above, and for the entire rest of the program pretend that it's all nice and single-threaded with a single global event loop. To me this is easier and simpler than having two threads which have to talk to each other.

As pointed out in the GNOME discourse discussion, you only need channels if you need communication between your interface(s) and your main logic.

Only? I don't have any channels now :shrug:

I have one main loop and one main thread where everything runs on. And I need but those 15 or so lines from above to plug zbus into that main loop.

Channels or any other kind of inter-thread communication will likely add more than those 15 lines, and worse, it wouldn't be isolated from the code like the current setup but creep into every part of the code.

If you don't want threads to be launched behind your back

I don't really care about threads launched behind my back; as far as I'm concerned zbus can happily use its own per connection thread. Just as long as I can control on which thread the interface implementation runs on :slight_smile: I can do that currently, and it looks as if I'm loosing this ability :pensive:

I know that I could alternatively use default-features = false to disable the internal-executor feature, and then tick the executor manually but I think that's not safe: Any transitive dependency on zbus which doesn't disable default-features will bring the internal-executor back in and that's entirely outside of my control.


That's it from me, though. I think we're running around in circles and starting to repeat ourselves, and I feel that I'm going after a lost cause :shrug:

I do not like the direction zbus is taking; I would prefer if zbus was going into another direction with less opaque automatisms and more composable building blocks for DBus servers, i.e. decoupling things, not coupling them tighter.

But those who do the work get to say how it's done :slight_smile:

zeenix commented 3 years ago

I know that I could alternatively use default-features = false to disable the internal-executor feature, and then tick the executor manually but I think that's not safe: Any transitive dependency on zbus which doesn't disable default-features will bring the internal-executor back in and that's entirely outside of my control.

  1. Why unsafe? It's not a problem if two threads tick the executor.
  2. library-crates really shouldn't be doing this. That's something for applications.
  3. Now it's also possible to decide that when creating the Connection instance, just FYI.

But as you said, this isn't the part that's problematic for you. :)

As pointed out in the GNOME discourse discussion, you only need channels if you need communication between your interface(s) and your main logic.

Only? I don't have any channels now :shrug:

Not sure how that's relevant to what you replied to but let me try again to clarify: with the new API, you will not need channels (or event_listener I referred to) if you don't communicate from your interface implementation to your main logic.

I have one main loop and one main thread where everything runs on. And I need but those 15 or so lines from above to plug zbus into that main loop.

Channels or any other kind of inter-thread communication will likely add more than those 15 lines, and worse, it wouldn't be isolated from the code like the current setup but creep into every part of the code.

I really don't see how, given your claim that you've very simple service.

zeenix commented 3 years ago

I know that I could alternatively use default-features = false to disable the internal-executor feature, and then tick the executor manually but I think that's not safe: Any transitive dependency on zbus which doesn't disable default-features will bring the internal-executor back in and that's entirely outside of my control.

  1. Why unsafe? It's not a problem if two threads tick the executor.

Ah, you meant it's not reliable since your deps can enable the feature, which is indeed very likely to happen since it's the default feature. However, as I wrote, you don't need to disable the feature and can control this at Connection creation time now.

zeenix commented 3 years ago

mentioned in commit f51aed975f3e2249f5d0033d965f6876bc4b75e9