Re: [Evolution-hackers] Regarding API breakage and lost test cases
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 ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Regarding API breakage and lost test cases
On Thu, 2013-01-31 at 18:25 +0900, Tristan Van Berkom wrote: 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. Note that e_source_registry_commit_source_sync() is just a convenience wrapper. If your test cases are just passing scratch sources then what you're actually calling is e_source_registry_create_sources_sync(). That's the function I fixed in bug 685986. So I guess there are 2 potential ways of addressing this problem: There's a 3rd approach. If you want an immediate client connection to a newly created address book, we could have evolution-addressbook-factory create the ESource on your behalf -- since it too has an ESourceRegistry -- and immediately instantiate a backend for it. That would eliminate the race without resorting to client-side hacks. It would require a new method on the addressbook and calendar factory interfaces, but that is easily done now that all our D-Bus interfaces are generated from XML specifications. That was the purpose of my recent rash of commits to master. Just this week I added e_book_client_connect_sync(), which combines the now-deprecated e_book_client_new() and e_client_open_sync() functions into one step. I could enhance this function to detect whether the given ESource is a scratch source, and invoke a different factory method which behaves as described above. No additional client-side APIs required. To summarize: The current addressbook factory D-Bus API looks like this: method name=OpenAddressBook arg name=source_uid direction=in type=s/ arg name=object_path direction=out type=s/ /method It only works for existing ESources already exported by the registry. I'm proposing to add a new method that takes the raw key file content from a scratch ESource in addition to a UID: method name=CreateAddressBook arg name=source_uid direction=in type=s/ arg name=source_data direction=in type=s/ arg name=object_path direction=out type=s/ /method Invoked by passing a scratch source to e_book_client_connect_sync(). The factory method would call e_source_registry_commit_source_sync() on your behalf, and assuming that goes well, return the object path to its newly-created EBookBackend. With equivalent changes to the calendar factory, of course. Matthew Barnes ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Regarding API breakage and lost test cases
On Thu, 2013-01-31 at 08:11 -0500, Matthew Barnes wrote: On Thu, 2013-01-31 at 18:25 +0900, Tristan Van Berkom wrote: 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. Note that e_source_registry_commit_source_sync() is just a convenience wrapper. If your test cases are just passing scratch sources then what you're actually calling is e_source_registry_create_sources_sync(). That's the function I fixed in bug 685986. So I guess there are 2 potential ways of addressing this problem: There's a 3rd approach. Hi, I vote for Tristan's 2), basically because it's the right approach in client-server architecture, also used in evo-mapi (and maybe in evo-ews, I do not know). Basically, if client doesn't have anything it was asked for, then it doesn't mean that the server doesn't have it too. The other things about the 3) I do not like: a) the point of changing book/cal APIs to not provide XML blobs of sources, but only UIDs was initiated and done by you, if I recall correctly, why to return back then? b) double commits? It might be about unnecessary overhead and double disk write/D-Bus noise too. I guess I got it right, because if it'll be only one commit and not two, then having a book factory creating the ESource, and still use it in the test/client code means that you still wait for a signal from the registry about source-added or whatever, which is basically no gain from the current state, only more complicated (D-Bus) API and giving a decision on the client whether to use one or the other function to open a book. Tristan's 2) can do all this transparently, without any API change on client side, maybe only a little change on the registry D-Bus API (adding function probably, I do not know). Anyway, I believe that keeping things simple is always a gain. Not that it's always possible, but why to confuse people by strange API? (Like those two 'remove' functions, and now proposed two opens.) Bye, Milan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
