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 (file)
--- 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<string, PersonaStore> _persona_stores;
   private Map<string, PersonaStore> _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 (file)
--- 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<string, Persona> _personas;
   private Map<string, Persona> _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 (file)
--- 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<string, PersonaStore> _persona_stores;
   private Map<string, PersonaStore> _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 (file)
--- 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 (file)
--- 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<string, Persona> _personas;
   private Map<string, Persona> _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 (file)
--- 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<string, PersonaStore> _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 (file)
--- 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 (file)
--- 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<string, PersonaStore> _persona_stores;
   private Map<string, PersonaStore> _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 (file)
--- 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<string, Persona> _personas;
   private Map<string, Persona> _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 (file)
--- 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<string, PersonaStore> _persona_stores;
   private Map<string, PersonaStore> _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 (file)
--- 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 (file)
--- 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 (file)
--- 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;