Re: [Evolution-hackers] Issues with new EBook dbus implementation

[Milan Crha <mcrha redhat com>]

On Fri, 2011-05-27 at 13:01 +0200, Alexander Larsson wrote:
> I needed the most recent eds to test the folks eds backend, and I picked
> up the new dbus EBook APIs. Unfortunatelly that seemed to be completely
> broken for me (evolution contacts hanged, test-self in the tests
> hanged), so I have debugged it. Turns out that it was sending a dbus
> signal like get_contact_done but the code was expecting a
> getContact_done signal.
> 
> Patches to fix this at:
> https://bugzilla.gnome.org/show_bug.cgi?id=651147

	Hi,
which test did you run? The
   tests/libebook/client/test-client-get-contact
doesn't hang for me. I'm moving to your bug report with further
investigation on this, though on the first look I think of avoiding
camel-cased DBus function names and keep them only with underscores.
It's easier to search/grep for them too.

> However, looking at the internals made me worry about some details.
> 
> First of all, gdbus_proxy_call_sync() when called on the main thread
> will recurse the mainloop. This (mainloop recursion) is considered very
> bad practice and should only be done very explicitly. Unchecked
> reentrancy like this can cause all sorts of weird problems and was the
> main reason bonobo failed.
> 
> For instance, a single call to a sync EBook API can reenter the
> mainloop, handle a gdk destroy event and close the main window of your
> app, or call *any* other apis in the whole app. To correctly handle this
> all internal state must be consistent and after return from any sync
> call you must assume all internal state may have changed. This is not
> something most apps can do, especially when this is happening in some
> lowlevel addressbook api call.
> 
> Furthermore, I don't see why this is required at all. GDBus (for this
> very reason, amongst others) does all its i/o in a worker thread and
> should be able to pump messages even while the main thread is blocking.

Maybe it can pump them, but it doesn't deliver them, as far as my tests
showed, because they are usually delivered on idle, which never happen
with blocked main loop. That's the reason why I added there this loop.
Only notice that this loop is running for "sync" calls, and only if it's
done from the main thread. The async calls and sync calls from a
dedicated thread doesn't do any such thing. You may not use sync calls
in your application, preferably. (Same as evolution shouldn't, which is
subject to change, to use the EBOok/CalClient APIs directly.)

> Secondly, why is it using a dbus signal for the done callbacks? Wouldn't
> it be more sane to use a direct message (without expecting a reply)? A
> signal will be sent to everyone subscribing to that signal, which will
> be all clients talking to the EBook, not just the one that requested the
> operation (i.e. called getContact()).

The new API has two types of functions:
a) "only instructions" - the client instructs backend (server) what
   to do, like "close", "cancel", "authenticate user". These has no
   return value and the client will not wait till they are done.
b) "functions with return values" - the client sends a request to
   backend and waits for its result. It's like the getContact, client
   asks backend for a certain contact (by UID) and backend "returns"
   it back to client within the "getContact_done" signal.
Main reason for this change was a fact that some operations may timeout
on a DBus call, because backend wasn't able to finish the requested
operation in a given time (defined by DBus; later with some workaround
on eds side). It could timeout either because remote server (the one
backend was talking to) didn't respond on time (used to happen most
often with evolution-mapi, for example) or because the backend itself
was busy with other stuff and the requested operation waited for its
processing. New API has this divided into two steps, the first is
invoking the function by simple DBus call, the second is waiting for the
done signal associated with this call. There is no problem with client's
async call, it's all fine. The problem comes with client's sync call, as
it is supposed to stop only after it has the result ready.
	Thanks and bye,
	Milan