GNOME Bugzilla – Bug 727431
Merge common factories' code into a superclass
Last modified: 2015-06-17 14:22:29 UTC
Merge all common code of the EData{Cal,Book}Factory into a EDataFactory. All methods that help to manage the lifetime of the factories and backends are now into a superclass. A few methods were kept as they were to avoid a spaghetti code, as the _open () methods of calendar/addressbook factories. Milan, let me know your thoughts about this patch while I'm running/writing the tests for this.
Created attachment 273398 [details] [review] Bug #727431 - Merge common factories' code into a superclass
Review of attachment 273398 [details] [review]: Just merge also those open handlers in factories, as we spoke on IRC, with some follow-up merges.
Review of attachment 273398 [details] [review]: Make sure you bump the libebackend soname in configure.ac since you're breaking the library's binary interface of EDataFactoryClass. Or you could avoid the ABI break by appending all the new members -- including 'prefix' and 'object_path' -- BELOW the 'backend_created' method pointer and then deducting 3 slots from the "reserved" pool in the class struct. That way the EDataFactoryClass struct size remains unchanged and you would not disrupt the byte offsets of the existing struct members. Also please document the new public functions in e-data-factory.c and add the function names to the docs/reference/eds/eds-sections.txt file.
Created attachment 273526 [details] [review] Bug #727431 - Merge common factories' code into a superclass
Review of attachment 273526 [details] [review]: I've got an error when opening a personal addressbook on an addressbook factory console: > e-data-server-CRITICAL **: e_source_registry_ref_source: assertion 'E_IS_SOURCE_REGISTRY (registry)' failed I guess it's because a long delay when I run the factory from the terminal with no other evolution processes running. The delay takes several seconds. I see the correct source-registry-server is running, but the factory probably doesn't know that, and has assigned the priv->registry to NULL. There might help to use the initable interface for that, as I noted below. The eds/tests/libebook/client/test-book-client-add-contact test fails to start the source registry, but it might be similar issue as with the addressbook factory. Please verify that `make check` passes the same as it does before your changes. It currently fails for me in tests/test-server-utils. ::: addressbook/libedata-book/e-data-book-factory.c @@ +80,1 @@ + priv = E_DATA_BOOK_FACTORY_GET_PRIVATE (server); I'm not a fan of overusing this macro, there used to be code-clean-up effort to get rid of its usage, because it's less efficient than typecast the object to the right type and use factory->priv instead. I would change it whenever possible (in the files you changed). @@ +111,2 @@ static gchar * +data_book_get_object_path (EDataFactory *data_factory, The function name suggests that it would just return some string, instead of doing any time-expensive operations, like the backend open. Why not keep it data_book_factory_open(), and the base class' virtual method simply EDataFactoryClass::open, instead of EDataFactoryClass::get_object_path? Alternatively EDataFactoryClass::data_open() would correlate with EDataFactoryClass::data_object_path_prefix variable. ::: calendar/libedata-cal/e-data-cal-factory.c @@ +56,3 @@ ((obj), E_TYPE_DATA_CAL_FACTORY, EDataCalFactoryPrivate)) +#define HANDLE_OPEN_CB(extension_name, minus) \ Move this whole block together with its functions (which use it) below, where the previous functions were defined. The #define itself can be just before the functions (like it is in this block), but it should be way lower in the source code. The common practice in eds/evo code is to have soem simple generic defines at the top, then the Private structure definition, then the type definition macro with some forward declarations (if any), and only then start with the real functions. You broke this pattern by this. @@ +168,3 @@ } + g_clear_object (&data_cal); You do not call this in the book factory code. ::: libebackend/e-data-factory.c @@ +122,3 @@ + e_dbus_server_quit (server, E_DBUS_SERVER_EXIT_NORMAL); + g_error_free (error); + } The previous implementation (book and cal) has g_assert_not_reached() here. Not that I want to use g_assert() here, but it doesn't feel right to call parent class' bus_acquired(), when it was not done. I know you took this code from the source registry. @@ +351,3 @@ + g_object_unref (priv->registry); + priv->registry = NULL; + } equivalent of g_clear_object (&priv->registry); @@ +423,3 @@ +GDBusInterfaceSkeleton * +e_data_factory_get_dbus_interface_skeleton (EDataFactory *factory); is this some left-over? I do not see it used anywhere. @@ +512,3 @@ + + data_factory->priv->registry = e_source_registry_new_sync ( + NULL, NULL); Could you try to define the EDataFactory as an initable and assign the priv->registry in its init, please? If nothing else, then providing the actual error to the caller is a good thing to do. I know the descendants already define the initable interface, thus maybe it'll be possible for this abstract type as well. @@ +693,3 @@ +data_factory_toggle_notify_cb (gpointer data, + GObject *backend, + gboolean is_last_ref) incorrect indent of arguments @@ +759,3 @@ + * + * Since: 3.14 + **/ not two spaces at the beginning of the line, but only one. This is repeating multiple times below as well. @@ +767,3 @@ + GHashTableIter iter; + gpointer key, value; + please do safety checks first in public API (and even in private API, but that is not that crucial there), like check whether the passed-in data_factory is an EDataFactory. @@ +775,3 @@ + while (g_hash_table_iter_next (&iter, &key, &value)) { + GPtrArray *array = (GPtrArray *) value; + gint i; no need to type-cast above; I also got use to 'ii' instead of 'i', as Matthew begun to use in the code some time ago (basically double-letter instead of single-letter variable names, it is easier for searching). @@ +852,3 @@ + return g_strdup_printf ( + "%s/%d/%u", + class->data_object_path_prefix, getpid (), counter); safety-check for class->data_object_path_prefix != NULL please ::: libebackend/e-source-registry-server.c @@ +695,3 @@ + auth_object_path = e_data_factory_construct_path ( + E_DATA_FACTORY (server)); no need to wrap here ::: libebackend/e-user-prompter-server.c @@ +375,3 @@ + g_dbus_interface_skeleton_unexport ( + G_DBUS_INTERFACE_SKELETON (priv->dbus_prompter)); As long as the data_factory_bus_acquired() calls g_dbus_interface_skeleton_export(), the data_factory_quit_server() should call g_dbus_interface_skeleton_unexport(), to be consistent between factories (EDataFactory descendants).
Created attachment 273700 [details] [review] Bug #727431 - Merge common factories' code into a superclass
Created attachment 273714 [details] [review] Bug #727431 - Merge common factories' code into a superclass
Created attachment 273753 [details] [review] Bug #727431 - Merge common factories' code into a superclass
Review of attachment 273753 [details] [review]: Please fix the below mentioned minor things and fix the user prompter, it currently crashes with: > (evolution-user-prompter:5546): GLib-GObject-WARNING **: invalid class cast from 'EUserPrompterServer' to 'EDataFactory' This happens when I just run it from a terminal: $PREFIX/libexec/evolution-user-prompter One thing as a follow-up, please check why, when the client side is killed, the factory (both book and calendar) properly finishes, but the affected (now without its client side) EBackend-s are not properly shut down. The backends usually notify about its shut down on console with message like this: > The EBookBackendWebdav instance for "Yahoo!" is shutting down. but not those freed by the client side close. The factory properly identifies this "unexpected client close", because it closes itself after the timeout. ::: addressbook/libedata-book/e-data-book-factory.c @@ +202,2 @@ + if (priv->dbus_factory != NULL) + g_clear_object (&priv->dbus_factory); Check the doc for g_clear_object(), the 'if' is not needed here (and on other places in the code). ::: addressbook/libedata-book/e-data-book.c @@ +279,2 @@ case E_DATA_BOOK_STATUS_NO_SUCH_BOOK: + case G_IO_ERROR_NOT_FOUND: Hmm, do not do it this way. The pair GError::<domain, code> uniquely identifies the error by number(s). The GError::code can (and does) clash between GError::domain-s. The thing you need is g_error_matches (). I would go that far that I would add before the: 238 if (error->domain != E_DATA_BOOK_ERROR) 239 return; simple check: /* Data-Factory returns common error for unknown/broken ESource-s */ if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) { error->domain = E_DATA_BOOK_ERROR; error->code = E_DATA_BOOK_STATUS_NO_SUCH_BOOK; } and keep the rest as is. Similar for the calendar part. ::: libebackend/e-data-factory.c @@ +827,3 @@ + for (ii = 0; ii < array->len; ii++) { + EBackend *backend = g_ptr_array_index (array, ii); + backends = g_slist_prepend (backends, backend); It would be safer to add referenced backends in the GSList and let the caller free the list with g_slist_free_full (backends, g_object_unref); (which should be written in the comment above the function). It'll make the function more thread-safe. @@ +912,3 @@ + * @connection: a #GDBusConnection + * @sender: a string + * @uid: argument passed by remote caller * @uid: UID of an #ESource to open @@ +913,3 @@ + * @sender: a string + * @uid: argument passed by remote caller + * @extension_name: an extension name (that represents an #ESource) to be accurate, it doesn't represent an ESource @@ +918,3 @@ + * Returns the object path of the installed #EData{Book,Calendar}. + * If the backend doesn't have the #EData{Book,Calendar} installed, + * then it will be created and installed as well. Do not mention the descendants, the parent class may not know anything about them. What if we call the function e_data_factory_open_backend() (instead of quite general e_data_factory_open()) and then mention the "EBackend data D-Bus object path".
Created attachment 273843 [details] [review] Bug #727431 - Merge common factories' code into a superclass
(In reply to comment #9) > One thing as a follow-up, please check why, when the client side is killed, the > factory (both book and calendar) properly finishes, but the affected (now > without its client side) EBackend-s are not properly shut down. The backends > usually notify about its shut down on console with message like this: > > The EBookBackendWebdav instance for "Yahoo!" is shutting down. > but not those freed by the client side close. The factory properly identifies > this "unexpected client close", because it closes itself after the timeout. Testing here with Evolution, I have the same behavior with or without my patch. Could you, please, give me a step by step to test it? Are you sure you don't have same behavior with git master?
(In reply to comment #11) > Testing here with Evolution, I have the same behavior with or without my patch. > Could you, please, give me a step by step to test it? Are you sure you don't > have same behavior with git master? Oh yes, the git master works the same, and it's wrong. I thought it's understood after our IRC chat, where I gave you a scenario. Let's make it more concrete. a) run evolution-calendar-factory (will be "the factory" since now on) * its state is no backend is opened b) have application A, which opens On This Computer/Personal calendar * the factory state is: one opened backend for the local calendar c) have application B, which opens On This Computer/Personal calendar and task list * the factory state is: two opened backends, local calendar has two clients, local memo list has one client d) let the application B crash, or it forgets to properly free the ECalClient structure * the _expected_ factory state is: the local memo list backend is closed, because it lost its client; the local calendar is left opened, with one client As I mentioned earlier, maybe it works this way, but I would expect to see the message about "shutting down" for the closed backends. If the message is not shown, then it might mean that the backend is kept alive in the background with no clients connected to it. It can be both good and bad. I consider it bad right now, I would prefer to see backends with no clients being automatically closed, the same as they are when you run the factory. Keep them alive without actual client feels like a memory leak.
Review of attachment 273843 [details] [review]: Looks fine. Please commit this version to master and deal with the above comment in a separate commit (and maybe even a bug report), unless you know what to do and you are able to fix it before the commit. Thanks.
Milan, I've pushed the patch as it is and I'll keep working on the follow-up(s) from here, so I'm not closing this bug.
Created attachment 275089 [details] [review] EBackend: Fix view leak when the client crashes When the client crashes, we have to remove the views from the backend before unref the backend itself.
Review of attachment 275089 [details] [review]: Please see the comments below. ::: calendar/libedata-cal/e-cal-backend.c @@ +729,3 @@ +{ + GList *list, *l; + list = e_cal_backend_list_views (E_CAL_BACKEND (backend)); coding style: missing empty line before the assignment (in both implementations) ::: libebackend/e-data-factory.c @@ +201,3 @@ + class = E_BACKEND_GET_CLASS (backend); + + class->close_views (backend); ehm, a) let's call the virtual function: prepare_shutdown() b) do not call the class function this way, rather create an e_backend_prepare_shutdown() and call that, which will also check whether the function is assigned or not c) document the function and its intention and when it is called d) the EBackend can have implemented an empty prepare_shutdown(), thus the subclass can call the parent class' method too (chain-up like with GObject::dispose or similar virtual functions) e) also, what if I have two applications connecting to the same backend and only one leaks the view/client? It's not correct to close views also for the still-alive connection @@ +202,3 @@ + + class->close_views (backend); + g_ptr_array_remove_fast (array, backend); I would keep there the g_ptr_array_set_size (array, 0); it feels quicker and correct, just do it after the 'for'. You may also use 'ii' instead of 'i'.
(In reply to comment #16) > a) let's call the virtual function: prepare_shutdown() As you might probably need to pass the 'connection' to the prepare_shutdown(), then more appropriate function name would be 'connection_vanished', thus the EBackend descendant may take care of the clean-up for objects being tight to that connection (in our case 'views').
Created attachment 275332 [details] [review] EBackend: Fix view leak when the client crashes Remove the views from the closed/crashed backend when it is not being used by any other connection.
Review of attachment 275332 [details] [review]: The data_factory_connections_remove_all() should call prepare_shutdown() for each opened backend (once, not multiple times). It reminded me that e_data_factory_list_backends() can return one backend multiple times in the list. ::: addressbook/libedata-book/e-book-backend.c @@ +602,3 @@ + g_list_free_full (list, g_object_unref); + + /* Chain up to parent's finalize() method. */ is it finalize() method? ::: calendar/libedata-cal/e-cal-backend.c @@ +740,3 @@ + g_list_free_full (list, g_object_unref); + + /* Chain up to parent's finalize() method. */ is it finalize() method? ::: libebackend/e-backend.c @@ +1118,3 @@ + * unref its view properly. + * This function must be called only by the last instance of the used @backend, + * otherwise it will clean up the objects for still alive backends. What about: Let's the @backend know that it'll be shut down shortly, no client connects to it anymore. The @backend can free any resources which reference it, for example the opened views. @@ +1130,3 @@ + + class = E_BACKEND_GET_CLASS (backend); + g_return_if_fail (class->prepare_shutdown); May write '!= NULL' here. ::: libebackend/e-data-factory.c @@ +173,3 @@ +static gboolean +data_factory_verify_backend_is_used (EDataFactory *data_factory, + const gchar *name, name it 'except_bus_name'. @@ +186,3 @@ + + connections = data_factory->priv->connections; + names = g_hash_table_get_keys (connections); memory leak @@ +188,3 @@ + names = g_hash_table_get_keys (connections); + + for (l = names; l != NULL && is_used != TRUE; l = g_list_next (l)) { Do not compare to TRUE. Aka use '!is_used' instead.
Created attachment 275395 [details] [review] EBackend: Fix view leak when the client crashes Remove the views from the closed/crashed backend when it is not being used by any other connection.
Review of attachment 275395 [details] [review]: It works pretty well. Thanks for it. I've only one last request: ::: libebackend/e-data-factory.c @@ +894,3 @@ for (ii = 0; ii < array->len; ii++) { EBackend *backend = g_ptr_array_index (array, ii); + if (g_slist_find (backends, backend) == NULL) This is awfully expensive, please use GHashTable instead (but still return GSList).
Created attachment 275403 [details] [review] EBackend: Fix view leak when the client crashes Remove the views from the closed/crashed backend when it is not being used by any other connection.
Review of attachment 275403 [details] [review]: That's it. Please commit to master. Thanks.
Pushed as cd3a094 (3.13.2+).
There was a regression after these changes, filled as bug #751108.