From 9109e6a456c61695b532d0902b992c121d675f5e Mon Sep 17 00:00:00 2001
From: Andrew Chadwick <a.t.chadwick@gmail.com>
Date: Wed, 5 Apr 2023 23:48:30 +0000
Subject: [PATCH] ThunarFile cache: improve mutex usage
MR !264
Fixes #1060
---
thunar/thunar-file.c | 189 ++++++++++++++++++++++++++++---------------
1 file changed, 124 insertions(+), 65 deletions(-)
diff --git a/thunar/thunar-file.c b/thunar/thunar-file.c
index cd67cd7ff..aad198ce0 100644
--- a/thunar/thunar-file.c
+++ b/thunar/thunar-file.c
@@ -111,13 +111,16 @@ static void thunar_file_monitor (GFileMonitor
GFileMonitorEvent event_type,
gpointer user_data);
static void thunar_file_watch_reconnect (ThunarFile *file);
+static gboolean thunar_file_load_unguarded (ThunarFile *file,
+ GCancellable *cancellable,
+ GError **error);
static gboolean thunar_file_load (ThunarFile *file,
GCancellable *cancellable,
GError **error);
static gboolean thunar_file_is_readable (const ThunarFile *file);
static gboolean thunar_file_same_filesystem (const ThunarFile *file_a,
const ThunarFile *file_b);
-
+static ThunarFile *thunar_file_cache_lookup_unguarded (const GFile *file);
G_LOCK_DEFINE_STATIC (file_cache_mutex);
@@ -125,7 +128,6 @@ G_LOCK_DEFINE_STATIC (file_content_type_mutex);
G_LOCK_DEFINE_STATIC (file_rename_mutex);
-
static ThunarUserManager *user_manager;
static GHashTable *file_cache;
static guint32 effective_user_id;
@@ -1175,24 +1177,15 @@ thunar_file_get_async_finish (GObject *object,
-/**
- * thunar_file_load:
- * @file : a #ThunarFile.
- * @cancellable : a #GCancellable.
- * @error : return location for errors or %NULL.
+/* Static, no-mutex version of thunar_file_load().
*
- * Loads all information about the file. As this is a possibly
- * blocking call, it can be cancelled using @cancellable.
- *
- * If loading the file fails or the operation is cancelled,
- * @error will be set.
- *
- * Return value: %TRUE on success, %FALSE on error or interruption.
- **/
+ * The calling function must execute G_LOCK(file_cache_mutex) before calling this
+ * function, and G_UNLOCK(file_cache_mutex) afterwards. */
+
static gboolean
-thunar_file_load (ThunarFile *file,
- GCancellable *cancellable,
- GError **error)
+thunar_file_load_unguarded (ThunarFile *file,
+ GCancellable *cancellable,
+ GError **error)
{
GError *err = NULL;
@@ -1202,10 +1195,8 @@ thunar_file_load (ThunarFile *file,
_thunar_return_val_if_fail (G_IS_FILE (file->gfile), FALSE);
/* remove the file from cache */
- G_LOCK (file_cache_mutex);
if (g_hash_table_lookup (file_cache, file->gfile) != NULL)
g_hash_table_remove (file_cache, file->gfile);
- G_UNLOCK (file_cache_mutex);
/* reset the file */
thunar_file_info_clear (file);
@@ -1237,17 +1228,53 @@ thunar_file_load (ThunarFile *file,
/* (re)insert the file into the cache */
if (file != NULL && file->kind != G_FILE_TYPE_UNKNOWN)
{
- G_LOCK (file_cache_mutex);
g_hash_table_insert (file_cache,
g_object_ref (file->gfile),
weak_ref_new (G_OBJECT (file)));
- G_UNLOCK (file_cache_mutex);
}
return TRUE;
}
+/**
+ * thunar_file_load:
+ * @file : a #ThunarFile.
+ * @cancellable : a #GCancellable.
+ * @error : return location for errors or %NULL.
+ *
+ * Loads all information about the file. As this is a possibly
+ * blocking call, it can be cancelled using @cancellable.
+ *
+ * If loading the file fails or the operation is cancelled,
+ * @error will be set.
+ *
+ * Return value: %TRUE on success, %FALSE on error or interruption.
+ **/
+static gboolean
+thunar_file_load (ThunarFile *file,
+ GCancellable *cancellable,
+ GError **error)
+{
+ gboolean result = FALSE;
+
+ _thunar_return_val_if_fail (THUNAR_IS_FILE (file), FALSE);
+ _thunar_return_val_if_fail (error == NULL || *error == NULL, FALSE);
+ _thunar_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), FALSE);
+ _thunar_return_val_if_fail (G_IS_FILE (file->gfile), FALSE);
+
+ /* Ensure that all lookups and inserts happen under the same mutex */
+ G_LOCK (file_cache_mutex);
+
+ result = thunar_file_load_unguarded (file, cancellable, error);
+
+ G_UNLOCK (file_cache_mutex);
+
+ return result;
+}
+
+
+
/**
* thunar_file_get:
* @file : a #GFile.
@@ -1266,16 +1293,21 @@ ThunarFile*
thunar_file_get (GFile *gfile,
GError **error)
{
- ThunarFile *file;
+ ThunarFile *file = NULL;
+ gboolean file_load_failed = FALSE;
_thunar_return_val_if_fail (G_IS_FILE (gfile), NULL);
+ /* Both lookup and insert must happen in the same critical section
+ * because the insert is contigent upon the lookup */
+ G_LOCK (file_cache_mutex);
+
/* check if we already have a cached version of that file */
- file = thunar_file_cache_lookup (gfile);
+ file = thunar_file_cache_lookup_unguarded (gfile);
if (G_UNLIKELY (file != NULL))
{
/* return the file, it already has an additional ref set
- * in thunar_file_cache_lookup */
+ * in thunar_file_cache_lookup_unguarded */
}
else
{
@@ -1283,27 +1315,33 @@ thunar_file_get (GFile *gfile,
file = g_object_new (THUNAR_TYPE_FILE, NULL);
file->gfile = g_object_ref (gfile);
- if (thunar_file_load (file, NULL, error))
+ /* load and cache the file, using the current lock */
+ if (thunar_file_load_unguarded (file, NULL, error))
{
- /* setup lock until the file is inserted */
- G_LOCK (file_cache_mutex);
-
- /* insert the file into the cache */
- g_hash_table_insert (file_cache,
- g_object_ref (file->gfile),
- weak_ref_new (G_OBJECT (file)));
-
- /* done inserting in the cache */
- G_UNLOCK (file_cache_mutex);
+ /* just check that it's been cached, if appropriate */
+ if ((file != NULL) && (file->kind != G_FILE_TYPE_UNKNOWN))
+ _thunar_assert (g_hash_table_contains (file_cache, file->gfile) == TRUE);
}
else
{
- /* failed loading, destroy the file */
+ /* failed loading, destroy the file (outside the lock) */
+ file_load_failed = TRUE;
+ }
+ }
+
+ /* Finished related activity on the cache */
+ G_UNLOCK (file_cache_mutex);
+
+ if (file_load_failed == TRUE)
+ {
+ if (file != NULL)
+ {
+ /* Calls thunar_file_finalize(), which itself requires the lock */
g_object_unref (file);
- /* make sure we return NULL */
- file = NULL;
}
+
+ return NULL;
}
return file;
@@ -1340,12 +1378,16 @@ thunar_file_get_with_info (GFile *gfile,
_thunar_return_val_if_fail (G_IS_FILE (gfile), NULL);
_thunar_return_val_if_fail (G_IS_FILE_INFO (info), NULL);
+ /* All contingent lookups and inserts must happen in the same critical section */
+ G_LOCK (file_cache_mutex);
+
/* check if we already have a cached version of that file */
- file = thunar_file_cache_lookup (gfile);
+ file = thunar_file_cache_lookup_unguarded (gfile);
+
if (G_UNLIKELY (file != NULL))
{
/* return the file, it already has an additional ref set
- * in thunar_file_cache_lookup */
+ * in thunar_file_cache_lookup_unguarded */
}
else
{
@@ -1366,18 +1408,16 @@ thunar_file_get_with_info (GFile *gfile,
if (not_mounted)
FLAG_UNSET (file, THUNAR_FILE_FLAG_IS_MOUNTED);
- /* setup lock until the file is inserted */
- G_LOCK (file_cache_mutex);
-
/* insert the file into the cache */
g_hash_table_insert (file_cache,
g_object_ref (file->gfile),
weak_ref_new (G_OBJECT (file)));
- /* done inserting in the cache */
- G_UNLOCK (file_cache_mutex);
}
-
+
+ /* done reading and writing the cache for this file instance */
+ G_UNLOCK (file_cache_mutex);
+
if (recent_info != NULL)
file->recent_info = g_object_ref (recent_info);
@@ -4605,6 +4645,42 @@ thunar_file_same_filesystem (const ThunarFile *file_a,
+/* Static, no-mutex version of thunar_file_cache_lookup().
+ *
+ * The calling function must execute G_LOCK(file_cache_mutex) before calling this
+ * function, and G_UNLOCK(file_cache_mutex) afterwards.
+ *
+ * This is responsible for creating the file cache too. */
+
+static ThunarFile *
+thunar_file_cache_lookup_unguarded (const GFile *file)
+{
+ GWeakRef *ref;
+ ThunarFile *cached_file;
+
+ _thunar_return_val_if_fail (G_IS_FILE (file), NULL);
+
+ /* allocate the ThunarFile cache on-demand */
+ if (G_UNLIKELY (file_cache == NULL))
+ {
+ file_cache = g_hash_table_new_full (g_file_hash,
+ (GEqualFunc) g_file_equal,
+ (GDestroyNotify) g_object_unref,
+ (GDestroyNotify) weak_ref_free);
+ }
+
+ ref = g_hash_table_lookup (file_cache, file);
+
+ if (ref == NULL)
+ cached_file = NULL;
+ else
+ cached_file = g_weak_ref_get (ref);
+
+ return cached_file;
+}
+
+
+
/**
* thunar_file_cache_lookup:
* @file : a #GFile.
@@ -4625,29 +4701,12 @@ thunar_file_same_filesystem (const ThunarFile *file_a,
ThunarFile *
thunar_file_cache_lookup (const GFile *file)
{
- GWeakRef *ref;
ThunarFile *cached_file;
_thunar_return_val_if_fail (G_IS_FILE (file), NULL);
G_LOCK (file_cache_mutex);
-
- /* allocate the ThunarFile cache on-demand */
- if (G_UNLIKELY (file_cache == NULL))
- {
- file_cache = g_hash_table_new_full (g_file_hash,
- (GEqualFunc) g_file_equal,
- (GDestroyNotify) g_object_unref,
- (GDestroyNotify) weak_ref_free);
- }
-
- ref = g_hash_table_lookup (file_cache, file);
-
- if (ref == NULL)
- cached_file = NULL;
- else
- cached_file = g_weak_ref_get (ref);
-
+ cached_file = thunar_file_cache_lookup_unguarded (file);
G_UNLOCK (file_cache_mutex);
return cached_file;
--
GitLab