GNOME Bugzilla – Bug 698726
Handle goa-daemon crashes/restarts gracefully
Last modified: 2021-05-19 11:03:45 UTC
When goa-daemon crash, EDS removes my google addressbook, and when it is restarted EDS creates a new google addressbook. That means that the ESource's UID is not persistent. Paragraph 4 of GDBusObjectManagerClient's doc[1] says how to distinguish between account removed, and service leaving. I'm not sure if this should be fixed in GOA or in EDS's GOA backend. I don't know how both work together. [1] https://developer.gnome.org/gio/stable/GDBusObjectManagerClient.html#GDBusObjectManagerClient.description
EDS just responds to "account-added" and "account-removed" signals from GoaClient, so my guess is it needs to be fixed in GoaClient. ESourceRegistry in EDS suffers a similar problem: it unconditionally emits "source-added" and "source-removed" signals in response to "object-added" and "object-removed" signals from its GDBusObjectManagerClient without checking the "name-owner" property. Until today I wasn't even aware we were supposed to.
Quoting from https://developer.gnome.org/goa/stable/overview-writing.html: "Applications must not destroy data if an account object is removed (e.g. when the "account-removed" signal is emitted) - for example, if the goa-daemon program crashes or is restarted on software upgrade, account objects will be removed only to be added back the next time goa-daemon is started." That paragraph was written by davidz, who also wrote GDBus, so I will let him respond here.
I did not read the code, so I'm not 100% sure, but GDBusObjectManagerClient gives an hint to distinguish between account-removed and service-exit (see link above), but GoaClient does not give that hint the apps. Is that correct?
s/hint the apps/hint to apps/
The tricky part is GOA exposes the raw GDBusObjects generated by GDBusObjectManagerClient (as GoaObjects), and so it does need to swap out one set of GDBusObjects for another when the goa-daemon restarts, but somehow manage the signal emissions so that applications don't blow away their own GOA-linked accounts in the process. This is a total brainstorm, and probably a bad idea, but the only thing I can think of at the moment is when the goa-daemon restarts, pair the old and new GoaObjects with identical account IDs into a new "account-swapped" signal, which would replace separate "account-added" and "account-removed" signal emissions for those equivalent objects. Then you'd be left with the old GoaObjects which were legitimately removed and the new GoaObjects which were legitimately added.
To clarify -- the "account-swapped" signal would have a signature of: void (*account_swapped) (GoaClient *client, GoaObject *old_object, GoaObject *new_object) The objects would be different incarnations of the same "online account" (in particular, same account ID) -- one from the previous goa-daemon session, one from the newly started session. This would allow applications to update their own data structures without doing anything destructive to their accounts -- like update a hash table entry to replace old_object with new_object.
I don't think it's a good idea to add a new signal. Instead, I think we should just rework the docs to say that GOA applications need to be extraordinarily careful before acting on the ::account-removed signal and then include an example app that does this. For example, like this (untested) static void on_account_removed (GoaClient *client, GoaObject *object, gpointer user_data) { GDBusObjectManagerClient *object_manager; gchar *name_owner; /* Never delete account data in response to GOA daemon restarts / crashes */ object_manager = G_DBUS_OBJECT_MANAGER_CLIENT (goa_client_get_object_manager(client))); name_owner = g_dbus_object_manager_client_get_name_owner (object_manager); if (name_owner != NULL) { delete_account_data(..) } g_free (name_owner); } Of course, this is untested, but it should work...
The thing is the slew of "account-removed" signals will be followed by a slew of "account-added" signals when the goa-daemon restarts, so GOA applications will have to distinguish not only between "account-removed" and "account-removed-but-not-really" events, but also between "account-added" and "account-added-but-not-really" events so as not to end up with a bunch of duplicate accounts. I think it's a bad idea to give the signals double meanings like this.
Downstream bug report with evolution-data-server 3.8: https://bugzilla.redhat.com/show_bug.cgi?id=956908
(In reply to comment #8) > The thing is the slew of "account-removed" signals will be followed by a slew > of "account-added" signals when the goa-daemon restarts, so GOA applications > will have to distinguish not only between "account-removed" and > "account-removed-but-not-really" events, but also between "account-added" and > "account-added-but-not-really" events so as not to end up with a bunch of > duplicate accounts. > > I think it's a bad idea to give the signals double meanings like this. No, there's no double meaning here. The point of the accounts-added and accounts-removed signals are _only_ to inform applications that the local cache of objects has changed. Which enables the application to update its own databases, e.g. models for list views, configuration data on the side etc. etc. So I think your interpretation of accounts-added/accounts-removed is wrong - you think it's about the user adding/removing the account manually while it's really just a way to inform that the local object cache has changed. That's why GOA is careful to state the tip that Debarshi posted in comment 2. It's fairly simple, though, for your application to figure out when a user adds/remove an account - comment 7 is one way... but there are many other (and better) ways to do this. For example, your application could just quit if goa-daemon crashes (and refuse to start if it can't be activated, e.g. no name owner). That's definitely the easiest solution and, I'd argue, it's also the right solution as goa-daemon is never restarted. It's annoying when working on goa-daemon though.. and since it's not a lot of extra work to make your code DTRT across goa-daemon restarts, I would suggest just doing it.
That's exactly the same semantics as GDBusObjectManager, so it leaves me wondering what's the point of GoaClient? I had assumed GoaClient was there to shield applications from these kinds of implementation details within GOA, but it seems to be just an extremely thin wrapper for GDBusObjectManager, offering little benefit over just listening to GDBusObjectManager signals directly. I still think it's pushing too much work on to applications, but I'll comply anyway.
(In reply to comment #11) > That's exactly the same semantics as GDBusObjectManager, so it leaves me > wondering what's the point of GoaClient? > > I had assumed GoaClient was there to shield applications from these kinds of > implementation details within GOA, but it seems to be just an extremely thin > wrapper for GDBusObjectManager, offering little benefit over just listening to > GDBusObjectManager signals directly. > > I still think it's pushing too much work on to applications, but I'll comply > anyway. Well, the point of GoaClient was to add more stuff to it, client-side utility stuff that applications typically use [1]. Maybe it would make sense to add a ::account-deleted signal that fires exactly when the conditions in comment 7 are true? Maybe also a ::account-created that is similar? [1] : For comparison, this is what udisks is doing, see e.g. http://udisks.freedesktop.org/docs/latest/UDisksClient.html where there's a bunch of useful utilities that udisks applications (mostly Disks and gvfs) are using to make their life simpler.
I ended up just writing my own GDBusObjectManager wrapper as a replacement for GoaClient. Mine implements comment #6, and reserves its "account-added" and "account-removed" signals to follow the more intuitive definition. Closing this as FIXED since it was filed specifically for EDS's behavior, but really all GOA-integrated apps need to watch out for this. Fixed for E-D-S 3.9.1 and 3.8.2: https://git.gnome.org/browse/evolution-data-server/commit/?id=725e976e67a62bf00820c70c47a3f439811d6c0d https://git.gnome.org/browse/evolution-data-server/commit/?h=gnome-3-8&id=0e87aafea9a370b6f27ea720729dadb572b03d72
Note that we probably need similar fix in the MissionControl plugin, it would be sad to duplicate that wrapper code... I also think the wrapper has an issue in this case: 1) goa-daemon crash 2) eds restarts 3) goa-daemon starts Because in step 2 EDS lose its orphans table and in step 3 you get the accounts-added signal and duplicate the accounts. I rekon those steps are unlikely. I really think GoaClient/GoaAccount are wrong, the account lifecycle should not be bound to the dbus object lifecycle. Also exposing generated code in public API seems to be a bad idea to me. I would really advise to make wrapper objects that hide the dbus proxies internally. And service restart should be totally transparent to API users. account-swapper signal is really just an awful hack, IMHO.
Reported bug #699198 for the MC plugin (code is in empathy source tree).
(In reply to comment #14) > I also think the wrapper has an issue in this case: > 1) goa-daemon crash > 2) eds restarts > 3) goa-daemon starts > > Because in step 2 EDS lose its orphans table and in step 3 you get the > accounts-added signal and duplicate the accounts. I rekon those steps are > unlikely. Actually I think we're okay, because step 2 should activate the goa-daemon and we'll just match GOA account IDs to EDS accounts as normal. EDS won't destroy its own accounts during that sequence. But it got me thinking -- if step 2 fails to activate the goa-daemon then EGoaClient instantiation will fail. And if the goa-daemon starts up sometime later, EDS won't see it (because we only try to create an EGoaClient once) and EDS will be disconnected from GOA until EDS is restarted again. Perhaps a further improvement is to have EGoaClient *not* be GInitable, and instead create its GDBusObjectManager from a "bus name appeared" callback. That way, even if the online accounts service is not available immediately, EGoaClient will automatically "come online", so to speak, if and when the bus name does appear. So that's another thing GoaClient could be doing for applications to make life easier, instead of forcing applications to watch the bus name themselves.
(In reply to comment #13) > I ended up just writing my own GDBusObjectManager wrapper as a replacement for > GoaClient. Mine implements comment #6, and reserves its "account-added" and > "account-removed" signals to follow the more intuitive definition. I think that's pretty short-sighted and likely to cause problems in the future, or at least maintenance-burden when someone who is not you is trying to fix your code. Yes, the D-Bus API is intended to be public but, really, you are supposed to use the C library. It's a bit anti-social.
(In reply to comment #17) > I think that's pretty short-sighted and likely to cause problems in the future, > or at least maintenance-burden when someone who is not you is trying to fix > your code. Yes, the D-Bus API is intended to be public but, really, you are > supposed to use the C library. It's a bit anti-social. I decided GoaClient is too trivial to argue over, but I deliberately wrote my own wrapper class such that the improvements could easily be merged back into GoaClient if Rishi chooses to. Consider it a fork of that class.
Except that it doesn't actually work. I can get evolution-data-server to crash by running: /usr/libexec/goa-daemon --replace on a GNOME 3.10/Fedora 20 machine. gvfs-goa and telepathy-mission-control also crashes under similar circumstances.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/Community/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/evolution-data-server/-/issues/ Thank you for your understanding and your help.