Re: [Evolution-hackers] Regarding API breakage and lost test cases

[Tristan Van Berkom <tristanvb openismus com>]

On 10/06/2012 09:01 AM, Matthew Barnes wrote:
> On Fri, 2012-10-05 at 22:19 +0900, Tristan Van Berkom wrote:
> >     a.) e_source_registry_commit_source_sync() seems not exactly
> >         very sync. I haven't looked into that in detail but
> >         surely the registry server needs to block on something else
> >         before sending the reply.
>
> The issue is on the client side in ESourceRegistry.  I had some pretty
> nasty code at one time to deal with this but must have yanked it while
> reworking the APIs.
>
> Even after the remote method call completes, ESourceRegistry will still
> have to stop and wait for an "object-added" signal from its internal
> GDBusObjectManagerClient, which is running in its own isolated thread.
> The "object-added" signal has the new GDBusObject needed to build the
> new ESource instance.  And it can't complete on just any "object-added"
> signal -- it has to be the "object-added" signal corresponding to the
> scratch ESource that was just submitted.
>
> I'll see if I can restore that nastiness again in 3.7.x.

Hi again.

I see there has been an improvement on this (mentioned in
https://bugzilla.gnome.org/show_bug.cgi?id=685986, which
refers to a recent commit trying to address the problem).

I'm not sure what the solution should be exactly, but we
should discuss this a bit because the problem space is
a little bigger than just the source existing in the
client-side before e_source_registry_commit_source_sync()
returns.

While I did modify the test cases to listen to the
"source-added" signal instead of relying on commit_source_sync()
(or the added timeout which I had in place before that), there
is still an interesting race condition to solve that is
observable, but not always reproducing, with the test cases.

Take the following pseudo code for instance:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ESourceRegistry *registry = e_source_registry_new_sync (...);
ESourceBackend *backend;
ESource *scratch = e_source_new_with_uid ("test-address-book", ...);
ESource *source;
EBookClient *book;

backend = e_source_get_extension (scratch, ADDRESS_BOOK);
e_source_backend_set_backend_name (backend, "local");

e_source_registry_commit_source_sync (registry, scratch...);
g_object_unref (scratch);

/* YAY, now we successfully get the source on the client side :) */
source = e_source_registry_ref_source (registry, "test-address-book");

/* ... but the addressbook factory service might not have it yet
 * ... Uh oh... race condition here !
 */
book = e_book_client_new (source, ...);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Basically, the problem is that we hit an assertion from
the data factories when creating a book or calendar.

I.e. e-data-book-factory.c:data_book_factory_open()
(or impl_BookFactory_get_book() in older versions)
goes ahead and also calls e_source_registry_ref_source(),
but we dont know that _that_ process actually has the
ESource yet.

So I guess there are 2 potential ways of addressing this problem:

  a.) The call to e_source_registry_commit_source() needs to postpone
      completion until _all_ connected ESourceRegistry clients also
      receive the new ESource, this way we know that the addressbook
      factory service actually has the ESource before trying to
      create a book for it. (or that the addressbook factory has
      not yet been activated, which is also safe since initially
      creating the ESourceRegistry will load all the existing sources
      before continuing).

  b.) I could envision a sort of wait/refresh explicit call on
      the ESourceRegistry.

      Perhaps even using e_source_registry_ref_source() would be
      good, i.e when calling e_source_registry_ref_source(), if
      the source is not yet in the local cache of sources, it
      could fallback on a D-Bus call to the registry server
      explicitly asking for the given source by UID.

      If a direct D-Bus call to the actual registry service
      indicates that the source doesn't exist, then it surely
      really doesn't exist and an error can be reported from
      e_source_registry_ref_source().

      Successful completion of the e_source_registry_commit_source()
      call would ensure, that any subsequent call to
      e_source_registry_ref_source() would also succeed on the
      same source UID.

Actually, going with the latter alternative might be a good way
of getting rid of the rather complex operation of waiting for
the "source-added" signal before completing the
e_source_registry_commit_source() call.

Cheers,
               -Tristan