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

2011-06-02 Thread Patrick Ohly
On Fr, 2011-05-27 at 11:59 -0400, Matthew Barnes wrote:
 On Fri, 2011-05-27 at 16:49 +0200, Milan Crha wrote: 
  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.)
 
 Can't we just consider it a programming error for a D-Bus method to be
 called synchronously from the main thread?  I don't see any reason to
 support this case since it would be by definition a bug.

I beg to differ. A command line tool like SyncEvolution is perfectly
usable with blocking the main thread in the synchronous API calls. Or
rather, it works as good as the synchronous API implementation.

One problem are timeouts. The other is that the calls don't detect when
the server dies because the corresponding D-Bus signal is not delivered
(seen in older EDS releases, not sure how relevant it is on master).

As Alexander said, the synchronous API serves a useful purpose.
SyncEvolution is one, simple tests another. +1 for keeping it and fixing
it so that it really works.

-- 
Bye, Patrick Ohly
--  
patrick.o...@gmx.de
http://www.estamos.de/


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-31 Thread Alexander Larsson
On Tue, 2011-05-31 at 05:50 +0200, Milan Crha wrote:
 On Mon, 2011-05-30 at 15:33 +0200, Alexander Larsson wrote:
  Btw, I was thinking about this. Is this kind of solution really
  needed? I mean, its easy to bump the timeout for the async ops, and i
  think the current maximum dbus timeout is six hours. Are there actual
  correct evolution operations that we expect to take more than six
  hours to complete?
 
   Hi,
 I'm not aware of any operation which may take more than 6 hours :) The
 default timeout is about 30 seconds, and even after that you cannot
 distinguish whether the factory is just stuck or the backend does other
 operations and your new request is pilled in. The new behaviour lets you
 know that the factory is actually alive, which I believe is a good thing
 to do.

The default timeout yes, but its easy to change that. And how does the
new behaviour let you know this? You know it was alive when it sent the
reply, yes, but it could well have hanged since then.

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-30 Thread Alexander Larsson
On Fri, 2011-05-27 at 16:49 +0200, Milan Crha wrote:

 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.

Btw, I was thinking about this. Is this kind of solution really needed?
I mean, its easy to bump the timeout for the async ops, and i think the
current maximum dbus timeout is six hours. Are there actual correct
evolution operations that we expect to take more than six hours to
complete?

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-30 Thread Milan Crha
On Mon, 2011-05-30 at 15:33 +0200, Alexander Larsson wrote:
 Btw, I was thinking about this. Is this kind of solution really
 needed? I mean, its easy to bump the timeout for the async ops, and i
 think the current maximum dbus timeout is six hours. Are there actual
 correct evolution operations that we expect to take more than six
 hours to complete?

Hi,
I'm not aware of any operation which may take more than 6 hours :) The
default timeout is about 30 seconds, and even after that you cannot
distinguish whether the factory is just stuck or the backend does other
operations and your new request is pilled in. The new behaviour lets you
know that the factory is actually alive, which I believe is a good thing
to do.
Bye,
Milan

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-27 Thread Milan Crha
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

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-27 Thread Milan Crha
On Fri, 2011-05-27 at 13:01 +0200, Alexander Larsson wrote:
 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()).

To be honest, I didn't get this fully. There is 1:1 correspondence
between EBookClient (formerly EBook) and EDataBook, each has its DBus
connection for the other part. The backend invokes those 'done' signals
on a corresponding EDataBook, with its operation ID set, thus the
receiver (though the only EBookClient) checks also the operation ID to
know for what it got the result (and whether the operation itself wasn't
cancelled meanwhile) and based on that finishes the operation. There
might not be a problem you are describing above. I hope.
Bye,
Milan

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-27 Thread Matthew Barnes
On Fri, 2011-05-27 at 16:49 +0200, Milan Crha wrote: 
 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.)

Can't we just consider it a programming error for a D-Bus method to be
called synchronously from the main thread?  I don't see any reason to
support this case since it would be by definition a bug.

In fact I'd go so far as to emit a runtime warning about it and return
immediately we detect that case.  Historically we've been not very good
about this sort of thing, and failing loudly might help flush out any
remaining blocking issues buried in Evolution or other parts of E-D-S.


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-27 Thread Milan Crha
On Fri, 2011-05-27 at 11:59 -0400, Matthew Barnes wrote:
 On Fri, 2011-05-27 at 16:49 +0200, Milan Crha wrote: 
  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.)
 
 Can't we just consider it a programming error for a D-Bus method to be
 called synchronously from the main thread?  I don't see any reason to
 support this case since it would be by definition a bug.
 
 In fact I'd go so far as to emit a runtime warning about it and return
 immediately we detect that case.  Historically we've been not very good
 about this sort of thing, and failing loudly might help flush out any
 remaining blocking issues buried in Evolution or other parts of E-D-S.

Hehe, quite cruel idea, though as long as Evolution itself uses
EBook/ECal, then we may keep it this way, because before such change
will be done the most of evo would be unusable.

But I take your idea and I like it. I only wish we have easier way of
detecting whether the function is called in the main thread or a
dedicated thread, down in the GLib, preferably with
g_thread_is_main(GThread *thread) usually called with thread =
g_thread_self(); because playing with main context ownership seems to me
like a gross hack (and maybe it sometimes even doesn't work, but that's
just my feeling, no proof so far).
Bye,
Milan

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-27 Thread Alexander Larsson
On Fri, 2011-05-27 at 16:49 +0200, Milan Crha wrote:
 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.

I'm running tests/libebook/test-self. Adding a more detailed analysis of
it to:
https://bugzilla.gnome.org/show_bug.cgi?id=651147#c13

  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.)

True, true, they are recieved on a thread and delivered on a mainloop
(via an idle). However can control which mainloop they get delivered to
by calling  g_main_context_push/pop_thread_default() around the dbus
message send, which allows you to create a new main context + loop
(g_main_context_new, g_main_loop_new) with nothing else in, then you can
run this mainloop with g_main_loop_run which is safe since all it will
do is handle the dbus signal.

Anyway, I agree that you shouldn't use sync calls, especially
evolutions. But, if they are there, something will call it. So, either
make the sync calls not reenter, or remove them.


___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers


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

2011-05-27 Thread Alexander Larsson
On Fri, 2011-05-27 at 17:09 +0200, Milan Crha wrote:
 On Fri, 2011-05-27 at 13:01 +0200, Alexander Larsson wrote:
  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()).
 
 To be honest, I didn't get this fully. There is 1:1 correspondence
 between EBookClient (formerly EBook) and EDataBook, each has its DBus
 connection for the other part. The backend invokes those 'done' signals
 on a corresponding EDataBook, with its operation ID set, thus the
 receiver (though the only EBookClient) checks also the operation ID to
 know for what it got the result (and whether the operation itself wasn't
 cancelled meanwhile) and based on that finishes the operation. There
 might not be a problem you are describing above. I hope.

Ah, I see. Each client gets their own object instance in the server, and
we emit the signal on these? Then it should be ok. Although I still
think its a bit weird to use signals (a subscription based multicast
method) to implement a single callback. All that is really needed is to
save the source of the initial method and an object path and then do a
regular dbus method call for the callback.

___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
http://mail.gnome.org/mailman/listinfo/evolution-hackers