GNOME Bugzilla – Bug 676825
Implement g_dbus_connection_get_last_serial ()
Last modified: 2012-06-06 17:49:17 UTC
Created attachment 214948 [details] [review] proposed patch In the old libdbus days we were using d-bus message serials quite a lot and were directly dependent on them. The use case was as follows: client side: 1. send a message (start an operation on the remote side), don't wait for the answer 2. get and store serial from the message we've just sent 3. when needed, do a separate call (operation cancellation) with an operation ID equal to the stored serial 4. eventually receive a reply from the message sent at point 1 server side: 1. accept the incoming message, don't send reply yet, keep the reference 2. ... do something 3. accept another incoming call (operation cancellation), get a message serial from the client side 4. look up the running operation with the message serial received, return the first call It was the easiest way to implement operation cancellation without a need for assigning and tracking separate IDs for each op. And now that we're porting old code to GDBus, adding an extra argument with this ID would be an API break. Also, it would bring additional overhead given the operations should be fast as possible and probability of cancellation is very small. After talking to David, an idea to implement similar functionality we had in libdbus was presented. So I'm attaching a patch which implements g_dbus_connection_get_last_serial(). It stores last message serial per-thread and per GDBusConnection instance to reflect its ability to be used from multiple threads at the same time. Implementation notes: the patch is a first shot, due to the fact GPrivate is only static nowadays and we can have multiple GDBusConnection instances in one thread (though not active at the same time), I'm creating separate hash tables for every thread (on a first use). The other (much easier) solution would be to return last serial used in the current thread regardless of the GDBusConnection instance. The g_dbus_connection_get_last_serial() function would not take any arguments then and its name would be misleading as well.
Review of attachment 214948 [details] [review]: Seems like you also need to add an entry to docs/reference/gio/gio-sections.txt to make it show up in the docs. ::: gio/gdbusconnection.c @@ +1616,3 @@ + g_object_weak_ref (G_OBJECT (connection), _gdbus_connection_destroyed, table); + g_hash_table_replace (table, connection, GUINT_TO_POINTER (serial)); + g_hash_table_foreach_remove (data, remove_item_and_weak_ref, data); g_object_weak_ref() isn't thread-safe and we're trying to move away from it in general (by using GWeakRef)... in this case, suggest to just just clean up in GDBusConnection's dispose() method instead @@ +1627,3 @@ + * Serials are stored per-thread for the particular GDBusConnection instance + * to reflect thread safety. It's safe to use @g_dbus_connection_get_last_serial +store_serial_for_connection (GDBusConnection *connection, I would rephrase this to Retrieves the last serial number assigned to a #GDBusMessage on the current thread. This includes messages sent via both low-level API such as g_dbus_connection_send_message() as well as high-level API such as g_dbus_connection_emit_signal(), g_dbus_connection_call() or g_dbus_proxy_call(). ::: gio/gdbusconnection.h @@ +90,3 @@ const gchar *g_dbus_connection_get_unique_name (GDBusConnection *connection); GCredentials *g_dbus_connection_get_peer_credentials (GDBusConnection *connection); +guint32 g_dbus_connection_get_last_serial (GDBusConnection *connection); I think this is missing something like GLIB_AVAILABLE_IN_2_34
Created attachment 215145 [details] [review] proposed patch (In reply to comment #1) > ::: gio/gdbusconnection.c > @@ +1616,3 @@ > + g_object_weak_ref (G_OBJECT (connection), _gdbus_connection_destroyed, > table); > + g_hash_table_replace (table, connection, GUINT_TO_POINTER (serial)); > + g_hash_table_foreach_remove (data, remove_item_and_weak_ref, data); > > g_object_weak_ref() isn't thread-safe and we're trying to move away from it in > general (by using GWeakRef)... in this case, suggest to just just clean up in > GDBusConnection's dispose() method instead I'm using g_object_weak_ref() only as a destroy notify, not storing object references for later use anywhere. This serves the purpose to clean the hashtable for objects going away in all threads. Presuming the g_object_weak_ref() callbacks are called on the threads from where they were subscribed. Which might not be always true... With GWeakRef I don't see a way how to connect a callback when object is going away. I could use GWeakRef to indicate stored pointer validity but that's not exactly what I need. And I'm afraid I can't use object dispose() method either since it runs only on one thread and wouldn't clean data in other threads. Or, maybe there's something I'm missing? > Review of attachment 214948 [details] [review]: Anyway, attaching a new version of the patch for the moment with all other concerns implemented.
Hmm, thinking more about this, I think we should try to avoid GPrivate. How about just doing something like this CONNECTION_LOCK (connection); g_hash_table_insert (connection->hash_thread_to_last_serial, g_thread_self (), GUINT_TO_POINTER (last_serial)); CONNECTION_UNLOCK (connection); when inserting and similar in the get_last_serial() method. Then we can simply nuke this hashtable in finalize and there are no leaks etc. I would also like to see a test-case for this newly added API in form of a new test-case in gio/tests/gdbus-connection.c...
(In reply to comment #3) > Hmm, thinking more about this, I think we should try to avoid GPrivate. How > about just doing something like this > > CONNECTION_LOCK (connection); > g_hash_table_insert (connection->hash_thread_to_last_serial, > g_thread_self (), > GUINT_TO_POINTER (last_serial)); > CONNECTION_UNLOCK (connection); Btw, yes, I'm aware that this means that we'll use one (key, value) pair for every thread from which you call g_dbus_connection_send_message() so this can be a problem if you keep doing this from new threads... in reality, I don't think it's going to be a problem (you are likely to use a thread-pool so threads are reused) but would be good to fix it regardless. It seems like we need a way to easily hook into thread-destruction. Maybe something like g_thread_add_destroy_notify (GThread *thread, GDestroyNotify callback, gpointer user_data); which will add the tupple (callback, user_data) if it hasn't been added already (probably also want g_thread_remove_destroy_notify()). Implementation-wise this will just be called from g_thread_unref(). I haven't thought a bunch about whether this is good API or not... the easiest thing would just to have a GPrivate per GDBusConnection object but the docs specifically say this is not a good idea. I haven't researched why...
The problem is not with the number of threads but rather the number of connections. GPrivate is a limited resource: on MacOS, for example, I think you can only have 128 of them in an entire program (which IIRC is the minimum that POSIX allows). That's not much. GPrivate cannot be destroyed due to the question about where to call the destroy notify on the existing data -- you can't interrupt the thread that the data belongs to and you don't want to destroy thread-local data for another thread from the one doing the destroying. There is no way that we can gain access to the thread-local data of other threads for ourselves even if we wanted to do the destruction locally. I'm not sure how the API that you propose could get you any closer to what you're trying to accomplish. Could you explain?
(In reply to comment #5) > I'm not sure how the API that you propose could get you any closer to what > you're trying to accomplish. Could you explain? Well, I'm not sure I can explain it better than in comment 3, but here goes. In a nutshell, we'd just have a GHashTable per GDBusConnection and whenever we send a message, we do this g_hash_table_insert (connection->hash_thread_to_last_serial, g_thread_self (), GUINT_TO_POINTER (last_serial)); This obviously causes the hash table to grow in size: every time there's a new thread it will contain an additional (key, value) pair. So the thinking is that we want a callback every time a thread is destroyed so we can do something like this static void on_thread_destroyed (GThread *thread, gpointer user_data) { GDBusConnection *connection = G_DBUS_CONNECTION (user_data); g_hash_table_remove (connection->hash_thread_to_last_serial, g_thread_self ()); } We'd register this callback _just_ after the g_hash_table_insert() above like this (calling it multiple time with the same (func, user_data) has no effect) g_thread_add_destroy_notify (on_thread_destroyed, connection); and we'd remove it again in GDBusConnection's dispose method: g_thread_remove_destroy_notify (on_thread_destroyed, connection); (Note that g_thread_remove_destroy_notify() will guarantee that once it returns, there are no (func, connection) callbacks in progress on other threads and no new ones will be scheduled either.)
Talking to mclasen in the office this morning, we decided to just live with leaking (thread_id, last_serial_in_pointer) pairs in the hash_thread_to_last_serial hash-table [1] for now... if this turns out to be a real problem (I doubt it will) we have plenty of time to fix it. Tomas: any chance you can cook up a new patch with this? Thanks! [1] : though, we definitely want a TODO near g_hash_table_insert() mentioning the leak and referencing this bug
(In reply to comment #7) > Tomas: any chance you can cook up a new patch with this? Thanks! I'm on it
(In reply to comment #4) > It seems like we need a way to easily hook into thread-destruction. Maybe > something like > > g_thread_add_destroy_notify (GThread *thread, > GDestroyNotify callback, > gpointer user_data); (In reply to comment #6) > We'd register this callback _just_ after the g_hash_table_insert() above like > this (calling it multiple time with the same (func, user_data) has no effect) > > g_thread_add_destroy_notify (on_thread_destroyed, > connection); > > and we'd remove it again in GDBusConnection's dispose method: > > g_thread_remove_destroy_notify (on_thread_destroyed, > connection); > > (Note that g_thread_remove_destroy_notify() will guarantee that once it > returns, there are no (func, connection) callbacks in progress on other threads > and no new ones will be scheduled either.) This would be really nice to have. The only culprit I see is how we reach other threads to call the function there. Either it would have to take the GThread argument and we would have to manage list of threads we registered on or (preferably) have the callback registered for whole threading system and only spawn it on the thread being currently destroyed with passing a GThread argument in.
Created attachment 215445 [details] [review] proposed patch New patch based on local hash table. Also added a test case for the new feature.
Review of attachment 215445 [details] [review]: Thanks for writing the test, much appreciated. ::: gio/gdbusconnection.c @@ +1689,3 @@ + /* TODO: watch the thread disposal and remove associated record + from hashtable + - see https://bugzilla.gnome.org/show_bug.cgi?id=676825#c7 */ Coding-style: generally, multi-line comments have a star on each line (lined up with the beginning /*) and have the ending */ on a separate line, e.g. /* blabla * blablablabla */ not /* blabla blablablabla */ ::: gio/tests/gdbus-connection.c @@ +1007,3 @@ + /* No calls on this thread yet */ + serial = g_dbus_connection_get_last_serial (c); +serials_thread_func (GDBusConnection *c) Generally, you should use g_assert_cmpint (x, ==, y) instead of g_assert (x == y); since the former will display the value of serial on failure instead of just the string "x == y" See http://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert-cmpint Same comment for other uses of g_assert(). @@ +1020,3 @@ + NULL, + NULL, /* callback */ + g_assert (serial == 0); Here I would use g_dbus_connection_send_messsage() (see below) and pass a non-NULL pointer for the "guint32 *out_serial"... Then I'd add g_usleep (250000); which makes the thread sleep for 0.25 seconds... this should make all the threads send the message at the same time and be done before any of the threads continue processing. This makes the test a lot more realistic / comprehensive since we really do check that get_last_serial() returns something different in each thread. @@ +1024,3 @@ + /* Success on getting the serial */ + serial = g_dbus_connection_get_last_serial (c); + g_dbus_connection_call (c, ... and finally I'd compare the serial obtained above, e.g. g_assert_cmpint (g_dbus_connection_get_last_serial(c), ==, serial_from_method_call); @@ +1163,3 @@ + session_bus_down (); +} + "com.example.Frob", /* interface name */ I appreciate the effort but I think this can be simplified a lot 1. we don't need to bring up a test-service, all messages sent in serials_thread_func() can be signals 2. we only need to test g_dbus_connection_send_message() from each thread, e.g. except for bringing up the bus, the test should simply be starting and joining threads... e.g. the thing is that there is not much point testing call() and emit_signal() since these are trivial wrappers for send_message().
Created attachment 215756 [details] [review] proposed patch (In reply to comment #11) > ::: gio/gdbusconnection.c > @@ +1689,3 @@ > + /* TODO: watch the thread disposal and remove associated record > + from hashtable > + - see https://bugzilla.gnome.org/show_bug.cgi?id=676825#c7 */ > > Coding-style: generally, multi-line comments have a star on each line (lined up > with the beginning /*) and have the ending */ on a separate line, e.g. > > /* blabla > * blablablabla > */ > > not > > /* blabla > blablablabla */ Fixed, having a place with coding style rules written down would be great to have, just like we (used to) have HIG rules. > I appreciate the effort but I think this can be simplified a lot > > 1. we don't need to bring up a test-service, all messages sent in > serials_thread_func() can be signals > > 2. we only need to test g_dbus_connection_send_message() from each thread, > e.g. except for bringing up the bus, the test should simply be starting and > joining threads... e.g. the thing is that there is not much point testing > call() and emit_signal() since these are trivial wrappers for send_message(). I've incorporated the rest of the points, simplified the test but left those few extra calls on the main thread to cover more cases.
Review of attachment 215756 [details] [review]: Looks good to me - thanks!
Cool, thanks for the review! Pushed to master: commit 032e8dabd15133952c7c4f9da05605380b17f79f Author: Tomas Bzatek <tbzatek@redhat.com> Date: Wed Jun 6 19:44:39 2012 +0200 gdbus: Implement g_dbus_connection_get_last_serial() This patch brings an ability to retrieve serial number of the last message sent within the current thread. https://bugzilla.gnome.org/show_bug.cgi?id=676825