From 4f3057921866aee317c23376ae0ce3c796484f59 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 6 Feb 2014 14:56:38 +0000 Subject: [PATCH 2/2] Let account storage plugins load asynchronously This means they don't have to be prepared to block during list(). (tests/twisted/mcp-account-diversion.c still loads its data at the last possible moment, in list(), to confirm that the no-op default implementation of list_async and list_finish works as intended.) --- mission-control-plugins/account-storage.c | 97 +++++++++++++++++++++++++++---- mission-control-plugins/account-storage.h | 18 ++++++ src/mcd-account-manager-default.c | 37 ++++++++++-- src/mcd-account-manager-priv.h | 3 - src/mcd-account-manager.c | 24 ++++---- src/mcd-master.c | 2 - src/mcd-storage.c | 86 ++++++++++++++++++++++----- src/mcd-storage.h | 7 +++ tests/twisted/dbus-account-plugin.c | 93 ++++++++++++++++++++++------- tests/twisted/mctest.py | 31 +++++----- 10 files changed, 317 insertions(+), 81 deletions(-) diff --git a/mission-control-plugins/account-storage.c b/mission-control-plugins/account-storage.c index 081597c..c05ad61 100644 --- a/mission-control-plugins/account-storage.c +++ b/mission-control-plugins/account-storage.c @@ -60,6 +60,8 @@ * iface->delete_async = foo_plugin_delete_async; * iface->delete_finish = foo_plugin_delete_finish; * iface->commit = foo_plugin_commit; + * iface->load_async = foo_plugin_load_async; + * iface->load_finish = foo_plugin_load_finish; * iface->list = foo_plugin_list; * iface->get_identifier = foo_plugin_get_identifier; * iface->get_additional_info = foo_plugin_get_additional_info; @@ -225,6 +227,27 @@ default_get_flags (McpAccountStorage *storage, } static void +default_load_async (McpAccountStorage *storage, + McpAccountManager *am, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) +{ + GTask *task = g_task_new (storage, cancellable, callback, user_data); + + g_task_return_boolean (task, TRUE); + g_object_unref (task); +} + +static gboolean +default_load_finish (McpAccountStorage *storage, + GAsyncResult *res, + GError **error) +{ + return g_task_propagate_boolean (G_TASK (res), error); +} + +static void class_init (gpointer klass, gpointer data) { @@ -232,6 +255,8 @@ class_init (gpointer klass, McpAccountStorageIface *iface = klass; iface->get_flags = default_get_flags; + iface->load_async = default_load_async; + iface->load_finish = default_load_finish; iface->create = default_create; iface->delete_async = default_delete_async; iface->delete_finish = default_delete_finish; @@ -398,6 +423,8 @@ mcp_account_storage_get_type (void) * @list_untyped_parameters: implementation * of mcp_account_storage_list_untyped_parameters() * @get_flags: implementation of mcp_account_storage_get_flags() + * @load_async: implementation of mcp_account_storage_load_async() + * @load_finish: implementation of mcp_account_storage_load_finish() * * The interface vtable for an account storage plugin. */ @@ -424,11 +451,6 @@ mcp_account_storage_get_type (void) * lowest to highest, so that higher priority plugins may overrule settings * from lower priority plugins. * - * Loading all the accounts is only done at startup, before the dbus name - * is claimed, and is therefore the only time plugins are allowed to indulge - * in blocking calls (indeed, they are expected to carry out this operation, - * and ONLY this operation, synchronously). - * * When values are being set, the plugins are invoked from highest priority * to lowest, with the first plugin that claims a setting being assigned * ownership, and all lower priority plugins being asked to delete the @@ -901,11 +923,7 @@ mcp_account_storage_commit (McpAccountStorage *storage, * @storage: an #McpAccountStorage instance * @am: an #McpAccountManager instance * - * Load details of every account stored by this plugin into an in-memory cache - * so that it can respond to requests promptly. - * - * This method is called only at initialisation time, before the dbus name - * has been claimed, and is the only one permitted to block. + * Return a list of account names. * * There is no default implementation. All implementations of this interface * must override this method. @@ -1251,3 +1269,62 @@ mcp_account_storage_has_any_flag (McpAccountStorage *storage, return ((mcp_account_storage_get_flags (storage, account) & require_one) != 0); } + +/** + * mcp_account_storage_load_async: + * @storage: an #McpAccountStorage instance + * @am: an #McpAccountManager instance + * @cancellable: (allow-none): optionally used to (try to) cancel the operation + * @callback: called on success or failure + * @user_data: data for @callback + * + * Load all accounts. This method will only be called once; if it fails, + * the plugin will not be used for account storage at all. + * + * The default implementation doesn't do anything, and reports success. + * Override it if there are things your plugin needs to do before it can + * respond to mcp_account_storage_list(). + * + * Implementations that override load_async must also override + * load_finish. + */ +void +mcp_account_storage_load_async (McpAccountStorage *storage, + McpAccountManager *am, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) +{ + McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage); + + SDEBUG (storage, ""); + + g_return_if_fail (iface != NULL); + g_return_if_fail (iface->load_async != NULL); + + iface->load_async (storage, am, cancellable, callback, user_data); +} + +/** + * mcp_account_storage_load_finish: + * @storage: an #McpAccountStorage instance + * @res: the result of mcp_account_storage_load_async() + * @error: used to raise an error if %FALSE is returned + * + * Process the result of mcp_account_storage_load_async(). + * + * Returns: %TRUE on success, %FALSE if the account could not be loaded + */ +gboolean +mcp_account_storage_load_finish (McpAccountStorage *storage, + GAsyncResult *result, + GError **error) +{ + McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage); + + SDEBUG (storage, ""); + g_return_val_if_fail (iface != NULL, FALSE); + g_return_val_if_fail (iface->load_finish != NULL, FALSE); + + return iface->load_finish (storage, result, error); +} diff --git a/mission-control-plugins/account-storage.h b/mission-control-plugins/account-storage.h index b801521..1388214 100644 --- a/mission-control-plugins/account-storage.h +++ b/mission-control-plugins/account-storage.h @@ -168,6 +168,15 @@ struct _McpAccountStorageIface McpAccountStorageFlags (*get_flags) (McpAccountStorage *storage, const gchar *account); + + void (*load_async) (McpAccountStorage *storage, + McpAccountManager *am, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data); + gboolean (*load_finish) (McpAccountStorage *storage, + GAsyncResult *res, + GError **error); }; /* virtual methods */ @@ -261,6 +270,15 @@ gboolean mcp_account_storage_has_any_flag ( const gchar *account, McpAccountStorageFlags require_one); +void mcp_account_storage_load_async (McpAccountStorage *storage, + McpAccountManager *am, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data); +gboolean mcp_account_storage_load_finish (McpAccountStorage *storage, + GAsyncResult *result, + GError **error); + void mcp_account_storage_emit_created (McpAccountStorage *storage, const gchar *account); void mcp_account_storage_emit_altered_one (McpAccountStorage *storage, diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c index e6fc14f..c77f250 100644 --- a/src/mcd-account-manager-default.c +++ b/src/mcd-account-manager-default.c @@ -862,17 +862,21 @@ am_default_load_directory (McdAccountManagerDefault *self, } } -static GList * -_list (McpAccountStorage *self, - McpAccountManager *am) +static void +load_async (McpAccountStorage *self, + McpAccountManager *am, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) { - GList *rval = NULL; McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self); GHashTableIter hash_iter; gchar *migrate_from = NULL; gpointer k, v; gboolean save = FALSE; + GTask *task; + /* For the moment, we just load synchronously. */ if (!amd->loaded) { const gchar * const *iter; @@ -1013,6 +1017,28 @@ _list (McpAccountStorage *self, tp_clear_pointer (&migrate_from, g_free); + task = g_task_new (self, cancellable, callback, user_data); + g_task_return_boolean (task, TRUE); + g_object_unref (task); +} + +static gboolean +load_finish (McpAccountStorage *storage, + GAsyncResult *res, + GError **error) +{ + return g_task_propagate_boolean (G_TASK (res), error); +} + +static GList * +_list (McpAccountStorage *self, + McpAccountManager *am) +{ + GList *rval = NULL; + McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self); + GHashTableIter hash_iter; + gpointer k, v; + g_hash_table_iter_init (&hash_iter, amd->accounts); while (g_hash_table_iter_next (&hash_iter, &k, &v)) @@ -1053,7 +1079,8 @@ account_storage_iface_init (McpAccountStorageIface *iface, iface->delete_finish = delete_finish; iface->commit = _commit; iface->list = _list; - + iface->load_async = load_async; + iface->load_finish = load_finish; } McdAccountManagerDefault * 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..e83e76d 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,64 @@ 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 (!mcp_account_storage_load_finish (loaded, 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; + McpAccountManager *api = MCP_ACCOUNT_MANAGER (self); + + sort_and_cache_plugins (); + + g_task_set_task_data (task, GSIZE_TO_POINTER (remaining), NULL); + + for (store = stores; store != NULL; store = store->next) + { + remaining++; + g_task_set_task_data (task, GSIZE_TO_POINTER (remaining), NULL); + mcp_account_storage_load_async (store->data, api, + NULL, mcd_storage_loaded_cb, g_object_ref (task)); + } + + /* just in case there were no stores (can't actually happen, + * since we compile one into MC itself) */ + 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 ec125bc..4354988 100644 --- a/tests/twisted/dbus-account-plugin.c +++ b/tests/twisted/dbus-account-plugin.c @@ -627,19 +627,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_load_async (McpAccountStorage *storage, + McpAccountManager *am, + 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; + GTask *task; DEBUG ("called"); @@ -691,8 +691,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, @@ -701,15 +702,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; @@ -728,12 +749,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_load_finish (McpAccountStorage *storage, + 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; } @@ -1440,4 +1491,6 @@ account_storage_iface_init (McpAccountStorageIface *iface) iface->get_additional_info = test_dbus_account_plugin_get_additional_info; iface->get_restrictions = test_dbus_account_plugin_get_restrictions; iface->create = test_dbus_account_plugin_create; + iface->load_async = test_dbus_account_plugin_load_async; + iface->load_finish = test_dbus_account_plugin_load_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