dbus2 / zbus-old

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

Support for reading cached properties without delay #192

Closed zeenix closed 1 year ago

zeenix commented 3 years ago

In GitLab by @danieldg on Aug 1, 2021, 17:10

It is useful to be able to read the properties of an object in contexts where you cannot incur the delay of a round-trip to the remote party providing the object (for example, while rendering graphics). While property caching provides this behavior, it is currently not obvious (at a type level) if this is enabled, nor is it documented that having caching enabled results in reads always making no round-trips.

Ideally I would like a sync interface (that is, one which does not internally call block_on) that gets a (possibly outdated) value of a cached property. An error like PropertyNotCached could be used to denote that the async or blocking interface must be used (which would presumably need to actually make a call the remote party).

This should be doable by changing the Mutex on the property cache to a std::sync::Mutex (or rwlock) and, if needed, having a separate async Mutex act as a write-lock if you need to serialize changes (though I do not see a need for this - updates are delivered as a Message and don't need any async functions to parse and update).

Current high-level API thoughts: keep async fn property(&self) for the existing behavior and allow adding fn cached_property(&self) for the cached version? It looks like set_ as a prefix is already reserved; cached_ could do the same, or it could be an attribute like #[dbus_proxy(property,cached)] if you think dbus properties called CachedFoo are more common than ones called SetFoo. Obviously only a getter can be cached.

zeenix commented 3 years ago

While property caching provides this behavior, it is currently not obvious (at a type level) if this is enabled, nor is it documented that having caching enabled results in reads always making no round-trips.

Don't think it needs to be encoded in the type system. Just because something is async, doesn't imply a guaranteed I/O operation. However, we can certainly improve the docs to make the caching part super obvious.

Ideally I would like a sync interface (that is, one which does not internally call block_on)

I'm not sure I want more API to work-around a general Rust async/await problem. If you want to read the property in an async context, use async proxies; otherwise just use the sync one that use block_on.

However, if I could be convinced on the need for providing such API, I think your suggested solution sounds good.

This should be doable by changing the Mutex on the property cache to a std::sync::Mutex (or rwlock)

As I wrote on the Matrix recently, I think that probably makes sense anyway.

zeenix commented 3 years ago

Ideally I would like a sync interface (that is, one which does not internally call block_on)

I'm not sure I want more API to work-around a general Rust async/await problem. If you want to read the property in an async context, use async proxies; otherwise just use the sync one that use block_on.

Note that I didn't close the issue. There maybe a chance of convincing me. Also depends on what @elmarco thinks and how strongly he feels about it. :)

This should be doable by changing the Mutex on the property cache to a std::sync::Mutex (or rwlock)

As I wrote on the Matrix recently, I think that probably makes sense anyway.

Could you please add a separate issue for that and set the dependency of this on that one?

zeenix commented 3 years ago

In GitLab by @danieldg on Aug 3, 2021, 02:06

A few reasons:

  1. If #178 is resolved by using overflow detection, then cached properties could be invalidated in some cases. I would expect the async versions of propery() to start blocking on the cache-refresh in that case. That's one reason why cached_ is called out as perhaps being out-of-date.
  2. If a property change notification invalidates a property, then the getter should start blocking again as the cache is re-populated. I'm not sure how this is handled today.
  3. Some properties can't be cached - see org.freedesktop.DBus.Property.EmitsChangedSignal in the dbus spec. In that case, there should not be a cached_ version and the normal getter should block. This is a case where the type system can communicate useful information.
  4. Lazy property caching (#184) is another case where a getter could reasonably either block or immediately return a cached value.
zeenix commented 3 years ago

In GitLab by @danieldg on Aug 3, 2021, 02:10

Could you please add a separate issue for that and set the dependency of this on that one?

I don't think I can add a dependency, but it's #185

zeenix commented 3 years ago

marked this issue as a duplicate of #185

zeenix commented 3 years ago

marked this issue as related to #185

zeenix commented 3 years ago

there is a /duplicate #issue quick action.

zeenix commented 3 years ago

errr.. that does the wrong thing. :) This is the best I could find:

zeenix commented 3 years ago

If a property change notification invalidates a property, then the getter should start blocking again as the cache is re-populated. I'm not sure how this is handled today.

Currently we just clear the cache for the invalidated properties and the next get from the user fetches the value from the dbus peer.

zeenix commented 3 years ago

mentioned in commit cc1dae253e3487bdf17910363a4bcd5882d026bf