gdbus-codegen: Ensure that generated skeletons are MT-safe

For example, if setting a property on a skeleton from another thread
than where it was constructed, the idle handler responsible for
emitting the PropertiesChanged() signal could run immediately and
clear skeleton->priv->changed_properties_idle_source causing
g_source_unref() to be called with a NULL pointer. This race was
easily be fixed by adding a lock to the skeleton object.

In addition to fixing this race, also move the code for setting up the
idle handler to a class handler for the GObject::notify signal. This
change allows use of g_object_freeze_notify() and g_object_thaw_notify()
to perform atomic property changes from another thread than the one
that the skeleton was created in.

Signed-off-by: David Zeuthen <davidz@redhat.com>
This commit is contained in:
David Zeuthen 2011-05-15 11:45:37 -04:00
parent 2122191595
commit 11e01802ab
2 changed files with 74 additions and 30 deletions

View File

@ -711,12 +711,17 @@ on_handle_hello_world (MyAppFrobber *object,
<para> <para>
To facilitate atomic changesets (multiple properties changing at To facilitate atomic changesets (multiple properties changing at
the same time), #GObject::notify signals are queued up when the same time), #GObject::notify signals are queued up when
received. The queue is drained in an idle handler and will cause received. The queue is drained in an idle handler (which is called from the
emissions of the <ulink <link linkend="g-main-context-push-thread-default">thread-default main loop</link>
of the thread where the skeleton object was
contructed) and will cause emissions of the <ulink
url="http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties">org.freedesktop.DBus.Properties::PropertiesChanged</ulink> url="http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties">org.freedesktop.DBus.Properties::PropertiesChanged</ulink>
signal with all the properties that have changed. Use signal with all the properties that have changed. Use
g_dbus_interface_skeleton_flush() or g_dbus_interface_skeleton_flush() or
g_dbus_object_skeleton_flush() to empty the queue immediately. g_dbus_object_skeleton_flush() to empty the queue
immediately. Use g_object_freeze_notify() and
g_object_thaw_notify() for atomic changesets if on a different
thread.
</para> </para>
</refsect2> </refsect2>
</refsect1> </refsect1>

View File

@ -1845,6 +1845,7 @@ class CodeGenerator:
' GList *changed_properties;\n' ' GList *changed_properties;\n'
' GSource *changed_properties_idle_source;\n' ' GSource *changed_properties_idle_source;\n'
' GMainContext *context;\n' ' GMainContext *context;\n'
' GMutex *lock;\n'
'};\n' '};\n'
'\n'%i.camel_name) '\n'%i.camel_name)
@ -2054,12 +2055,19 @@ class CodeGenerator:
%(i.name_lower)) %(i.name_lower))
if len(i.properties) > 0: if len(i.properties) > 0:
self.c.write(' %sSkeleton *skeleton = %s%s_SKELETON (_skeleton);\n' self.c.write(' %sSkeleton *skeleton = %s%s_SKELETON (_skeleton);\n'
' gboolean emit_changed = FALSE;\n'
'\n'
' g_mutex_lock (skeleton->priv->lock);\n'
' if (skeleton->priv->changed_properties_idle_source != NULL)\n' ' if (skeleton->priv->changed_properties_idle_source != NULL)\n'
' {\n' ' {\n'
' g_source_destroy (skeleton->priv->changed_properties_idle_source);\n' ' g_source_destroy (skeleton->priv->changed_properties_idle_source);\n'
' skeleton->priv->changed_properties_idle_source = NULL;\n' ' skeleton->priv->changed_properties_idle_source = NULL;\n'
' _%s_emit_changed (skeleton);\n' ' emit_changed = TRUE;\n'
' }\n' ' }\n'
' g_mutex_unlock (skeleton->priv->lock);\n'
'\n'
' if (emit_changed)\n'
' _%s_emit_changed (skeleton);\n'
%(i.camel_name, i.ns_upper, i.name_upper, i.name_lower)) %(i.camel_name, i.ns_upper, i.name_upper, i.name_lower))
self.c.write('}\n' self.c.write('}\n'
'\n') '\n')
@ -2118,11 +2126,13 @@ class CodeGenerator:
self.c.write(' g_source_destroy (skeleton->priv->changed_properties_idle_source);\n') self.c.write(' g_source_destroy (skeleton->priv->changed_properties_idle_source);\n')
self.c.write(' if (skeleton->priv->context != NULL)\n') self.c.write(' if (skeleton->priv->context != NULL)\n')
self.c.write(' g_main_context_unref (skeleton->priv->context);\n') self.c.write(' g_main_context_unref (skeleton->priv->context);\n')
self.c.write(' g_mutex_free (skeleton->priv->lock);\n')
self.c.write(' G_OBJECT_CLASS (%s_skeleton_parent_class)->finalize (object);\n' self.c.write(' G_OBJECT_CLASS (%s_skeleton_parent_class)->finalize (object);\n'
'}\n' '}\n'
'\n'%(i.name_lower)) '\n'%(i.name_lower))
# property accessors (TODO: generate PropertiesChanged signals in setter) # property accessors (TODO: generate PropertiesChanged signals in setter)
if len(i.properties) > 0:
self.c.write('static void\n' self.c.write('static void\n'
'%s_skeleton_get_property (GObject *object,\n' '%s_skeleton_get_property (GObject *object,\n'
' guint prop_id,\n' ' guint prop_id,\n'
@ -2131,11 +2141,13 @@ class CodeGenerator:
'{\n'%(i.name_lower)) '{\n'%(i.name_lower))
self.c.write(' %sSkeleton *skeleton = %s%s_SKELETON (object);\n' self.c.write(' %sSkeleton *skeleton = %s%s_SKELETON (object);\n'
' g_assert (prop_id != 0 && prop_id - 1 < %d);\n' ' g_assert (prop_id != 0 && prop_id - 1 < %d);\n'
' g_mutex_lock (skeleton->priv->lock);\n'
' g_value_copy (&skeleton->priv->properties->values[prop_id - 1], value);\n' ' g_value_copy (&skeleton->priv->properties->values[prop_id - 1], value);\n'
' g_mutex_unlock (skeleton->priv->lock);\n'
%(i.camel_name, i.ns_upper, i.name_upper, len(i.properties))) %(i.camel_name, i.ns_upper, i.name_upper, len(i.properties)))
self.c.write('}\n' self.c.write('}\n'
'\n') '\n')
if len(i.properties) > 0:
# if property is already scheduled then re-use entry.. though it could be # if property is already scheduled then re-use entry.. though it could be
# that the user did # that the user did
# #
@ -2157,6 +2169,8 @@ class CodeGenerator:
' GVariantBuilder builder;\n' ' GVariantBuilder builder;\n'
' GVariantBuilder invalidated_builder;\n' ' GVariantBuilder invalidated_builder;\n'
' guint num_changes;\n' ' guint num_changes;\n'
'\n'
' g_mutex_lock (skeleton->priv->lock);\n'
' g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));\n' ' g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{sv}"));\n'
' g_variant_builder_init (&invalidated_builder, G_VARIANT_TYPE ("as"));\n' ' g_variant_builder_init (&invalidated_builder, G_VARIANT_TYPE ("as"));\n'
' for (l = skeleton->priv->changed_properties, num_changes = 0; l != NULL; l = l->next)\n' ' for (l = skeleton->priv->changed_properties, num_changes = 0; l != NULL; l = l->next)\n'
@ -2195,9 +2209,11 @@ class CodeGenerator:
self.c.write(' g_list_free (skeleton->priv->changed_properties);\n') self.c.write(' g_list_free (skeleton->priv->changed_properties);\n')
self.c.write(' skeleton->priv->changed_properties = NULL;\n') self.c.write(' skeleton->priv->changed_properties = NULL;\n')
self.c.write(' skeleton->priv->changed_properties_idle_source = NULL;\n') self.c.write(' skeleton->priv->changed_properties_idle_source = NULL;\n')
self.c.write(' g_mutex_unlock (skeleton->priv->lock);\n')
self.c.write(' return FALSE;\n' self.c.write(' return FALSE;\n'
'}\n' '}\n'
'\n') '\n')
# holding lock while being called
self.c.write('static void\n' self.c.write('static void\n'
'_%s_schedule_emit_changed (%sSkeleton *skeleton, const _ExtendedGDBusPropertyInfo *info, guint prop_id, const GValue *orig_value)\n' '_%s_schedule_emit_changed (%sSkeleton *skeleton, const _ExtendedGDBusPropertyInfo *info, guint prop_id, const GValue *orig_value)\n'
'{\n' '{\n'
@ -2223,7 +2239,22 @@ class CodeGenerator:
' g_value_init (&cp->orig_value, G_VALUE_TYPE (orig_value));\n' ' g_value_init (&cp->orig_value, G_VALUE_TYPE (orig_value));\n'
' g_value_copy (orig_value, &cp->orig_value);\n' ' g_value_copy (orig_value, &cp->orig_value);\n'
' }\n' ' }\n'
' if (skeleton->priv->changed_properties_idle_source == NULL)\n' '}\n'
'\n'
%())
# Postpone setting up the refresh source until the ::notify signal is emitted as
# this allows use of g_object_freeze_notify()/g_object_thaw_notify() ...
# This is useful when updating several properties from another thread than
# where the idle will be emitted from
self.c.write('static void\n'
'%s_skeleton_notify (GObject *object,\n'
' GParamSpec *pspec)\n'
'{\n'
' %sSkeleton *skeleton = %s%s_SKELETON (object);\n'
' g_mutex_lock (skeleton->priv->lock);\n'
' if (skeleton->priv->changed_properties != NULL &&\n'
' skeleton->priv->changed_properties_idle_source == NULL)\n'
' {\n' ' {\n'
' skeleton->priv->changed_properties_idle_source = g_idle_source_new ();\n' ' skeleton->priv->changed_properties_idle_source = g_idle_source_new ();\n'
' g_source_set_priority (skeleton->priv->changed_properties_idle_source, G_PRIORITY_DEFAULT);\n' ' g_source_set_priority (skeleton->priv->changed_properties_idle_source, G_PRIORITY_DEFAULT);\n'
@ -2231,18 +2262,21 @@ class CodeGenerator:
' g_source_attach (skeleton->priv->changed_properties_idle_source, skeleton->priv->context);\n' ' g_source_attach (skeleton->priv->changed_properties_idle_source, skeleton->priv->context);\n'
' g_source_unref (skeleton->priv->changed_properties_idle_source);\n' ' g_source_unref (skeleton->priv->changed_properties_idle_source);\n'
' }\n' ' }\n'
' g_mutex_unlock (skeleton->priv->lock);\n'
'}\n' '}\n'
'\n' '\n'
%(i.name_lower)) %(i.name_lower, i.camel_name, i.ns_upper, i.name_upper, i.name_lower))
self.c.write('static void\n' self.c.write('static void\n'
'%s_skeleton_set_property (GObject *object,\n' '%s_skeleton_set_property (GObject *object,\n'
' guint prop_id,\n' ' guint prop_id,\n'
' const GValue *value,\n' ' const GValue *value,\n'
' GParamSpec *pspec)\n' ' GParamSpec *pspec)\n'
'{\n'%(i.name_lower)) '{\n'%(i.name_lower))
if len(i.properties) > 0:
self.c.write(' %sSkeleton *skeleton = %s%s_SKELETON (object);\n' self.c.write(' %sSkeleton *skeleton = %s%s_SKELETON (object);\n'
' g_assert (prop_id != 0 && prop_id - 1 < %d);\n' ' g_assert (prop_id != 0 && prop_id - 1 < %d);\n'
' g_mutex_lock (skeleton->priv->lock);\n'
' g_object_freeze_notify (object);\n'
' if (!_g_value_equal (value, &skeleton->priv->properties->values[prop_id - 1]))\n' ' if (!_g_value_equal (value, &skeleton->priv->properties->values[prop_id - 1]))\n'
' {\n' ' {\n'
' if (g_dbus_interface_skeleton_get_connection (G_DBUS_INTERFACE_SKELETON (skeleton)) != NULL)\n' ' if (g_dbus_interface_skeleton_get_connection (G_DBUS_INTERFACE_SKELETON (skeleton)) != NULL)\n'
@ -2250,6 +2284,8 @@ class CodeGenerator:
' g_value_copy (value, &skeleton->priv->properties->values[prop_id - 1]);\n' ' g_value_copy (value, &skeleton->priv->properties->values[prop_id - 1]);\n'
' g_object_notify_by_pspec (object, pspec);\n' ' g_object_notify_by_pspec (object, pspec);\n'
' }\n' ' }\n'
' g_mutex_unlock (skeleton->priv->lock);\n'
' g_object_thaw_notify (object);\n'
%(i.camel_name, i.ns_upper, i.name_upper, len(i.properties), i.name_lower, i.name_lower)) %(i.camel_name, i.ns_upper, i.name_upper, len(i.properties), i.name_lower, i.name_lower))
self.c.write('}\n' self.c.write('}\n'
'\n') '\n')
@ -2259,6 +2295,7 @@ class CodeGenerator:
'{\n' '{\n'
' skeleton->priv = G_TYPE_INSTANCE_GET_PRIVATE (skeleton, %sTYPE_%s_SKELETON, %sSkeletonPrivate);\n' ' skeleton->priv = G_TYPE_INSTANCE_GET_PRIVATE (skeleton, %sTYPE_%s_SKELETON, %sSkeletonPrivate);\n'
%(i.name_lower, i.camel_name, i.ns_upper, i.name_upper, i.camel_name)) %(i.name_lower, i.camel_name, i.ns_upper, i.name_upper, i.camel_name))
self.c.write(' skeleton->priv->lock = g_mutex_new ();\n')
self.c.write(' skeleton->priv->context = g_main_context_get_thread_default ();\n') self.c.write(' skeleton->priv->context = g_main_context_get_thread_default ();\n')
self.c.write(' if (skeleton->priv->context != NULL)\n') self.c.write(' if (skeleton->priv->context != NULL)\n')
self.c.write(' g_main_context_ref (skeleton->priv->context);\n') self.c.write(' g_main_context_ref (skeleton->priv->context);\n')
@ -2281,10 +2318,12 @@ class CodeGenerator:
'\n' '\n'
' gobject_class = G_OBJECT_CLASS (klass);\n' ' gobject_class = G_OBJECT_CLASS (klass);\n'
' gobject_class->finalize = %s_skeleton_finalize;\n' ' gobject_class->finalize = %s_skeleton_finalize;\n'
' gobject_class->get_property = %s_skeleton_get_property;\n' %(i.name_lower, i.camel_name, i.camel_name, i.name_lower))
' gobject_class->set_property = %s_skeleton_set_property;\n'
'\n'%(i.name_lower, i.camel_name, i.camel_name, i.name_lower, i.name_lower, i.name_lower))
if len(i.properties) > 0: if len(i.properties) > 0:
self.c.write(' gobject_class->get_property = %s_skeleton_get_property;\n'
' gobject_class->set_property = %s_skeleton_set_property;\n'
' gobject_class->notify = %s_skeleton_notify;\n'
'\n'%(i.name_lower, i.name_lower, i.name_lower))
self.c.write('\n' self.c.write('\n'
' %s_override_properties (gobject_class, 1);\n'%(i.name_lower)) ' %s_override_properties (gobject_class, 1);\n'%(i.name_lower))
self.c.write('\n' self.c.write('\n'