mirror of
https://gitlab.gnome.org/GNOME/glib.git
synced 2024-11-09 19:06:15 +01:00
gdbusconnection: Allocate SignalSubscriber structs individually
The `SignalSubscriber` structs contain the callback and `user_data` of each subscriber to a signal, along with the `guint id` token held by that subscriber to identify their subscription. There are one or more `SignalSubscriber` structs for a given signal match rule, which is represented as a `SignalData` struct. Previously, the `SignalSubscriber` structs were stored in a `GArray` in the `SignalData` struct, to reduce the number of allocations needed when subscribing to a signal. However, this means that a `SignalSubscriber` struct cannot have a lifetime which exceeds the `SignalData` which contains it. In order to fix the race in #978, one thread needs to be able to unsubscribe from a signal (destroying the `SignalData` struct) while zero or more other threads are in the process of calling the callbacks from a previous emission of that signal (using the callback and `user_data` from zero or more `SignalSubscriber` structs). Multiple threads could be calling callbacks because callbacks are invoked in the `GMainContext` which originally made a subscription, and GDBus supports subscribing to a signal from multiple threads. In that case, the callbacks are dispatched to multiple threads. In order to allow the `SignalSubscriber` structs to outlive the `SignalData` which contained their old match rule, store them in a `GPtrArray` in the `SignalData` struct, and refcount them individually. This commit in itself should make no functional changes to how GDBus works, but will allow following commits to do so. Signed-off-by: Philip Withnall <withnall@endlessm.com> Helps: #978
This commit is contained in:
Philip Withnall
parent
e1cf40a6b2
commit
9b1c8d7dd5
@ -3248,18 +3248,9 @@ typedef struct
|
||||
gchar *object_path;
|
||||
gchar *arg0;
|
||||
GDBusSignalFlags flags;
|
||||
GArray *subscribers;
|
||||
GPtrArray *subscribers; /* (owned) (element-type SignalSubscriber) */
|
||||
} SignalData;
|
||||
|
||||
typedef struct
|
||||
{
|
||||
GDBusSignalCallback callback;
|
||||
gpointer user_data;
|
||||
GDestroyNotify user_data_free_func;
|
||||
guint id;
|
||||
GMainContext *context;
|
||||
} SignalSubscriber;
|
||||
|
||||
static void
|
||||
signal_data_free (SignalData *signal_data)
|
||||
{
|
||||
@ -3270,10 +3261,37 @@ signal_data_free (SignalData *signal_data)
|
||||
g_free (signal_data->member);
|
||||
g_free (signal_data->object_path);
|
||||
g_free (signal_data->arg0);
|
||||
g_array_free (signal_data->subscribers, TRUE);
|
||||
g_ptr_array_unref (signal_data->subscribers);
|
||||
g_free (signal_data);
|
||||
}
|
||||
|
||||
typedef struct
|
||||
{
|
||||
gatomicrefcount ref_count;
|
||||
GDBusSignalCallback callback;
|
||||
gpointer user_data;
|
||||
GDestroyNotify user_data_free_func;
|
||||
guint id;
|
||||
GMainContext *context;
|
||||
} SignalSubscriber;
|
||||
|
||||
static SignalSubscriber *
|
||||
signal_subscriber_ref (SignalSubscriber *subscriber)
|
||||
{
|
||||
g_atomic_ref_count_inc (&subscriber->ref_count);
|
||||
return subscriber;
|
||||
}
|
||||
|
||||
static void
|
||||
signal_subscriber_unref (SignalSubscriber *subscriber)
|
||||
{
|
||||
if (g_atomic_ref_count_dec (&subscriber->ref_count))
|
||||
{
|
||||
g_main_context_unref (subscriber->context);
|
||||
g_free (subscriber);
|
||||
}
|
||||
}
|
||||
|
||||
static gchar *
|
||||
args_to_rule (const gchar *sender,
|
||||
const gchar *interface_name,
|
||||
@ -3464,7 +3482,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
|
||||
{
|
||||
gchar *rule;
|
||||
SignalData *signal_data;
|
||||
SignalSubscriber subscriber;
|
||||
SignalSubscriber *subscriber;
|
||||
GPtrArray *signal_data_array;
|
||||
const gchar *sender_unique_name;
|
||||
|
||||
@ -3505,17 +3523,19 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
|
||||
else
|
||||
sender_unique_name = "";
|
||||
|
||||
subscriber.callback = callback;
|
||||
subscriber.user_data = user_data;
|
||||
subscriber.user_data_free_func = user_data_free_func;
|
||||
subscriber.id = (guint) g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */
|
||||
subscriber.context = g_main_context_ref_thread_default ();
|
||||
subscriber = g_new0 (SignalSubscriber, 1);
|
||||
subscriber->ref_count = 1;
|
||||
subscriber->callback = callback;
|
||||
subscriber->user_data = user_data;
|
||||
subscriber->user_data_free_func = user_data_free_func;
|
||||
subscriber->id = (guint) g_atomic_int_add (&_global_subscriber_id, 1); /* TODO: overflow etc. */
|
||||
subscriber->context = g_main_context_ref_thread_default ();
|
||||
|
||||
/* see if we've already have this rule */
|
||||
signal_data = g_hash_table_lookup (connection->map_rule_to_signal_data, rule);
|
||||
if (signal_data != NULL)
|
||||
{
|
||||
g_array_append_val (signal_data->subscribers, subscriber);
|
||||
g_ptr_array_add (signal_data->subscribers, subscriber);
|
||||
g_free (rule);
|
||||
goto out;
|
||||
}
|
||||
@ -3529,8 +3549,8 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
|
||||
signal_data->object_path = g_strdup (object_path);
|
||||
signal_data->arg0 = g_strdup (arg0);
|
||||
signal_data->flags = flags;
|
||||
signal_data->subscribers = g_array_new (FALSE, FALSE, sizeof (SignalSubscriber));
|
||||
g_array_append_val (signal_data->subscribers, subscriber);
|
||||
signal_data->subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
|
||||
g_ptr_array_add (signal_data->subscribers, subscriber);
|
||||
|
||||
g_hash_table_insert (connection->map_rule_to_signal_data,
|
||||
signal_data->rule,
|
||||
@ -3560,12 +3580,12 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
|
||||
|
||||
out:
|
||||
g_hash_table_insert (connection->map_id_to_signal_data,
|
||||
GUINT_TO_POINTER (subscriber.id),
|
||||
GUINT_TO_POINTER (subscriber->id),
|
||||
signal_data);
|
||||
|
||||
CONNECTION_UNLOCK (connection);
|
||||
|
||||
return subscriber.id;
|
||||
return subscriber->id;
|
||||
}
|
||||
|
||||
/* ---------------------------------------------------------------------------------------------------- */
|
||||
@ -3575,7 +3595,7 @@ g_dbus_connection_signal_subscribe (GDBusConnection *connection,
|
||||
static void
|
||||
unsubscribe_id_internal (GDBusConnection *connection,
|
||||
guint subscription_id,
|
||||
GArray *out_removed_subscribers)
|
||||
GPtrArray *out_removed_subscribers)
|
||||
{
|
||||
SignalData *signal_data;
|
||||
GPtrArray *signal_data_array;
|
||||
@ -3591,16 +3611,19 @@ unsubscribe_id_internal (GDBusConnection *connection,
|
||||
|
||||
for (n = 0; n < signal_data->subscribers->len; n++)
|
||||
{
|
||||
SignalSubscriber *subscriber;
|
||||
SignalSubscriber *subscriber = signal_data->subscribers->pdata[n];
|
||||
|
||||
subscriber = &(g_array_index (signal_data->subscribers, SignalSubscriber, n));
|
||||
if (subscriber->id != subscription_id)
|
||||
continue;
|
||||
|
||||
/* It’s OK to rearrange the array order using the ‘fast’ #GPtrArray
|
||||
* removal functions, since we’re going to exit the loop below anyway — we
|
||||
* never move on to the next element. Secondly, subscription IDs are
|
||||
* guaranteed to be unique. */
|
||||
g_warn_if_fail (g_hash_table_remove (connection->map_id_to_signal_data,
|
||||
GUINT_TO_POINTER (subscription_id)));
|
||||
g_array_append_val (out_removed_subscribers, *subscriber);
|
||||
g_array_remove_index (signal_data->subscribers, n);
|
||||
g_ptr_array_add (out_removed_subscribers, signal_subscriber_ref (subscriber));
|
||||
g_ptr_array_remove_index_fast (signal_data->subscribers, n);
|
||||
|
||||
if (signal_data->subscribers->len == 0)
|
||||
{
|
||||
@ -3658,13 +3681,13 @@ void
|
||||
g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
|
||||
guint subscription_id)
|
||||
{
|
||||
GArray *subscribers;
|
||||
GPtrArray *subscribers;
|
||||
guint n;
|
||||
|
||||
g_return_if_fail (G_IS_DBUS_CONNECTION (connection));
|
||||
g_return_if_fail (check_initialized (connection));
|
||||
|
||||
subscribers = g_array_new (FALSE, FALSE, sizeof (SignalSubscriber));
|
||||
subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
|
||||
|
||||
CONNECTION_LOCK (connection);
|
||||
unsubscribe_id_internal (connection,
|
||||
@ -3678,15 +3701,15 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection,
|
||||
/* call GDestroyNotify without lock held */
|
||||
for (n = 0; n < subscribers->len; n++)
|
||||
{
|
||||
SignalSubscriber *subscriber;
|
||||
subscriber = &(g_array_index (subscribers, SignalSubscriber, n));
|
||||
SignalSubscriber *subscriber = subscribers->pdata[n];
|
||||
call_destroy_notify (subscriber->context,
|
||||
subscriber->user_data_free_func,
|
||||
subscriber->user_data);
|
||||
g_main_context_unref (subscriber->context);
|
||||
subscriber->user_data_free_func = NULL;
|
||||
subscriber->user_data = NULL;
|
||||
}
|
||||
|
||||
g_array_free (subscribers, TRUE);
|
||||
g_ptr_array_unref (subscribers);
|
||||
}
|
||||
|
||||
/* ---------------------------------------------------------------------------------------------------- */
|
||||
@ -3876,12 +3899,10 @@ schedule_callbacks (GDBusConnection *connection,
|
||||
|
||||
for (m = 0; m < signal_data->subscribers->len; m++)
|
||||
{
|
||||
SignalSubscriber *subscriber;
|
||||
SignalSubscriber *subscriber = signal_data->subscribers->pdata[m];
|
||||
GSource *idle_source;
|
||||
SignalInstance *signal_instance;
|
||||
|
||||
subscriber = &(g_array_index (signal_data->subscribers, SignalSubscriber, m));
|
||||
|
||||
signal_instance = g_new0 (SignalInstance, 1);
|
||||
signal_instance->subscription_id = subscriber->id;
|
||||
signal_instance->callback = subscriber->callback;
|
||||
@ -3954,7 +3975,7 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
|
||||
GHashTableIter iter;
|
||||
gpointer key;
|
||||
GArray *ids;
|
||||
GArray *subscribers;
|
||||
GPtrArray *subscribers;
|
||||
guint n;
|
||||
|
||||
ids = g_array_new (FALSE, FALSE, sizeof (guint));
|
||||
@ -3965,7 +3986,7 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
|
||||
g_array_append_val (ids, subscription_id);
|
||||
}
|
||||
|
||||
subscribers = g_array_new (FALSE, FALSE, sizeof (SignalSubscriber));
|
||||
subscribers = g_ptr_array_new_with_free_func ((GDestroyNotify) signal_subscriber_unref);
|
||||
for (n = 0; n < ids->len; n++)
|
||||
{
|
||||
guint subscription_id = g_array_index (ids, guint, n);
|
||||
@ -3978,15 +3999,15 @@ purge_all_signal_subscriptions (GDBusConnection *connection)
|
||||
/* call GDestroyNotify without lock held */
|
||||
for (n = 0; n < subscribers->len; n++)
|
||||
{
|
||||
SignalSubscriber *subscriber;
|
||||
subscriber = &(g_array_index (subscribers, SignalSubscriber, n));
|
||||
SignalSubscriber *subscriber = subscribers->pdata[n];
|
||||
call_destroy_notify (subscriber->context,
|
||||
subscriber->user_data_free_func,
|
||||
subscriber->user_data);
|
||||
g_main_context_unref (subscriber->context);
|
||||
subscriber->user_data_free_func = NULL;
|
||||
subscriber->user_data = NULL;
|
||||
}
|
||||
|
||||
g_array_free (subscribers, TRUE);
|
||||
g_ptr_array_unref (subscribers);
|
||||
}
|
||||
|
||||
/* ---------------------------------------------------------------------------------------------------- */
|
||||
|
||||
Loading…
Reference in New Issue
Block a user