After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 670348 - Handle Telepathy CMs crashing/being invalidated
Handle Telepathy CMs crashing/being invalidated
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: Telepathy backend
git master
Other All
: Normal major
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2012-02-18 14:13 UTC by Philip Withnall
Modified: 2012-06-07 17:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Handle TpProxy::invalidated signals in the Telepathy backend (5.35 KB, patch)
2012-02-18 14:16 UTC, Philip Withnall
reviewed Details | Review
Handle TpProxy::invalidated signals in the Telepathy backend (updated) (6.29 KB, patch)
2012-03-24 18:45 UTC, Philip Withnall
committed Details | Review

Description Philip Withnall 2012-02-18 14:13:13 UTC
Folks currently doesn't handle the TpProxy::invalidated signal of any of the TpProxys it uses, such as TpConnection, TpAccount and TpAccountManager. Consequently, folks doesn't notice if (for example) the CM crashes, and will proceed to get an error on the next TpProxy-level call it makes.

Patch coming up which fixes this.
Comment 1 Philip Withnall 2012-02-18 14:16:49 UTC
Created attachment 207922 [details] [review]
Handle TpProxy::invalidated signals in the Telepathy backend

This attempts to fix the problem by handling the ::invalidated signal for TpAccount, TpConnection and TpAccountManager.

At the moment, they have the following behaviour:
 • TpAccount::invalidated removes the Tpf.PersonaStore
 • TpAccountManager::invalidated removes the Tpf.PersonaStore
 • TpConnection::invalidated resets (but doesn't remove) the Tpf.PersonaStore

There's no deep reasoning behind these choices, and they're open to being changed. My basic reasoning was that a Tpf.PersonaStore without a TpConnection is just offline (there's code to handle this already) — but a Tpf.PersonaStore can't exist without a TpAccount or TpAccountManager, since they're needed at construction time.
Comment 2 Guillaume Desmottes 2012-03-19 14:24:55 UTC
Review of attachment 207922 [details] [review]:

::: backends/telepathy/lib/tpf-persona-store.vala
@@ +133,3 @@
+        {
+          this._account = value;
+          this._account.invalidated.connect ((d, c, m) =>

Does Vala automatically disconnect this sig when 'this' is destroyed?
Comment 3 Philip Withnall 2012-03-24 18:45:45 UTC
Created attachment 210525 [details] [review]
Handle TpProxy::invalidated signals in the Telepathy backend (updated)

Here's an updated patch which disconnects signals as appropriate, adds a NEWS entry, and doesn't do everything twice when the connection is disconnected/invalidated.
Comment 4 Philip Withnall 2012-03-24 18:46:42 UTC
(In reply to comment #2)
> Review of attachment 207922 [details] [review]:
> 
> ::: backends/telepathy/lib/tpf-persona-store.vala
> @@ +133,3 @@
> +        {
> +          this._account = value;
> +          this._account.invalidated.connect ((d, c, m) =>
> 
> Does Vala automatically disconnect this sig when 'this' is destroyed?

Technically it does, since Vala uses g_signal_connect_object() almost all the time. However, there's always the bug with g_signal_connect_object() that it leaks signal connections (even if the callback isn't called after object destruction), so I've changed the patch to explicitly disconnect signals.
Comment 5 Philip Withnall 2012-03-24 19:26:05 UTC
Branch here: https://www.gitorious.org/folks/folks/commits/670348-tp-proxy-invalidation
Comment 6 Guillaume Desmottes 2012-03-26 07:45:56 UTC
Review of attachment 210525 [details] [review]:

++
Comment 7 Philip Withnall 2012-03-26 10:32:59 UTC
Comment on attachment 210525 [details] [review]
Handle TpProxy::invalidated signals in the Telepathy backend (updated)

commit 3b3df69cbf0c13e7a6bcee6d4e3945722b45694b
Author: Philip Withnall <philip@tecnocode.co.uk>
Date:   Sat Feb 18 13:55:37 2012 +0000

    telepathy: Handle TpProxy::invalidated signals in the Telepathy backend
    
    If the TpConnection is invalidated (due to the CM crashing), we want to
    reset the state of the TpfPersonaStore (but not remove it). If the
    TpAccountManager is invalidated (due to it crashing), we assume that all
    accounts have been invalidated, and remove all TpfPersonaStores. Same for
    the TpAccount.
    
    Closes: https://bugzilla.gnome.org/show_bug.cgi?id=670348

 NEWS                                          |    1 +
 backends/telepathy/lib/tpf-persona-store.vala |   61 +++++++++++++++++++++----
 2 files changed, 53 insertions(+), 9 deletions(-)
Comment 8 Travis Reitter 2012-06-07 17:11:24 UTC
This may be duplicated by Red Hat bug https://bugzilla.redhat.com/show_bug.cgi?id=810356