[phosphor-logging] About the "Stop emitting Entry propChanged before ifacesAdded" change reason

[Ed Tanous]

On Thu, Nov 4, 2021 at 2:19 PM Patrick Williams <patrick at stwcx.xyz> wrote:
>
> On Thu, Nov 04, 2021 at 12:39:05PM -0700, Ed Tanous wrote:
> > On Wed, Oct 20, 2021 at 10:18 AM Patrick Williams <patrick at stwcx.xyz> wrote:
> > >
> > > On Wed, Oct 20, 2021 at 10:13:06AM -0500, Matt Spinler wrote:
> > > > values, and then explicitly emits the IA signal.   Others can chime in,
> > > > but I didn't see it as proper D-Bus behavior to emit propertiesChanged
> > > > before InterfacesAdded, since in fact no property is changing after the
> > > > interface was added.
> > >
> > > Correct.  PropertiesChanged signals should not show up before InterfacesAdded.
> >
> > But they should still show up eventually (after the InterfacesAdded),
> > right?
>
> I'm not positive what you're asking.  Does it happen or should it be done?  What
> I tried to describe is what correct behavior would look like.
>
> My understanding is that if you don't have a service name, no signals will be
> emitted.  I don't recall where I've seen that in code before.
>
> If you have a service name, but the object does not have an object manager in
> the path, then no InterfacesAdded signals will be emitted.  Many processes put
> this into the root, so this isn't an issue.
>
> PropertiesChanged and InterfacesAdded are independent from a sd_bus perspective
> (and they belong to two different Interfaces anyhow).  If you call
> sd_bus_emit_properties_changed before calling sd_bus_emit_interfaces_added or
> sd_bus_emit_object_added you will still get signals emitted for the properties
> changed (I confirmed this in the systemd repo).
>
> There is no queueing or deferring of the PropertiesChanged signals until after
> the InterfacesAdded.
>
> To me, it does not make any sense to emit signals for PropertiesChanged prior to
> actually informing the world that the interface exists via InterfacesAdded.
> You'll end up sending out signals for properties which nobody even knows they
> exist anyhow.
>
> It is _also_ a bad idea to send out InterfacesAdded signals before you have
> fully constructed and initialized your object.  This can cause other processes
> to act on the property information they received via the InterfacesAdded signal
> but with half-constructed / invalid property state.
>
> > The patchset here seems to be under the impression that
> > PropertyChanged is never emitted when the object is added, which seems
> > incorrect, or at the very least is a breaking behavioral change.
> > https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/48231
>
> The PropertyChanged signal is only emitted if the property changes *after* the
> object was fully formed (by sending out its InterfacesAdded).  The
> PropertyChanged signals are not queued up from the time the object is first
> added until after InterfacesAdded is emitted, which is what it sort of sounded
> like you were implying might happen.
>

My only concern here is that this would require everyone that
implements a Log to implement ObjectManager, which might be
reasonable, but we should document as part of which interfaces
actually REQUIRE ObjectManager.  As written, the above patch would
break if a logging implementation chose to not implement
ObjectManager, which I think is legal in the dbus interfaces today.
This I think is the first place where we would explicitly require an
object manager (maybe we did before for sensors?).

> I'm not sure why you are saying it is breaking behavioral change, other than it
> might have use to work this way, but that way it use to work didn't make any
> sense from a dbus perspective.

I say breaking change as in there was a user facing feature that is
now broken because of this patch.  I'm not making any statement about
which is correct in terms of the dbus interface.  Maybe "regression"
is the better word?

> This proposed change in bmcweb seems
> conceptually reasonable.
>
> --
> Patrick Williams