Commit 2001d7ad authored by Philip Withnall's avatar Philip Withnall
Browse files

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
parent 12b7985e
......@@ -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
......
......@@ -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
......@@ -143,6 +159,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);
......@@ -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;
}
......@@ -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.
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment