Created attachment 118471 [details] Reverse notify / emit pairs to fix stale account properties requestedPresence signals before the account object property has been made up-to-date, causing the property to be out of sync when used in conjunction with the signal. This could be causing all sorts of problems to do with stale properties. This patch reverses every notify / signal emit pair to fix this.
I'm not sure I understand parent->notify just emits propertyChanged("propertyName") * Notify that a property named \a propertyName changed. * * This method will emit propertyChanged() for \a propertyName. * * \todo Use for more classes beyond Account. Most importantly Contact. */ void Object::notify(const char *propertyName) { emit propertyChanged(QLatin1String(propertyName)); } As far as I can tell, the only user of this is AccountSet and some unit tests. git grep propertyChanged TelepathyQt/account-set.cpp: SIGNAL(propertyChanged(QString)), TelepathyQt/object.cpp: * This method will emit propertyChanged() for \a propertyName. TelepathyQt/object.cpp: emit propertyChanged(QLatin1String(propertyName)); TelepathyQt/object.h: void propertyChanged(const QString &propertyName); tests/dbus/account-connection-factory.cpp: SIGNAL(propertyChanged(QString)), tests/dbus/account-connection-factory.cpp: SIGNAL(propertyChanged(QString)), As for requested presence; it's set on line 4466 and the emit is on line 4470, so that's always set before the emit.
Created attachment 119368 [details] attachment-17384-0.html Does the notify fetch the updated property in advance? Because reversing the order does fix a stale property issue when immediately fetching the property by the signal, e.g. when fetching the requested presence of a number of accounts in kci global presence, a stale presence can be brought in on a fresh signal and taint the global presence for that global presence change. The reason it doesn't happen every presence change is that the signal property isn't used anyway and the account property isn't yet changed to the new value and the current presence has a higher priority than the expected presence, causing the old presence to be kept. If the stale presence is higher priority when the last account signals this is how we get a stuck presence. Without this patch it is assumed that the signal is always right because it has a property attached and the notify line would seem redundant because it doesn't send any updated property. I like this approach reversed, because -any- signal comes after -any- consideration of the corresponding property, especially if the signal is used to query the property immediately. Either way, I've been using this patch to fix the global presence issue for a while now, it looks correct and doesn't seem to have any regressions. I briefly second-guessed also changing the serviceNameChanged and profileChanged signals, because the above behaviour may have been relied on so that the account was still adressable by these properties until the new property was useable and there doesn't seem to be any reason to rely on these properties to access the account while they are being changed anyway. -------- Original message -------- From: bugzilla-daemon@freedesktop.org Date: 11-02-2015 11:42 AM (GMT-07:00) To: smithjd15@gmail.com Subject: [Bug 92153] Account properties notify signals are dispatched before the account property is updated Comment # 1 on bug 92153 from David Edmundson I'm not sure I understand parent->notify just emits propertyChanged("propertyName") * Notify that a property named \a propertyName changed. * * This method will emit propertyChanged() for \a propertyName. * * \todo Use for more classes beyond Account. Most importantly Contact. */ void Object::notify(const char *propertyName) { emit propertyChanged(QLatin1String(propertyName)); } As far as I can tell, the only user of this is AccountSet and some unit tests. git grep propertyChanged TelepathyQt/account-set.cpp: SIGNAL(propertyChanged(QString)), TelepathyQt/object.cpp: * This method will emit propertyChanged() for \a propertyName. TelepathyQt/object.cpp: emit propertyChanged(QLatin1String(propertyName)); TelepathyQt/object.h: void propertyChanged(const QString &propertyName); tests/dbus/account-connection-factory.cpp: SIGNAL(propertyChanged(QString)), tests/dbus/account-connection-factory.cpp: SIGNAL(propertyChanged(QString)), As for requested presence; it's set on line 4466 and the emit is on line 4470, so that's always set before the emit. You are receiving this mail because: You reported the bug.
Created attachment 128461 [details] [review] Fix stale account properties; tweak the ordering of presence properties. As it happens, some changes are also necessary to reflect that for example a current presence change has taken effect because of a connection change, or that we are in fact changing presence because of a requested presence change. So I've moved the presence-related properties after the connection-related properties, and reordered them so that their signals are sent before any presence property they affect.
Hello James. Sounds reasonable for me. I'll accept this in three days if there will be no objections. Thank you for the work!
Well, my 2 cents... I don't understand the issue at all. And certainly don't understand the patch. Property signals and the notify() signal should be 100% equivalent and the order in which they are emitted shouldn't matter. The actual property value is changed before both of these are emitted, so I don't see why a property would be stale... Regarding the second patch, I would say it is kind of dangerous to depend on the *order* of signals... Perhaps what needs to happen there is to change all the property values before emitting any of the signals, so that the state of the object is correct in all slots connected to these signals.
First patch: As far as I can see, the -signal- property is correct every time. Unfortunately there are times when we can't use the signal property, for example when recomputing an average of identical properties of different accounts. If one of the other accounts is also changing, and the property change is signalled -before- the parent account object is notified, the property still has the old value, and the signal has been dispatched along with the correct property. If we affect an averaging like this on the first account that signals a property change, a stale property is theoretically possible because the parent account object hasn't been notified of any property change. We mostly ignore the property that came with the signal when affecting an average, instead reading the property from any number of accounts starting with the first account that signalled. Second patch: The presence and connection properties are intristically more related to each other than most other properties. More care can be taken towards evaluating certain properties after others. Properties that fit this criteria are ordered such that 'CurrentPresence' is the last property, and all other properties are to precede a property whose changing can indicate a pending property change in itself ('AutomaticPresence' is otherwise related to the presence properties, and so follows 'CurrentPresence'). When 'Connection' changes there is typically a change in 'CurrentPresence'; when 'RequestedPresence' changes there could be changes for both 'ChangingPresence' and 'CurrentPresence', so 'RequestedPresence' should precede 'ChangingPresence' and be somewhere after 'Connection' and before 'CurrentPresence'.
I made a tool to help identify accounts that have stale properties after the account property signal is emitted, it's at git://git.kde.org/scratch/smithjd/tpsignalwatcher.git .
The first patch doesn't for better or worse doesn't change the output appreciably, so the problem must be elsewhere. The second patch does indicate that some property reordering can provide better results. Providing a PresenceChanging property change both before and after connection and presence changes can provide a more workable solution for clients, that can wait for the last signalling account to affect a presence average change of their own. This looks like something for which the spec could possibly be more clear on implementation, e.g. always order PresenceChanging -before- RequestedPresence property in the same AccountPropertyChanged, conversely, send a separate AccountPropertyChanged with the PresenceChanging property -after- the CurrentPresence property. Each individual connection manager could be checked to see if this property is properly ordered and in separate AccountPropertyChanged signals, though it already looks like a required compensation in updateProperties(). For reference: https://telepathy.freedesktop.org/spec/Account.html#Property:ChangingPresence
Created attachment 128535 [details] [review] Fix property signal emission Splits ChangingPresence before Connection and after RequestedPresence, and reorders presence properties.
Created attachment 128601 [details] [review] Reorder connection / presence properties Fix ChangingPresence:false property so it reports before CurrentPresence sends the final presence.
Created attachment 128618 [details] [review] Reorder connection / presence properties This is the simplest fix to the presence / connection property order: RequestedPresence ChangingPresence Connection CurrentPresence The only property to change place is CurrentPresence, now after Connection.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-qt/issues/50.
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.