Bug 72958 - [SPEC EXTENSION] [PATCH] Introduce new "const" value for org.freedesktop.DBus.Property.EmitsChangedSignal annotation
Summary: [SPEC EXTENSION] [PATCH] Introduce new "const" value for org.freedesktop.DBus...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-12-22 01:46 UTC by Lennart Poettering
Modified: 2014-10-28 13:49 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
spec patch (2.16 KB, patch)
2013-12-22 01:46 UTC, Lennart Poettering
Details | Splinter Review

Description Lennart Poettering 2013-12-22 01:46:23 UTC
Created attachment 91108 [details] [review]
spec patch

For the EmitsChangedSignal annotation to be useful for code-generators to implement client-side property caching we need a fourth possible value: "const". If set this should indicate that the value of the property never changes during the entire lifetime of the object the property is on, thus making caching easy.

Without this programs would resort to "false" for these properties which would force clients to never cache the properties, even though they'd be the perfect candidates.

In systemd we have quite a few properties of this "const" type. They are usually just a 1:1 representation of stuff that was originally read from configuration files on disk and cannot be changed otherwise. When configuration is reloaded or systemd reexecuted all objects disappear (though afterwards they might -- or might not -- reappear), for which separate signals are generated which inform the clients of the complete invalidation of the objects in their entirety.

The attached patch makes this small addition to the spec.

I discussed this a while back with David Zeuthen (who made the original addition to the spec for the annotation) and he agreed that this would be a useful addition to the bus property cache protocol.
Comment 1 Simon McVittie 2014-01-06 15:12:41 UTC
dbus/doc is a trap, the specification is actually dbus/core (Bug #67034) and I don't get bugmail for dbus/doc.

Either invalidates or true would be entirely valid for these properties too: the signal is emitted whenever they change (because a false predicate implies anything: it's equally valid to say "your computer catches fire whenever they change"). The patch seems fine, though.
Comment 2 Lennart Poettering 2014-01-16 16:24:42 UTC
(In reply to comment #1)

> Either invalidates or true would be entirely valid for these properties too:
> the signal is emitted whenever they change (because a false predicate
> implies anything: it's equally valid to say "your computer catches fire
> whenever they change"). The patch seems fine, though.

Well, from the client side code generator's view "true", "const" and "invalidates" could probably be treated the same way. But for the same reason we have seperate values "true" and "invalidates" already we should also distuingish "const", since server-side code generators do or could care. (libsystemd-bus cares, for example).

Does your "patch seems fine" mean that you'd be Ok if I push this?
Comment 3 Simon McVittie 2014-01-16 16:58:06 UTC
Please hold off until next week, at least - I'm about to do a 1.8.x stable-branch, so dbus master is somewhat frozen.

There are two considerations here:

a) we should give server-side code generators a hint that the thing is constant (they can not emit a notify_thing_changed() function I suppose?), give client-side code generators a hint that they can cache it (as they could for "true" and perhaps "invalidates", and give documentation readers that information

b) we shouldn't break clients that don't know what to do with a "const" value for this annotation (they would presumably fall back to treating it as "false" which is exactly what we don't want)

With hindsight, this should probably have been several boolean annotations: EmitsChanged, EmitsInvalidated, and now Constant. Then adding new ones would be a simple matter of "adding more of these annotations gives more guarantees".

I would personally lean towards adding a parallel Constant annotation, which gives us both (a) and (b).

If people don't like that, I'd tolerate a new "const" value, which gives us (a) but breaks (b).

Any opinions from the other maintainers?
Comment 4 Simon McVittie 2014-01-16 17:02:29 UTC
Concrete proposal (slightly pseudocode, I can't remember exactly how the XML works):

<property name="Pi" access="read" type="d">
  <annotation name="org.freedesktop.DBus.Property.Constant"
    value="true"/>
  <!-- Optionally, for backwards compat: this is saying "if this property
       ever changes (which it doesn't) we will emit the signal with
       the value" so that client bindings know they can cache it.
       Client bindings SHOULD NOT require this, but older ones will. -->
  <annotation name="org.freedesktop.DBus.Property.EmitsChangedSignal"
    value="true"/>
</property>

I think "true" is slightly better than "invalidates" for this purpose: you can claim either with a straight face, and a binding that wants to guarantee to get new values synchronously might conceivably ignore "invalidated".
Comment 5 Lennart Poettering 2014-01-16 17:06:26 UTC
(In reply to comment #3)
> Please hold off until next week, at least - I'm about to do a 1.8.x
> stable-branch, so dbus master is somewhat frozen.
> 
> There are two considerations here:
> 
> a) we should give server-side code generators a hint that the thing is
> constant (they can not emit a notify_thing_changed() function I suppose?),
> give client-side code generators a hint that they can cache it (as they
> could for "true" and perhaps "invalidates", and give documentation readers
> that information
> 
> b) we shouldn't break clients that don't know what to do with a "const"
> value for this annotation (they would presumably fall back to treating it as
> "false" which is exactly what we don't want)
> 
> With hindsight, this should probably have been several boolean annotations:
> EmitsChanged, EmitsInvalidated, and now Constant. Then adding new ones would
> be a simple matter of "adding more of these annotations gives more
> guarantees".
> 
> I would personally lean towards adding a parallel Constant annotation, which
> gives us both (a) and (b).
> 
> If people don't like that, I'd tolerate a new "const" value, which gives us
> (a) but breaks (b).

I tried to find code uses this annotation and only found telepathy, and that code doesn't really care much about any new value. Also, I don't think taking an unknown value as "false" wouldn't be much of a problem. I mean, it might make things a bit slower, since client code would not cache the value, but it would still work correctly. And if it took it for "true" or for "invalidates" nothing would happen either.

I'd really like to keep this simple, and just add to the existing annotation rather than a new one... But I am happy with either...
Comment 6 Lennart Poettering 2014-09-03 17:59:59 UTC
Heya, would like to resurrect this, the discussion somehow petered out. I still think the original patch is the best option... ;-)

Simon, any word on this? Would love to just commit this...
Comment 7 Simon McVittie 2014-09-08 13:08:45 UTC
(In reply to comment #3)
> I would personally lean towards adding a parallel Constant annotation, which
> gives us both (a) and (b).

I still think this (Comment #4), but...

> If people don't like that, I'd tolerate a new "const" value, which gives us
> (a) but breaks (b).

... I do see Lennart's point about simplicity, so I'm not going to veto Lennart's patch if people prefer it.

Does anyone else have an opinion? thiago? walters? davidz?
Comment 8 Simon McVittie 2014-10-13 13:01:25 UTC
(In reply to Lennart Poettering from comment #6)
> Heya, would like to resurrect this, the discussion somehow petered out. I
> still think the original patch is the best option... ;-)

I still think my "separate annotation" proposal is the best option, each of us seems to be prepared to tolerate the other's proposal, and apparently nobody else has any opinion whatsoever.

To resolve the deadlock, I've applied your patch - worse is better, and all that. Fixed in git for specification v0.25 (dbus 1.9.2).
Comment 9 Lennart Poettering 2014-10-28 13:49:16 UTC
Thanks for commiting this. sd-bus carries an implementation of this already (though it's not exported officially yet).


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.