Re: [Evolution-hackers] Issues with new EBook dbus implementation
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
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
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
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
[Evolution-hackers] Issues with new EBook dbus implementation
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 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. 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()). ___ 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
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
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
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
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
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
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
