[folks] core: Rate-limit AvatarCache.store_avatar() to prevent FD exhaustion

[Philip Withnall <pwithnall src gnome org>]

commit 2001d7adaff969a7c900dce613a00d4668cae01c
Author: Philip Withnall <philip tecnocode co uk>
Date:   Sun Apr 6 11:10:16 2014 +0100

    core: Rate-limit AvatarCache.store_avatar() to prevent FD exhaustion
    
    If connecting to a new EDS address book, or doing something else which
    may cause a torrent of AvatarCache.store_avatar() calls, it is possible
    to hit the operating system’s file descriptor limit, which causes
    further store operations to fail.
    
    Implement a simple FIFO rate limit on concurrent
    AvatarCache.store_avatar() calls to try and ensure this won’t happen. It
    is still possible if the process calls store_avatar() several times
    while already near the FD limit, but then failure is pretty inevitable
    anyway, and is already handled gracefully.
    
    The changes are implemented in a self-contained wrapper function around
    store_avatar(), and only result in memory allocations in the queueing
    case.
    
    A test case is included. This does not affect the AvatarCache API or
    ABI.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=705742

 NEWS                          |    1 +
 folks/avatar-cache.vala       |   58 +++++++++++++++++++++++++++++++++++++++++
 tests/folks/avatar-cache.vala |   51 ++++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+), 0 deletions(-)
---
diff --git a/NEWS b/NEWS
index d1cf948..951f666 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,7 @@ Bugs fixed:
  • Bug 727405 — setting FOLKS_DISABLE_LINKING to on does not work
  • Bug 722579 — Contacts are displayed for Google accounts where contact
    management is disabled in GOA
+ • Bug 705742 — Implement rate limiting in AvatarCache.store_avatar()
 
 API changes:
  • Add Individual.display_name
diff --git a/folks/avatar-cache.vala b/folks/avatar-cache.vala
index 78ff67b..eee4e81 100644
--- a/folks/avatar-cache.vala
+++ b/folks/avatar-cache.vala
@@ -35,12 +35,22 @@ using GLib;
  * same namespace, so callers must ensure that the IDs they use for avatars are
  * globally unique (e.g. by using the corresponding { link Folks.Persona.uid}).
  *
+ * Ongoing store operations ({ link Folks.AvatarCache.store_avatar}) are rate
+ * limited to try and prevent file descriptor exhaustion. Load operations
+ * ({ link Folks.AvatarCache.load_avatar}) must be rate limited by the client,
+ * as the file I/O occurs when calling { link LoadableIcon.load} rather than
+ * when retrieving the { link LoadableIcon} from the cache.
+ *
  * @since 0.6.0
  */
 public class Folks.AvatarCache : Object
 {
   private static weak AvatarCache? _instance = null; /* needs to be locked */
   private File _cache_directory;
+  private uint _n_ongoing_stores = 0;
+  private Queue<DelegateWrapper> _pending_stores =
+      new Queue<DelegateWrapper> ();
+  private const uint _max_n_ongoing_stores = 10;
 
   /**
    * Private constructor for an instance of the avatar cache. The singleton
@@ -104,6 +114,9 @@ public class Folks.AvatarCache : Object
   /**
    * Fetch an avatar from the cache by its globally unique ID.
    *
+   * It is up to the caller to ensure that file I/O is rate-limited when loading
+   * many avatars in parallel, by limiting calls to { link LoadableIcon.load}.
+   *
    * @param id the globally unique ID for the avatar
    * @return Avatar from the cache, or ``null`` if it doesn't exist in the cache
    * @throws GLib.Error if checking for existence of the cache file failed
@@ -134,6 +147,9 @@ public class Folks.AvatarCache : Object
    * ID (e.g. an asynchronous call may be made, and a subsequent asynchronous
    * call may begin before the first has finished).
    *
+   * Concurrent file I/O may be rate limited within each { link AvatarCache}
+   * instance to avoid file descriptor exhaustion.
+   *
    * @param id the globally unique ID for the avatar
    * @param avatar the avatar data to cache
    * @return a URI for the file storing the cached avatar
@@ -144,6 +160,41 @@ public class Folks.AvatarCache : Object
   public async string store_avatar (string id, LoadableIcon avatar)
       throws GLib.Error
     {
+      string avatar_uri = "";
+
+      if (this._n_ongoing_stores > AvatarCache._max_n_ongoing_stores)
+        {
+          /* Add to the pending queue. */
+          var wrapper = new DelegateWrapper ();
+          wrapper.cb = store_avatar.callback;
+          this._pending_stores.push_tail ((owned) wrapper);
+          yield;
+        }
+
+      /* Do the actual store operation. */
+      try
+        {
+          this._n_ongoing_stores++;
+          avatar_uri = yield this._store_avatar_unlimited (id, avatar);
+        }
+      finally
+        {
+          this._n_ongoing_stores--;
+
+          /* If there is a store operation pending, resume it, FIFO-style. */
+          var wrapper = this._pending_stores.pop_head ();
+          if (wrapper != null)
+            {
+              wrapper.cb ();
+            }
+        }
+
+      return avatar_uri;
+    }
+
+  private async string _store_avatar_unlimited (string id, LoadableIcon avatar)
+      throws GLib.Error
+    {
       var dest_avatar_file = this._get_avatar_file (id);
 
       debug ("Storing avatar '%s' in file '%s'.", id,
@@ -264,3 +315,10 @@ public class Folks.AvatarCache : Object
         }
     }
 }
+
+/* See: https://mail.gnome.org/archives/vala-list/2011-June/msg00005.html */
+[Compact]
+private class DelegateWrapper
+{
+  public SourceFunc cb;
+}
diff --git a/tests/folks/avatar-cache.vala b/tests/folks/avatar-cache.vala
index c0f4808..d25245c 100644
--- a/tests/folks/avatar-cache.vala
+++ b/tests/folks/avatar-cache.vala
@@ -43,6 +43,7 @@ public class AvatarCacheTests : Folks.TestCase
       this.add_test ("store-and-load-avatar", this.test_store_and_load_avatar);
       this.add_test ("store-avatar-overwrite",
           this.test_store_avatar_overwrite);
+      this.add_test ("store-many-avatars", this.test_store_many_avatars);
       this.add_test ("load-avatar-non-existent",
           this.test_load_avatar_non_existent);
       this.add_test ("remove-avatar", this.test_remove_avatar);
@@ -216,6 +217,56 @@ public class AvatarCacheTests : Folks.TestCase
       this._assert_avatars_equal (this._avatar, avatar);
     }
 
+  public void test_store_many_avatars ()
+    {
+      /* Test storing hundreds of avatars in parallel. This should *not* cause
+       * the process to run out of FDs. */
+      const uint n_avatars = 1000;
+      uint n_remaining = n_avatars;
+
+      for (uint i = 0; i < n_avatars; i++)
+        {
+          var id = "test-store-many-avatars-%u".printf (i);
+
+          this._cache.store_avatar.begin (id, this._avatar, (obj, res) =>
+            {
+              try
+                {
+                  this._cache.store_avatar.end (res);
+                  n_remaining--;
+                }
+              catch (GLib.Error e)
+                {
+                  error ("Error storing avatar %u: %s", i, e.message);
+                }
+            });
+        }
+
+      /* Wait for all the operations to finish. The idle callback should always
+       * be queued behind all the I/O operations. */
+      while (n_remaining > 0)
+        {
+          Idle.add (() =>
+            {
+              this._main_loop.quit ();
+              return false;
+            });
+
+          this._main_loop.run ();
+        }
+
+      /* Load the avatars again and check they're all OK. */
+      for (uint i = 0; i < n_avatars; i++)
+        {
+          var id = "test-store-many-avatars-%u".printf (i);
+          var avatar = this._assert_load_avatar (id);
+
+          assert (avatar != null);
+          assert (avatar is LoadableIcon);
+          this._assert_avatars_equal (this._avatar, avatar);
+        }
+    }
+
   public void test_load_avatar_non_existent ()
     {
       // Load a non-existent avatar.