From 855d286845d72f0da3c0e77c24bce117dcb90af2 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 6 Feb 2014 14:56:38 +0000 Subject: [PATCH] If account storage plugins are GAsyncInitable, initialize them before list() This means they don't have to be prepared to block during list(). --- src/mcd-account-manager-priv.h | 3 -- src/mcd-account-manager.c | 24 ++++----- src/mcd-master.c | 2 - src/mcd-storage.c | 88 +++++++++++++++++++++++++----- src/mcd-storage.h | 7 +++ tests/twisted/dbus-account-plugin.c | 103 ++++++++++++++++++++++++++++-------- tests/twisted/mctest.py | 31 ++++++----- 7 files changed, 191 insertions(+), 67 deletions(-) diff --git a/src/mcd-account-manager-priv.h b/src/mcd-account-manager-priv.h index e89b1d3..b57c89a 100644 --- a/src/mcd-account-manager-priv.h +++ b/src/mcd-account-manager-priv.h @@ -30,9 +30,6 @@ G_BEGIN_DECLS -G_GNUC_INTERNAL void _mcd_account_manager_setup - (McdAccountManager *account_manager); - G_GNUC_INTERNAL GHashTable *_mcd_account_manager_get_accounts (McdAccountManager *account_manager); diff --git a/src/mcd-account-manager.c b/src/mcd-account-manager.c index a221910..9b9eb0e 100644 --- a/src/mcd-account-manager.c +++ b/src/mcd-account-manager.c @@ -1373,26 +1373,20 @@ migrate_accounts (McdAccountManager *self) } } -/** - * _mcd_account_manager_setup: - * @account_manager: the #McdAccountManager. - * - * This function must be called by the McdMaster; it reads the accounts from - * the config file, and it needs a McdMaster instance to be active. - */ -void -_mcd_account_manager_setup (McdAccountManager *account_manager) +static void +storage_loaded_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) { + McdAccountManager *account_manager = MCD_ACCOUNT_MANAGER (user_data); McdAccountManagerPrivate *priv = account_manager->priv; McdStorage *storage = priv->storage; GHashTable *accounts; GHashTableIter iter; gpointer k, v; - /* for simplicity we don't support re-entrant setup */ - g_return_if_fail (priv->setup_lock == 0); - - priv->setup_lock = 1; /* will be released at the end of this function */ + /* it can't fail at the moment; only check it if assertions are enabled */ + g_assert (mcd_storage_load_finish (storage, res, NULL)); tp_list_connection_names (priv->dbus_daemon, list_connection_names_cb, NULL, NULL, @@ -1658,7 +1652,9 @@ _mcd_account_manager_constructed (GObject *obj) NULL); DEBUG ("loading plugins"); - mcd_storage_load (priv->storage); + priv->setup_lock = 1; + mcd_storage_load_async (priv->storage, storage_loaded_cb, + g_object_ref (account_manager)); /* initializes the interfaces */ mcd_dbus_init_interfaces_instances (account_manager); diff --git a/src/mcd-master.c b/src/mcd-master.c index d4b53cf..2858e20 100644 --- a/src/mcd-master.c +++ b/src/mcd-master.c @@ -208,8 +208,6 @@ mcd_master_constructor (GType type, guint n_params, priv->dispatcher = mcd_dispatcher_new (priv->dbus_daemon, master); g_assert (MCD_IS_DISPATCHER (priv->dispatcher)); - _mcd_account_manager_setup (priv->account_manager); - dbus_connection_set_exit_on_disconnect ( dbus_g_connection_get_connection ( tp_proxy_get_dbus_connection (TP_PROXY (priv->dbus_daemon))), diff --git a/src/mcd-storage.c b/src/mcd-storage.c index b2d57dc..c8d35a5 100644 --- a/src/mcd-storage.c +++ b/src/mcd-storage.c @@ -614,23 +614,20 @@ reconnect_cb (McpAccountStorage *plugin, account_name); } -/* - * mcd_storage_load: - * @storage: An object implementing the #McdStorage interface - * - * Load the long term account settings storage into our internal cache. - * Should only really be called during startup, ie before our DBus names - * have been claimed and other people might be relying on responses from us. - */ -void -mcd_storage_load (McdStorage *self) +static void +mcd_storage_load_progress (GTask *task) { + McdStorage *self = MCD_STORAGE (g_task_get_source_object (task)); + gsize remaining = GPOINTER_TO_SIZE (g_task_get_task_data (task)); McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self); GList *store = NULL; - g_return_if_fail (MCD_IS_STORAGE (self)); - - sort_and_cache_plugins (); + if (remaining > 1) + { + /* wait for the rest */ + g_task_set_task_data (task, GSIZE_TO_POINTER (remaining - 1), NULL); + return; + } /* fetch accounts stored in plugins, highest priority first, so that * low priority plugins can be overidden by high priority */ @@ -680,6 +677,8 @@ mcd_storage_load (McdStorage *self) /* already freed the contents, just need to free the list itself */ g_list_free (stored); } + + g_task_return_boolean (task, TRUE); } /* @@ -2357,3 +2356,66 @@ finally: g_strfreev (untyped_parameters); return ret; } + +static void +mcd_storage_loaded_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) +{ + McpAccountStorage *loaded = MCP_ACCOUNT_STORAGE (source_object); + GError *error = NULL; + + if (!g_async_initable_init_finish (G_ASYNC_INITABLE (source_object), + res, &error)) + { + /* don't use that one */ + WARNING ("%s failed to initialize: %s #%d: %s", + mcp_account_storage_name (loaded), + g_quark_to_string (error->domain), error->code, error->message); + stores = g_list_remove (stores, loaded); + } + + /* Whether it was OK or not, we can stop waiting for it. */ + mcd_storage_load_progress (G_TASK (user_data)); + + g_object_unref (user_data); +} + +void +mcd_storage_load_async (McdStorage *self, + GAsyncReadyCallback callback, + gpointer user_data) +{ + GList *store; + GTask *task = g_task_new (self, NULL, callback, user_data); + gsize remaining = 1; + + sort_and_cache_plugins (); + + g_task_set_task_data (task, GSIZE_TO_POINTER (remaining), NULL); + + for (store = stores; store != NULL; store = store->next) + { + if (G_IS_ASYNC_INITABLE (store->data)) + { + remaining++; + g_task_set_task_data (task, GSIZE_TO_POINTER (remaining), NULL); + g_async_initable_init_async (store->data, G_PRIORITY_DEFAULT, + NULL, mcd_storage_loaded_cb, g_object_ref (task)); + } + } + + /* just in case there were no async-initable stores */ + mcd_storage_load_progress (task); + g_object_unref (task); +} + +gboolean +mcd_storage_load_finish (McdStorage *self, + GAsyncResult *result, + GError **error) +{ + g_return_val_if_fail (g_task_is_valid (result, self), FALSE); + + return g_task_propagate_boolean (G_TASK (result), error); +} diff --git a/src/mcd-storage.h b/src/mcd-storage.h index 56732f1..bbeab73 100644 --- a/src/mcd-storage.h +++ b/src/mcd-storage.h @@ -170,6 +170,13 @@ gboolean mcd_storage_maybe_migrate_parameters (McdStorage *self, const gchar *account_name, TpProtocol *protocol); +void mcd_storage_load_async (McdStorage *self, + GAsyncReadyCallback callback, + gpointer user_data); +gboolean mcd_storage_load_finish (McdStorage *self, + GAsyncResult *result, + GError **error); + G_END_DECLS #endif /* MCD_STORAGE_H */ diff --git a/tests/twisted/dbus-account-plugin.c b/tests/twisted/dbus-account-plugin.c index 79464ed..d62136c 100644 --- a/tests/twisted/dbus-account-plugin.c +++ b/tests/twisted/dbus-account-plugin.c @@ -78,6 +78,7 @@ test_dbus_account_free (gpointer p) } static void account_storage_iface_init (McpAccountStorageIface *); +static void async_initable_iface_init (GAsyncInitableIface *); struct _TestDBusAccountPlugin { GObject parent; @@ -95,6 +96,8 @@ struct _TestDBusAccountPluginClass { G_DEFINE_TYPE_WITH_CODE (TestDBusAccountPlugin, test_dbus_account_plugin, G_TYPE_OBJECT, + G_IMPLEMENT_INTERFACE (G_TYPE_ASYNC_INITABLE, + async_initable_iface_init); G_IMPLEMENT_INTERFACE (MCP_TYPE_ACCOUNT_STORAGE, account_storage_iface_init)); @@ -631,19 +634,19 @@ parameters_changed_cb (GDBusConnection *bus, g_variant_unref (deleted); } -static GList * -test_dbus_account_plugin_list (McpAccountStorage *storage, - McpAccountManager *am) +static void get_accounts_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data); + +static void +test_dbus_account_plugin_init_async (GAsyncInitable *initable, + int io_priority, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { - TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage); - GError *error = NULL; - GVariant *tuple, *accounts; - GVariant *attributes, *attribute_flags; - GVariant *parameters, *untyped_parameters, *param_flags; - GVariantIter account_iter; - const gchar *account_name; - GList *ret = NULL; - guint32 restrictions; + TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (initable); + GTask *task; DEBUG ("called"); @@ -695,8 +698,9 @@ test_dbus_account_plugin_list (McpAccountStorage *storage, g_object_ref (self), g_object_unref); - /* list is allowed to block */ - tuple = g_dbus_connection_call_sync (self->bus, + task = g_task_new (self, cancellable, callback, user_data); + + g_dbus_connection_call (self->bus, TEST_DBUS_ACCOUNT_SERVICE, TEST_DBUS_ACCOUNT_SERVICE_PATH, TEST_DBUS_ACCOUNT_SERVICE_IFACE, @@ -705,15 +709,35 @@ test_dbus_account_plugin_list (McpAccountStorage *storage, G_VARIANT_TYPE ("(a{s(a{sv}a{su}a{sv}a{ss}a{su}u)})"), G_DBUS_CALL_FLAGS_NONE, -1, - NULL, /* no cancellable */ - &error); + cancellable, + get_accounts_cb, + task); +} + +static void +get_accounts_cb (GObject *source_object, + GAsyncResult *res, + gpointer user_data) +{ + GTask *task = G_TASK (user_data); + TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN ( + g_task_get_source_object (task)); + GError *error = NULL; + GVariant *tuple, *accounts; + GVariant *attributes, *attribute_flags; + GVariant *parameters, *untyped_parameters, *param_flags; + GVariantIter account_iter; + const gchar *account_name; + guint32 restrictions; + + tuple = g_dbus_connection_call_finish (self->bus, res, &error); if (g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_NAME_HAS_NO_OWNER) || g_error_matches (error, G_DBUS_ERROR, G_DBUS_ERROR_SERVICE_UNKNOWN)) { - /* this regression test isn't using the fake accounts service */ - g_clear_error (&error); - return NULL; + g_task_return_error (task, error); + g_object_unref (task); + return; } self->active = TRUE; @@ -732,12 +756,42 @@ test_dbus_account_plugin_list (McpAccountStorage *storage, test_dbus_account_plugin_add_account (self, account_name, attributes, attribute_flags, parameters, untyped_parameters, param_flags, restrictions); - - ret = g_list_prepend (ret, g_strdup (account_name)); } g_variant_unref (accounts); g_variant_unref (tuple); + + g_task_return_boolean (task, TRUE); + g_object_unref (task); +} + +static gboolean +test_dbus_account_plugin_init_finish (GAsyncInitable *initable, + GAsyncResult *res, + GError **error) +{ + return g_task_propagate_boolean (G_TASK (res), error); +} + +static GList * +test_dbus_account_plugin_list (McpAccountStorage *storage, + McpAccountManager *am) +{ + TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage); + GList *ret = NULL; + GHashTableIter iter; + gpointer k; + + DEBUG ("called"); + + /* in tests where we're not using this plugin, this shouldn't be called */ + g_return_val_if_fail (self->active, NULL); + + g_hash_table_iter_init (&iter, self->accounts); + + while (g_hash_table_iter_next (&iter, &k, NULL)) + ret = g_list_prepend (ret, g_strdup (k)); + return ret; } @@ -1445,3 +1499,10 @@ account_storage_iface_init (McpAccountStorageIface *iface) iface->get_restrictions = test_dbus_account_plugin_get_restrictions; iface->create = test_dbus_account_plugin_create; } + +static void +async_initable_iface_init (GAsyncInitableIface *iface) +{ + iface->init_async = test_dbus_account_plugin_init_async; + iface->init_finish = test_dbus_account_plugin_init_finish; +} diff --git a/tests/twisted/mctest.py b/tests/twisted/mctest.py index 549a742..f8e4f4f 100644 --- a/tests/twisted/mctest.py +++ b/tests/twisted/mctest.py @@ -61,7 +61,8 @@ def install_colourer(): return sys.stdout class MC(dbus.proxies.ProxyObject): - def __init__(self, queue, bus, wait_for_names=True, initially_online=True): + def __init__(self, queue, bus, wait_for_names=True, initially_online=True, + wait_for_fake_accounts_service=False): """ Arguments: @@ -84,7 +85,13 @@ class MC(dbus.proxies.ProxyObject): self.bus = bus if wait_for_names: - self.wait_for_names() + if wait_for_fake_accounts_service: + self.wait_for_names(servicetest.EventPattern('dbus-signal', + path=cs.TEST_DBUS_ACCOUNT_PLUGIN_PATH, + interface=cs.TEST_DBUS_ACCOUNT_PLUGIN_IFACE, + signal='Active')) + else: + self.wait_for_names() def wait_for_names(self, *also_expect): """ @@ -121,9 +128,16 @@ def exec_test_deferred (fun, params, protocol=None, timeout=None, queue.attach_to_bus(bus) error = None + if use_fake_accounts_service: + fake_accounts_service = FakeAccountsService(queue, bus) + else: + fake_accounts_service = None + if preload_mc: try: - mc = MC(queue, bus, initially_online=initially_online) + mc = MC(queue, bus, initially_online=initially_online, + wait_for_fake_accounts_service=use_fake_accounts_service) + except Exception, e: import traceback traceback.print_exc() @@ -131,17 +145,6 @@ def exec_test_deferred (fun, params, protocol=None, timeout=None, else: mc = None - if use_fake_accounts_service: - fake_accounts_service = FakeAccountsService(queue, bus) - - if preload_mc: - queue.expect('dbus-signal', - path=cs.TEST_DBUS_ACCOUNT_PLUGIN_PATH, - interface=cs.TEST_DBUS_ACCOUNT_PLUGIN_IFACE, - signal='Active') - else: - fake_accounts_service = None - if pass_kwargs: kwargs=dict(fake_accounts_service=fake_accounts_service) else: -- 1.9.rc1