From: Philip Withnall Date: Thu, 8 Dec 2011 16:21:45 +0000 (+0000) Subject: backends: Tidy up prepare() and unprepare() methods’ mutual exclusion X-Git-Tag: FOLKS_0_6_6~4 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=58dba5a8129e498d7aaf8811597208f347f7ff31;p=platform%2Fupstream%2Ffolks.git backends: Tidy up prepare() and unprepare() methods’ mutual exclusion As discovered in bgo#665728, all our prepare() (and unprepare()) methods are currently vulnerable to being run multiple times concurrently from a single thread. Add a _pending_prepare flag to all of them to prevent this. It doesn't need to be locked, since it should only ever be accessed from a single thread (since only a single thread can get through the lock{} recursive mutex at once). Helps: bgo#665728 --- diff --git a/backends/eds/eds-backend.vala b/backends/eds/eds-backend.vala index 7b162e2..9886222 100644 --- a/backends/eds/eds-backend.vala +++ b/backends/eds/eds-backend.vala @@ -37,6 +37,7 @@ public class Folks.Backends.Eds.Backend : Folks.Backend private static const string _use_address_books = "FOLKS_BACKEND_EDS_USE_ADDRESS_BOOKS"; private bool _is_prepared = false; + private bool _prepare_pending = false; /* used for unprepare() too */ private bool _is_quiescent = false; private HashMap _persona_stores; private Map _persona_stores_ro; @@ -95,8 +96,15 @@ public class Folks.Backends.Eds.Backend : Folks.Backend { lock (this._is_prepared) { - if (!this._is_prepared) + if (this._is_prepared || this._prepare_pending) { + return; + } + + try + { + this._prepare_pending = true; + this._create_avatars_cache_dir (); E.BookClient.get_sources (out this._ab_sources); @@ -110,6 +118,10 @@ public class Folks.Backends.Eds.Backend : Folks.Backend this._is_quiescent = true; this.notify_property ("is-quiescent"); } + finally + { + this._prepare_pending = false; + } } } @@ -120,8 +132,15 @@ public class Folks.Backends.Eds.Backend : Folks.Backend { lock (this._is_prepared) { - if (this._is_prepared) + if (!this._is_prepared || this._prepare_pending) { + return; + } + + try + { + this._prepare_pending = true; + foreach (var persona_store in this._persona_stores.values) { this._remove_address_book (persona_store); @@ -136,6 +155,10 @@ public class Folks.Backends.Eds.Backend : Folks.Backend this._is_prepared = false; this.notify_property ("is-prepared"); } + finally + { + this._prepare_pending = false; + } } } diff --git a/backends/eds/lib/edsf-persona-store.vala b/backends/eds/lib/edsf-persona-store.vala index ea75f17..6f3dc84 100644 --- a/backends/eds/lib/edsf-persona-store.vala +++ b/backends/eds/lib/edsf-persona-store.vala @@ -37,6 +37,7 @@ public class Edsf.PersonaStore : Folks.PersonaStore private HashMap _personas; private Map _personas_ro; private bool _is_prepared = false; + private bool _prepare_pending = false; private bool _is_quiescent = false; private E.BookClient _addressbook; private E.BookClientView _ebookview; @@ -533,11 +534,13 @@ public class Edsf.PersonaStore : Folks.PersonaStore /* FIXME: https://bugzilla.gnome.org/show_bug.cgi?id=652637 */ lock (this._is_prepared) { - if (this._is_prepared) + if (this._is_prepared == true || this._prepare_pending == true) { return; } + this._prepare_pending = true; + try { /* Listen for removal signals for the address book. There's no @@ -625,11 +628,16 @@ public class Edsf.PersonaStore : Folks.PersonaStore * and the second is an error message. */ _("Couldn't open address book ‘%s’: %s"), this.id, e1.message); } + finally + { + this._prepare_pending = false; + } if (this._addressbook.is_opened () == false) { /* Remove the persona store on error */ this.removed (); + this._prepare_pending = false; throw new PersonaStoreError.INVALID_ARGUMENT ( /* Translators: the parameter is an address book URI. */ @@ -697,6 +705,10 @@ public class Edsf.PersonaStore : Folks.PersonaStore /* Translators: the parameteter is an error message. */ _("Couldn't get address book capabilities: %s"), e2.message); } + finally + { + this._prepare_pending = false; + } /* Get the set of capabilities supported by the address book. * Specifically, we're looking for do-initial-query, which signifies @@ -724,6 +736,10 @@ public class Edsf.PersonaStore : Folks.PersonaStore /* Translators: the parameteter is an error message. */ _("Couldn't get address book capabilities: %s"), e4.message); } + finally + { + this._prepare_pending = false; + } bool got_view = false; try @@ -815,8 +831,13 @@ public class Edsf.PersonaStore : Folks.PersonaStore _("Couldn't get view for address book ‘%s’: %s"), this.id, e3.message); } + finally + { + this._prepare_pending = false; + } this._is_prepared = true; + this._prepare_pending = false; this.notify_property ("is-prepared"); /* If the address book isn't going to do an initial query (i.e. diff --git a/backends/key-file/kf-backend.vala b/backends/key-file/kf-backend.vala index 9a2bb9b..73fc11d 100644 --- a/backends/key-file/kf-backend.vala +++ b/backends/key-file/kf-backend.vala @@ -35,6 +35,7 @@ extern const string BACKEND_NAME; public class Folks.Backends.Kf.Backend : Folks.Backend { private bool _is_prepared = false; + private bool _prepare_pending = false; /* used for unprepare() too */ private bool _is_quiescent = false; private HashMap _persona_stores; private Map _persona_stores_ro; @@ -92,8 +93,15 @@ public class Folks.Backends.Kf.Backend : Folks.Backend { lock (this._is_prepared) { - if (!this._is_prepared) + if (this._is_prepared || this._prepare_pending) { + return; + } + + try + { + this._prepare_pending = true; + File file; unowned string path = Environment.get_variable ( "FOLKS_BACKEND_KEY_FILE_PATH"); @@ -130,6 +138,10 @@ public class Folks.Backends.Kf.Backend : Folks.Backend this._is_quiescent = true; this.notify_property ("is-quiescent"); } + finally + { + this._prepare_pending = false; + } } } @@ -138,19 +150,36 @@ public class Folks.Backends.Kf.Backend : Folks.Backend */ public override async void unprepare () throws GLib.Error { - foreach (var persona_store in this._persona_stores.values) + lock (this._is_prepared) { - this.persona_store_removed (persona_store); - } + if (!this._is_prepared || this._prepare_pending == true) + { + return; + } - this._persona_stores.clear (); - this.notify_property ("persona-stores"); + try + { + this._prepare_pending = true; - this._is_quiescent = false; - this.notify_property ("is-quiescent"); + foreach (var persona_store in this._persona_stores.values) + { + this.persona_store_removed (persona_store); + } + + this._persona_stores.clear (); + this.notify_property ("persona-stores"); + + this._is_quiescent = false; + this.notify_property ("is-quiescent"); - this._is_prepared = false; - this.notify_property ("is-prepared"); + this._is_prepared = false; + this.notify_property ("is-prepared"); + } + finally + { + this._prepare_pending = false; + } + } } private void _store_removed_cb (Folks.PersonaStore store) diff --git a/backends/key-file/kf-persona-store.vala b/backends/key-file/kf-persona-store.vala index c04f3af..b33f5e2 100644 --- a/backends/key-file/kf-persona-store.vala +++ b/backends/key-file/kf-persona-store.vala @@ -38,6 +38,7 @@ public class Folks.Backends.Kf.PersonaStore : Folks.PersonaStore private GLib.KeyFile _key_file; private unowned Cancellable _save_key_file_cancellable = null; private bool _is_prepared = false; + private bool _prepare_pending = false; private bool _is_quiescent = false; private const string[] _always_writeable_properties = @@ -168,8 +169,15 @@ public class Folks.Backends.Kf.PersonaStore : Folks.PersonaStore { lock (this._is_prepared) { - if (!this._is_prepared) + if (this._is_prepared || this._prepare_pending) { + return; + } + + try + { + this._prepare_pending = true; + var filename = this._file.get_path (); this._key_file = new GLib.KeyFile (); @@ -283,6 +291,10 @@ public class Folks.Backends.Kf.PersonaStore : Folks.PersonaStore this._is_quiescent = true; this.notify_property ("is-quiescent"); } + finally + { + this._prepare_pending = false; + } } } diff --git a/backends/libsocialweb/lib/swf-persona-store.vala b/backends/libsocialweb/lib/swf-persona-store.vala index 6557490..c078d76 100644 --- a/backends/libsocialweb/lib/swf-persona-store.vala +++ b/backends/libsocialweb/lib/swf-persona-store.vala @@ -37,6 +37,7 @@ public class Swf.PersonaStore : Folks.PersonaStore private HashMap _personas; private Map _personas_ro; private bool _is_prepared = false; + private bool _prepare_pending = false; private bool _is_quiescent = false; private ClientService _service; private ClientContactView _contact_view; @@ -210,8 +211,10 @@ public class Swf.PersonaStore : Folks.PersonaStore { lock (this._is_prepared) { - if (!this._is_prepared) + if (!this._is_prepared && !this._prepare_pending) { + this._prepare_pending = true; + /* Take a reference to the PersonaStore while waiting for the * async call to return. See: bgo#665039. */ this.ref (); @@ -222,6 +225,7 @@ public class Swf.PersonaStore : Folks.PersonaStore if (caps == null) { this.unref (); + this._prepare_pending = false; return; } @@ -230,6 +234,7 @@ public class Swf.PersonaStore : Folks.PersonaStore if (!has_contacts) { this.unref (); + this._prepare_pending = false; return; } @@ -247,6 +252,7 @@ public class Swf.PersonaStore : Folks.PersonaStore if (contact_view == null) { this.unref (); + this._prepare_pending = false; return; } @@ -259,6 +265,7 @@ public class Swf.PersonaStore : Folks.PersonaStore this._contact_view = contact_view; this._is_prepared = true; + this._prepare_pending = false; this.notify_property ("is-prepared"); /* FIXME: for lsw Stores with 0 contacts or badly diff --git a/backends/libsocialweb/sw-backend.vala b/backends/libsocialweb/sw-backend.vala index 9f78a67..f4b60f4 100644 --- a/backends/libsocialweb/sw-backend.vala +++ b/backends/libsocialweb/sw-backend.vala @@ -34,6 +34,7 @@ extern const string BACKEND_NAME; public class Folks.Backends.Sw.Backend : Folks.Backend { private bool _is_prepared = false; + private bool _prepare_pending = false; private bool _is_quiescent = false; private Client _client; private HashMap _persona_stores; @@ -91,8 +92,10 @@ public class Folks.Backends.Sw.Backend : Folks.Backend { lock (this._is_prepared) { - if (!this._is_prepared) + if (!this._is_prepared && !this._prepare_pending) { + this._prepare_pending = true; + /* Hold a ref. on the Backend while we wait for the callback from * this._client.get_services() to prevent the Backend being * destroyed in the mean time. See: bgo#665039. */ @@ -105,6 +108,7 @@ public class Folks.Backends.Sw.Backend : Folks.Backend this.add_service (service_name); this._is_prepared = true; + this._prepare_pending = false; this.notify_property ("is-prepared"); this._is_quiescent = true; @@ -121,22 +125,33 @@ public class Folks.Backends.Sw.Backend : Folks.Backend */ public override async void unprepare () throws GLib.Error { - foreach (var store in this._persona_stores.values) + lock (this._is_prepared) { - store.removed.disconnect (this.store_removed_cb); - this.persona_store_removed (store); - } + if (!this._is_prepared || this._prepare_pending) + { + return; + } - this._client = null; + this._prepare_pending = true; - this._persona_stores.clear (); - this.notify_property ("persona-stores"); + foreach (var store in this._persona_stores.values) + { + store.removed.disconnect (this.store_removed_cb); + this.persona_store_removed (store); + } + + this._client = null; - this._is_quiescent = false; - this.notify_property ("is-quiescent"); + this._persona_stores.clear (); + this.notify_property ("persona-stores"); - this._is_prepared = false; - this.notify_property ("is-prepared"); + this._is_quiescent = false; + this.notify_property ("is-quiescent"); + + this._is_prepared = false; + this._prepare_pending = false; + this.notify_property ("is-prepared"); + } } private void add_service (string service_name) diff --git a/backends/telepathy/lib/tpf-persona-store.vala b/backends/telepathy/lib/tpf-persona-store.vala index 567ec17..76caec8 100644 --- a/backends/telepathy/lib/tpf-persona-store.vala +++ b/backends/telepathy/lib/tpf-persona-store.vala @@ -562,7 +562,12 @@ public class Tpf.PersonaStore : Folks.PersonaStore { lock (this._is_prepared) { - if (!this._is_prepared && !this._prepare_pending) + if (this._is_prepared || this._prepare_pending) + { + return; + } + + try { this._prepare_pending = true; @@ -629,9 +634,12 @@ public class Tpf.PersonaStore : Folks.PersonaStore } this._is_prepared = true; - this._prepare_pending = false; this.notify_property ("is-prepared"); } + finally + { + this._prepare_pending = false; + } } } diff --git a/backends/telepathy/tp-backend.vala b/backends/telepathy/tp-backend.vala index 0c3696c..37c7481 100644 --- a/backends/telepathy/tp-backend.vala +++ b/backends/telepathy/tp-backend.vala @@ -34,6 +34,7 @@ public class Folks.Backends.Tp.Backend : Folks.Backend { private AccountManager _account_manager; private bool _is_prepared = false; + private bool _prepare_pending = false; /* used by unprepare() too */ private bool _is_quiescent = false; private HashMap _persona_stores; private Map _persona_stores_ro; @@ -91,7 +92,12 @@ public class Folks.Backends.Tp.Backend : Folks.Backend { lock (this._is_prepared) { - if (!this._is_prepared) + if (this._is_prepared || this._prepare_pending) + { + return; + } + + try { this._account_manager = AccountManager.dup (); yield this._account_manager.prepare_async (null); @@ -113,6 +119,10 @@ public class Folks.Backends.Tp.Backend : Folks.Backend this._is_quiescent = true; this.notify_property ("is-quiescent"); } + finally + { + this._prepare_pending = false; + } } } @@ -121,28 +131,40 @@ public class Folks.Backends.Tp.Backend : Folks.Backend */ public override async void unprepare () throws GLib.Error { - if (!this._is_prepared) - return; + lock (this._is_prepared) + { + if (!this._is_prepared || this._prepare_pending) + { + return; + } - this._account_manager.account_enabled.disconnect ( - this._account_enabled_cb); - this._account_manager.account_validity_changed.disconnect ( - this._account_validity_changed_cb); - this._account_manager = null; + try + { + this._account_manager.account_enabled.disconnect ( + this._account_enabled_cb); + this._account_manager.account_validity_changed.disconnect ( + this._account_validity_changed_cb); + this._account_manager = null; - foreach (var persona_store in this._persona_stores.values) - { - this.persona_store_removed (persona_store); - } + foreach (var persona_store in this._persona_stores.values) + { + this.persona_store_removed (persona_store); + } - this._persona_stores.clear (); - this.notify_property ("persona-stores"); + this._persona_stores.clear (); + this.notify_property ("persona-stores"); - this._is_quiescent = false; - this.notify_property ("is-quiescent"); + this._is_quiescent = false; + this.notify_property ("is-quiescent"); - this._is_prepared = false; - this.notify_property ("is-prepared"); + this._is_prepared = false; + this.notify_property ("is-prepared"); + } + finally + { + this._prepare_pending = false; + } + } } private void _account_validity_changed_cb (Account account, bool valid) diff --git a/backends/tracker/lib/trf-persona-store.vala b/backends/tracker/lib/trf-persona-store.vala index bf98f37..66f5a53 100644 --- a/backends/tracker/lib/trf-persona-store.vala +++ b/backends/tracker/lib/trf-persona-store.vala @@ -166,6 +166,7 @@ public class Trf.PersonaStore : Folks.PersonaStore private HashMap _personas; private Map _personas_ro; private bool _is_prepared = false; + private bool _prepare_pending = false; private bool _is_quiescent = false; private static const int _default_timeout = 100; private Resources _resources_object; @@ -1056,8 +1057,15 @@ public class Trf.PersonaStore : Folks.PersonaStore { lock (this._is_prepared) { - if (!this._is_prepared) + if (this._is_prepared || this._prepare_pending) { + return; + } + + try + { + this._prepare_pending = true; + try { this._connection = @@ -1113,6 +1121,10 @@ public class Trf.PersonaStore : Folks.PersonaStore throw new PersonaStoreError.INVALID_ARGUMENT (e3.message); } } + finally + { + this._prepare_pending = false; + } } } diff --git a/backends/tracker/tr-backend.vala b/backends/tracker/tr-backend.vala index 8887cdb..40ab4df 100644 --- a/backends/tracker/tr-backend.vala +++ b/backends/tracker/tr-backend.vala @@ -33,6 +33,7 @@ extern const string BACKEND_NAME; public class Folks.Backends.Tr.Backend : Folks.Backend { private bool _is_prepared = false; + private bool _prepare_pending = false; /* used by unprepare() too */ private bool _is_quiescent = false; private HashMap _persona_stores; private Map _persona_stores_ro; @@ -89,8 +90,15 @@ public class Folks.Backends.Tr.Backend : Folks.Backend { lock (this._is_prepared) { - if (!this._is_prepared) + if (this._is_prepared || this._prepare_pending) { + return; + } + + try + { + this._prepare_pending = true; + this._add_default_persona_store (); this._is_prepared = true; @@ -99,6 +107,10 @@ public class Folks.Backends.Tr.Backend : Folks.Backend this._is_quiescent = true; this.notify_property ("is-quiescent"); } + finally + { + this._prepare_pending = false; + } } } @@ -107,19 +119,36 @@ public class Folks.Backends.Tr.Backend : Folks.Backend */ public override async void unprepare () throws GLib.Error { - foreach (var persona_store in this._persona_stores.values) + lock (this._is_prepared) { - this.persona_store_removed (persona_store); - } + if (!this._is_prepared || this._prepare_pending) + { + return; + } - this._persona_stores.clear (); - this.notify_property ("persona-stores"); + try + { + this._prepare_pending = true; + + foreach (var persona_store in this._persona_stores.values) + { + this.persona_store_removed (persona_store); + } - this._is_quiescent = false; - this.notify_property ("is-quiescent"); + this._persona_stores.clear (); + this.notify_property ("persona-stores"); + + this._is_quiescent = false; + this.notify_property ("is-quiescent"); - this._is_prepared = false; - this.notify_property ("is-prepared"); + this._is_prepared = false; + this.notify_property ("is-prepared"); + } + finally + { + this._prepare_pending = false; + } + } } /** diff --git a/folks/backend.vala b/folks/backend.vala index 1ed815e..09fc846 100644 --- a/folks/backend.vala +++ b/folks/backend.vala @@ -116,6 +116,12 @@ public abstract class Folks.Backend : Object * * This function is guaranteed to be idempotent (since version 0.3.0). * + * Concurrent calls to this function from different threads will block until + * preparation has completed. However, concurrent calls to this function from + * a single thread might not, i.e. the first call will block but subsequent + * calls might return before the first one. (Though they will be safe in every + * other respect.) + * * @since 0.1.11 */ public abstract async void prepare () throws GLib.Error; @@ -132,6 +138,12 @@ public abstract class Folks.Backend : Object * * If this function throws an error, the Backend will not be functional. * + * Concurrent calls to this function from different threads will block until + * preparation has completed. However, concurrent calls to this function from + * a single thread might not, i.e. the first call will block but subsequent + * calls might return before the first one. (Though they will be safe in every + * other respect.) + * * @since 0.3.2 */ public abstract async void unprepare () throws GLib.Error; diff --git a/folks/individual-aggregator.vala b/folks/individual-aggregator.vala index b74a4da..b349ce4 100644 --- a/folks/individual-aggregator.vala +++ b/folks/individual-aggregator.vala @@ -471,6 +471,12 @@ public class Folks.IndividualAggregator : Object * * This function is guaranteed to be idempotent (since version 0.3.0). * + * Concurrent calls to this function from different threads will block until + * preparation has completed. However, concurrent calls to this function from + * a single thread might not, i.e. the first call will block but subsequent + * calls might return before the first one. (Though they will be safe in every + * other respect.) + * * @since 0.1.11 */ public async void prepare () throws GLib.Error @@ -481,14 +487,24 @@ public class Folks.IndividualAggregator : Object lock (this._is_prepared) { - if (!this._is_prepared && !this._prepare_pending) + if (this._is_prepared || this._prepare_pending) + { + return; + } + + try { this._prepare_pending = true; + yield this._backend_store.load_backends (); + this._is_prepared = true; - this._prepare_pending = false; this.notify_property ("is-prepared"); } + finally + { + this._prepare_pending = false; + } } } diff --git a/folks/persona-store.vala b/folks/persona-store.vala index b64feeb..cf43325 100644 --- a/folks/persona-store.vala +++ b/folks/persona-store.vala @@ -580,6 +580,12 @@ public abstract class Folks.PersonaStore : Object * * This function is guaranteed to be idempotent (since version 0.3.0). * + * Concurrent calls to this function from different threads will block until + * preparation has completed. However, concurrent calls to this function from + * a single thread might not, i.e. the first call will block but subsequent + * calls might return before the first one. (Though they will be safe in every + * other respect.) + * * @since 0.1.11 */ public abstract async void prepare () throws GLib.Error;