zbus_macros: add support for EmitsChangedSignal annotation on properties #263

Closed zeenix closed 1 year ago

In GitLab by @lucab on Feb 16, 2022, 21:46

https://dbus.freedesktop.org/doc/dbus-specification.html#introspection-format describes a standard org.freedesktop.DBus.Property.EmitsChangedSignal annotation that can be added to a property to control caching behavior in a fine-grained way.

It would be nice to have macro support for something like #[dbus_proxy(property_emits_changed_signal = ...)].

The false variant completely bypasses caching, so it likely needs some changes to Proxy itself first.

Apart from caching itself, macro will need to ensure cached_ getters don't get generated for these props.

It would be nice to have macro support for something like #[dbus_proxy(property_emits_changed_signal = ...)].

Since the only implication is for caching, I think it probably makes sense to keep the api and attribute names specific to that (perhaps there'd be other uses for disabling caching per property other than EmitsChangedSignal). Also would be nice to re-use the property attribute (which the user will have to use as well anyway:

#[dbus_proxy(property(cache = false)]

In GitLab by @lucab on Feb 17, 2022, 10:16

Mmh, I may have mis-characterized it, as it has some further implications other than caching.

The const variant for example affects both caching (enabled) and the receive_<foo>_changed logic (disabled). On false it is the opposite combination disabled+enabled.

The invalidates variant is more subtle, and I'm still not sure how it should be reflected in the generated bindings.

I agree it is nicer to parametrize the property attribute, I wasn't sure if that syntax would be retro-compatible.

The const variant for example affects both caching (enabled) and the receive_<foo>_changed logic (disabled). On false it is the opposite combination disabled+enabled.

Sorry, I don't follow. const variant? What's that?

The invalidates variant is more subtle, and I'm still not sure how it should be reflected in the generated bindings.

Not 100% sure what you're referring to with "variant" but AFAIK invalidation is only relevant to caching: when a property is invalidated, he just clear the cache and next read request fetches the property from the peer. I don't see the relevance to properties that don't emit change notification though.

In GitLab by @lucab on Feb 17, 2022, 12:56

Sorry, I was speaking about the valid values for EmitsChangedSignal: true, invalidates, const, false.

For const, no change notification is never emitted so a receive_<foo>_changed is pointless and can be omitted.

For invalidates, the change notification is emitted but the value is not included in the signal.

Sorry, I was speaking about the valid values for EmitsChangedSignal: true, invalidates, const, false.

Ah, I thought it was just a boolean. :smile:

For const, no change notification is never emitted so a receive_<foo>_changed is pointless and can be omitted.

yeah, it should be.

For invalidates, the change notification is emitted but the value is not included in the signal.

That one we don't actually need to do anything about. Our code handles property invalidation already, even if the property doesn't say this is what it does. IOW I don't think we need to differentiate between true and invalidates cases here. That means, we don't need annotation/support for EmittsChangedSignal itself but rather the ability to disable caching for a specific property.

In GitLab by @lucab on Feb 17, 2022, 19:01

Today I started looking at the zbus_macros code for this.

A couple of shower-thoughts before I forget:

the const case likely requires an additional attribute to skip the receive method; i.e. something like property(cache = true, receive_changed = false))]
the attribute parser is shared with dbus_interface. For the sake of future-proofing, it may be wiser to stick to the specs (using a single enum instead of splitting into bools), so that an hypothetical #[dbus_interface(property(emits_changed_signal = "invalidates"))] could in future generate the relevant code+XML for the server side too.

Today I started looking at the zbus_macros code for this.

:thumbsup:

the const case likely requires an additional attribute to skip the receive method; i.e. something like property(cache = true, receive_changed = false))]

How come? Did you read my previous comment here? As I wrote, unless I missed something, I don't see why we need to cater for "invalidates" variant.

In GitLab by @lucab on Feb 17, 2022, 22:26

Yes I got that reply, thanks for the insights, and I'm already basically ignoring that variant.

Though there are three remaining cases, with the following behaviors:

true: caching enabled :white_checkmark: - `receive_changed` generated :white_check_mark:
false: caching disabled :x: - receive_<prop>_changed omitted :x:
const: caching enabled :white_checkmark: - `receive_changed` omitted :x:

For context, systemd uses all of those (on different properties), that's why I'm trying to model all the behaviors upfront.

Thanks for explaining. I see. Reading the docs, I'm a bit unsure false necessarily mean what we're assuming:

If set to false, the org.freedesktop.DBus.Properties.PropertiesChanged signal, see the section called “org.freedesktop.DBus.Properties” is not guaranteed to be emitted if the property changes.

My understanding of this is that change notification still may be emitted, just not something you can rely on. So I'm a bit unsure about the direction to take here.

In GitLab by @lucab on Feb 18, 2022, 09:49

Fair. The key point is that we can interpret the not guaranteed to be emitted in two ways:

do not generate a receive_<prop>_changed, because that stream will be empty (in the best case) or plainly misleading (in the worst case) as there is no way (without polling) to detect if a notification was NOT emitted.
generate a receive_<prop>_changed (maybe with a specific name suffix to convey the no-guarantees?), even though for most users it will be an unreliable primitive (and a pretty footgun IMHO)

So far I've been leaning toward the first option. Because I'm lazy, so I prefer libraries which do not expose footguns. And because in practice the false cases I've seen in systemd do not emit change notifications (e.g. because they are sourced externally or sampled in real-time).

My guess is that the spec was just written in a broad way to catch all the "do not cache, do not rely on notifications" cases, without further splitting into "guaranteed no notifications" and "sometimes may notify, without guarantees".

But I am open to hear arguments for the second option.

hmm.. it's tricky. I agree with the footgun argument but there is no way to ensure that all services will interpret false in the same way if the spec leaves it open to interpretations. So there could be cases where client code wants the <Property>Changed signals but won't have any way to do so. However, I find this situation a bit unlikely to happen in practice so I'm fine with your suggestion as long as we document this behaviour.

In GitLab by @lucab on Feb 19, 2022, 13:57

Ack. Perhaps the receive methods can be omitted for a start now, and added later if somebody finds a real-world usecase for that.

Going back to the macro syntax discussion, in the end should this stay as a single variant attribute (for later re-use in the server generator), or be split into two boolean attributes?

Ack. Perhaps the receive methods can be omitted for a start now, and added later if somebody finds a real-world usecase for that.

True. It's easily fixable at least.

Going back to the macro syntax discussion, in the end should this stay as a single variant attribute (for later re-use in the server generator), or be split into two boolean attributes?

I agree with what you said above:

For the sake of future-proofing, it may be wiser to stick to the specs (using a single enum instead of splitting into bools), so that an hypothetical #[dbus_interface(property(emits_changed_signal = "invalidates"))] could in future generate the relevant code+XML for the server side too.

mentioned in commit 7afb86c09749dcbd4e8fec0a5eda994d11bbf8f5

dbus2 / zbus-old

zbus_macros: add support for EmitsChangedSignal annotation on properties #263