API for converting Message to dbus-proxy generated signal types #276

Closed zeenix closed 1 year ago

In GitLab by @MaxVerevkin on May 25, 2022, 16:05

I create a Connection, then I subscribe to some events using DBusProxy::add_match(), then I create MessageStream from said connection.

When I finally receive a message, I think it would be intuitive if I could write something like

let message = stream.next().await
if let Ok(signal) = PropertiesChanged::try_from(message) {
    // ...
}

Maybe I'm doing something wrong, and this is already possible? And what is the recommended way to deal with messages from different interfaces?

Not quite against the suggestion here but I'm not understanding the use case. You use the lowest-level API to get the message but then want to switch to the slightly higher-level API to handle the message?

Why not make use of dbus_proxy macro and get different streams for different signals?

In GitLab by @MaxVerevkin on May 25, 2022, 17:48

You use the lowest-level API to get the message but then want to switch to the slightly higher-level API to handle the message?

Sort of.

One of my rules is type='signal', interface='org.freedesktop.DBus.Properties', member='PropertiesChanged', path_namespace='/org/freedesktop/UPower/devices' (note the path_namespace instead of path), and I don't think its possible to achieve this with a proxy.

More specifically, I have

    let proxy = DBusProxy::new(&dbus_conn).await?;
    proxy
        .add_match(
            "type='signal',\
                            interface='org.freedesktop.DBus.Properties',\
                            member='PropertiesChanged',\
                            path_namespace='/org/freedesktop/UPower/devices'",
        )
        .await?;
    proxy
        .add_match(
            "type='signal',\
                            interface='org.freedesktop.UPower',\
                            member='DeviceRemoved'",
        )
        .await?;
    proxy
        .add_match(
            "type='signal',\
                            interface='org.freedesktop.UPower',\
                            member='DeviceAdded'",
        )
        .await?;

And I would like to do something like

while let Some(Ok(msg)) = stream.next().await {
    if let Ok(added) = DeviceAdded::try_from(msg) => {
        // ...
        continue;
    }
    if let Ok(removed) = DeviceRemoved::try_from(msg) => {
        // ...
        continue;
    }
    // ...
}

One of my rules is type='signal', interface='org.freedesktop.DBus.Properties', member='PropertiesChanged', path_namespace='/org/freedesktop/UPower/devices' (note the path_namespace instead of path),

Gotcha so you want to only use one rule for multiple properties to avoid a lot of traffic. We could think about adding API to do that. Not sure how it'd look like but let's think about it.

For the other signals, proxy is sufficient.

In GitLab by @MaxVerevkin on May 26, 2022, 18:33

For the other signals, proxy is sufficient.

Last time I looked into the code, I got an impression that when I create multiple streams from one connection (via proxies), each stream would receive every message and then filter them. That was a wrong impression, was it?

We could think about adding API to do that

In this case it will also make sense to add an API for arg0namespace, arg<N>path, optional sender (example), etc.

For the other signals, proxy is sufficient.

Last time I looked into the code, I got an impression that when I create multiple streams from one connection (via proxies), each stream would receive every message and then filter them. That was a wrong impression, was it?

Yes and no. The stream user gets only get the relevant messages, the filtering is an internal detail.

We could think about adding API to do that

In this case it will also make sense to add an API for arg0namespace, arg<N>path, optional sender (example), etc.

sure but we'll need nicer names. :)

In GitLab by @MaxVerevkin on May 27, 2022, 11:29

By the way, this is the way dbus crate works. Example.

Ah that's where you got this original idea from. :) We should be very cautious about learning from dbus because if their API was good, I wouldn't have created zbus. As you can see, that's very low-level (message-based) solution.

However, as I wrote in the first comment, I'm not necessarily against the change you originally requested. We could start with implementing this the message->specific type conversions but the goal should be to provide a high-level API for these cases.

In GitLab by @Alxandr on Oct 7, 2022, 20:37

As !571 solves this partially, it got me thinking about how to solve the rest. The issue (as I see it) with all of the signals (like fdo::PropertiesChanged) is that they do not contain their data in a deserialized form, instead they only contain a Arc<Message>. This makes things both simpler and harder. It's trivial to convert a Arc<Message> to a fdo::PropertiesChanged (granted you have access to the internals), but knowing weather or not it's safe to do so is a different question all-together.

I see 3 viable strategies for doing a "type test" (and please correct me if there are others I've not thought about):

Do an identity check using the interface-name and message member-name (and message type). This has the unfortunate effect that I can craft a message claiming to be some interface/member and not have the correct data - but this is also the case when you receive data over the bus I guess.
Try to deserialize the data using the TryFrom impl in !571. If it works, create the signal struct (which requires deserializing the data again anyhow). If not, return the TryFrom-error.
Validate against the signature - I don't know if this is possible or built in to zbus. If it is, and it's easy & quick to do, I would probably do 1 & 3.

My proposal/suggestion is doing something like this:

Create a trait for all signals:

pub trait Signal: AsRef<Message> + TryFrom<Arc<Message>, Error=zbus::Error> {
  const INTERFACE: InterfaceName<'static>;
  const MEMBER_NAME: MemberName<'static>;
}

Implement trait on generated signal structs:

// somewhat pseudo
impl Signal for PropertiesChanged {
  const INTERFACE: &'static str = ".....";
  const MEMBER_NAME: &'static str = "PropertiesChanged";
}

impl TryFrom<Arc<Message>> for PropertiesChanged {
  type Error = zbus::Error;

  fn try_from(message: Arc<Message>) -> Result<Self, Self::Error> {
    match (message.message_type(), message.interface(), message.member_name()) {
      (MessageType::Signal, Some(INTERFACE), Some(MEMBER_NAME)) => Ok(Self(message)),
      _ => Err(......),
    }
}

I can start the implementation if this seems like a reasonable solution.

I really don't see why this needs to be complicated. The macro generating the TryFrom impl has access to the member, interface etc so there is no reason for any trait.

@MaxVerevkin Hey, looking though this again, I really feel like !571 already solved this for you. I know you asked for conversion from a Message to a Signal (and that is why @Alxandr and I thought the MR only solves this partially) but on closer look, I got reminded that Signal itself is not useful w/o SignalArgs. Since you now have the ability to convert Message to SignalArgs, I think you can skip the Signal.

You'll notice that the API is much simpler and strongly-typed than the dbus-rs one that you pointed to.

Please feel free to re-open if you disagree.

In GitLab by @Alxandr on Oct 23, 2022, 23:29

I disagree - because the args-type has a lifetime, so I can't easily send it through a channel. I'm using zbus in a actor system where I have a single actor that receives the signal, and then routes it based on it's type. I would really like to be able to convert the Message to a SomeSignal, before sending it to the actor - if not I have to create my own wrapper types for each signal type.

You can send the message and convert the message to signal arguments on the receiving side.

In GitLab by @Alxandr on Oct 24, 2022, 24:49

Yes - but then I can't differentiate on type - or I have to duplicate the logic. My current code looks something like this:

// one end
match (signal_iface, signal_member) {
  ("Iface", "Signal") => send(SignalArgs::try_parse(message)?) // doesn't work - can't send between threads,
  ...
}

// actor end
impl Handle<SomeSignal> for Actor {
  ....
}

iml Handle<OtherSignal> for Actor {
  ....
}

I would have to run the same switch logic on both end of the pipe if I can just send a message. Or I have to create my own MessageWrapper - which basically does the exact same thing as the Signal-struct, except that I'm able to construct it myself.

I've no objection on adding API to convert to Option<Signal> btw so feel free to reopen your PR (or open a new one) and convert it to that.

It's just that I think this specific issue is resolved.

In GitLab by @Alxandr on Oct 24, 2022, 24:52

Ah - in that case, I'll do just that. It'll probably be in about a week though as my vaccation starts then.

Cool, thanks. Np and enjoy your vacation! 👍

In GitLab by @Alxandr on Oct 27, 2022, 17:28

Do you have any thoughts about the naming of said convert function? If we're converting using Option, I don't think we can use any of the builtin traits due to orphan rules, so it would likely have to be something like:

impl SomeSignal {
  // what do we call this?
  fn new(msg: Arc<Message>) -> Option<Self> { ... }
}

Do you have any thoughts about the naming of said convert function? If we're converting using Option, I don't think we can use any of the builtin traits due to orphan rules

True.

fn new(msg: Arc) -> Option { ... }

This isn't so bad actually but maybe a more explicit from_message is better?

In GitLab by @Alxandr on Oct 28, 2022, 10:27

Should I also add a from_message_unchecked, that just does the conversion? That will allow you to construct a signal from something that just has a body.

I don't think there is a need for that. It'll have to be marked unsafe and I'd need more convincing about the use case for adding unsafe APIs.

In GitLab by @Alxandr on Oct 29, 2022, 16:46

It wouldn't have to be marked unsafe at all actually. Given that getting the props returns a Result, there would be no safety concerns with it, as that's where the "real" validation happens anyways. And since that's already the case, you can never really be sure that any given SomeSignal is a valid SomeSignal.

That being said, I only proposed the API cause it feels somewhat typical of similar rust APIs - not cause I feel it's terribly important.

I guess I didn't quite understand what API exactly you are proposing but since it has _unchecked in the name, I assumed it'll be unsafe.

That being said, I only proposed the API cause it feels somewhat typical of similar rust APIs - not cause I feel it's terribly important.

Up to you. If you want to push for the API, I'll need to see the signature and some examples of similar Rust APIs. :)

dbus2 / zbus-old

API for converting Message to dbus-proxy generated signal types #276