From 5f950cb71ac47c6c41df1565c0732a34d930a73f Mon Sep 17 00:00:00 2001
From: Cyrille Bagard <nocbos@gmail.com>
Date: Fri, 24 Aug 2018 01:43:36 +0200
Subject: Fixed a rare dead lock when loading several files at the same time.
---
src/analysis/loading.c | 1 +
src/analysis/project.c | 103 ++++++++++++++++++++++++++++++++++++-------------
2 files changed, 77 insertions(+), 27 deletions(-)
diff --git a/src/analysis/loading.c b/src/analysis/loading.c
index fa2118f..b8fbeea 100644
--- a/src/analysis/loading.c
+++ b/src/analysis/loading.c
@@ -765,6 +765,7 @@ void g_content_explorer_delete_group(GContentExplorer *explorer, wgroup_id_t wid
g_mutex_lock(&explorer->mutex);
group = g_content_explorer_find_group(explorer, wid);
+ assert(group != NULL);
/* Supression des contenus chargés */
diff --git a/src/analysis/project.c b/src/analysis/project.c
index 83b41c4..66faa16 100644
--- a/src/analysis/project.c
+++ b/src/analysis/project.c
@@ -853,33 +853,38 @@ static void g_loading_handler_dispose(GLoadingHandler *handler)
GContentResolver *resolver; /* Resolveur de contenus */
size_t i; /* Boucle de parcours */
- /* Supression des groupes de travail */
+ /**
+ * On se sert du projet comme sentinelle pour la validité des
+ * identifiants handler->exp_wids[i].
+ */
- explorer = get_current_content_explorer();
- resolver = get_current_content_resolver();
+ if (handler->project != NULL)
+ {
+ /* Supression des groupes de travail */
- g_signal_handlers_disconnect_by_func(explorer, G_CALLBACK(on_new_content_explored), handler);
- g_signal_handlers_disconnect_by_func(resolver, G_CALLBACK(on_new_content_resolved), handler);
+ explorer = get_current_content_explorer();
+ resolver = get_current_content_resolver();
- g_mutex_lock(&handler->mutex);
+ g_signal_handlers_disconnect_by_func(explorer, G_CALLBACK(on_new_content_explored), handler);
+ g_signal_handlers_disconnect_by_func(resolver, G_CALLBACK(on_new_content_resolved), handler);
- for (i = 0; i < handler->exp_count; i++)
- {
- g_content_resolver_delete_group(resolver, handler->exp_wids[i]);
- g_content_explorer_delete_group(explorer, handler->exp_wids[i]);
- }
+ for (i = 0; i < handler->exp_count; i++)
+ {
+ g_content_resolver_delete_group(resolver, handler->exp_wids[i]);
+ g_content_explorer_delete_group(explorer, handler->exp_wids[i]);
+ }
- g_mutex_unlock(&handler->mutex);
+ g_object_unref(G_OBJECT(explorer));
+ g_object_unref(G_OBJECT(resolver));
- g_object_unref(G_OBJECT(explorer));
- g_object_unref(G_OBJECT(resolver));
+ /* Nettoyage plus général */
- /* Nettoyage plus général */
+ g_mutex_clear(&handler->mutex);
+ g_cond_clear(&handler->wait_cond);
- g_mutex_clear(&handler->mutex);
- g_cond_clear(&handler->wait_cond);
+ g_clear_object(&handler->project);
- g_object_unref(G_OBJECT(handler->project));
+ }
G_OBJECT_CLASS(g_loading_handler_parent_class)->dispose(G_OBJECT(handler));
@@ -1089,13 +1094,61 @@ static bool g_loading_handler_check(GLoadingHandler *handler, wgroup_id_t wid)
bool result; /* Bilan à retourner */
size_t i; /* Boucle de parcours */
- assert(!g_mutex_trylock(&handler->mutex));
+ /**
+ * Les deux appelants on_new_content_explored() et on_new_content_resolved()
+ * ne doivent absolument pas poser un large verrou en filtrant les identifiants
+ * via cette fonction.
+ *
+ * On pose donc ce verrou ici.
+ *
+ * La raison mineure est que les deux appelants n'accèdent qu'en lecture
+ * (à peu de chose près) aux propriétés de l'objet handler.
+ *
+ * La raison principale est la suivante :
+ *
+ * - on_new_content_explored() crée un groupe de tâches via l'appel
+ * g_content_resolver_create_group().
+ *
+ * - on_new_content_resolved() peut conduire à une libération de l'objet
+ * handler, procédure qui va libérer ensuite le groupe de tâches associé.
+ *
+ * Ce qui peut conduire à un dead lock avec un large verrou :
+ *
+ * - un thread T1 termine une résolution wid=3 ; il va donc appeler
+ * tous les handlers via le signal "resolved".
+ *
+ * - T1 va rencontrer le handler qui gère wid=3. C'était la dernière
+ * résolution, donc un broadcast sur le compteur "resolved" va
+ * être émis.
+ *
+ * - un thread T2 en attente dans g_loading_handler_process() va donc
+ * terminer sa tâche. Depuis la fonction g_loading_handler_dispose(),
+ * cette tâche va libérer le groupe associé, dont l'exécution est
+ * assurée par T1.
+ *
+ * - le thread T2 va donc terminer sur un g_thread_join() dans la fonction
+ * g_work_group_dispose(), en attandant que T1 remarque l'ordre d'arrêt.
+ *
+ * - or T1 va continuer la propagation du signal "resolved" aux autres
+ * résolveurs (par exemple, celui gérant wid=4).
+ *
+ * - nouvelle exécution de on_new_content_resolved(), qui bloque cette
+ * fois, car le handler wid=4 est occupé dans un thread T3 à la fonction
+ * on_new_content_explored(), suite à un signal "explored" avec wid=4.
+ *
+ * - si le verrou handler->mutex est posé en même temps que la modification
+ * des groupes de tâches, alors T1, T2 et T3 vont se bloquer mutuellement.
+ */
+
+ g_mutex_lock(&handler->mutex);
result = false;
for (i = 0; i < handler->exp_count && !result; i++)
result = (handler->exp_wids[i] == wid);
+ g_mutex_unlock(&handler->mutex);
+
return result;
}
@@ -1122,8 +1175,6 @@ static void on_new_content_explored(GContentExplorer *explorer, wgroup_id_t wid,
GContentResolver *resolver; /* Resolveur de contenus */
size_t i; /* Boucle de parcours */
- g_mutex_lock(&handler->mutex);
-
if (g_loading_handler_check(handler, wid))
{
available = g_content_explorer_get_all(explorer, wid, &count);
@@ -1142,8 +1193,6 @@ static void on_new_content_explored(GContentExplorer *explorer, wgroup_id_t wid,
}
- g_mutex_unlock(&handler->mutex);
-
}
@@ -1173,8 +1222,6 @@ static void on_new_content_resolved(GContentResolver *resolver, wgroup_id_t wid,
xmlXPathObjectPtr xobject; /* Cible d'une recherche */
bool status; /* Bilan d'une restauration */
- g_mutex_lock(&handler->mutex);
-
if (g_loading_handler_check(handler, wid))
{
available = g_content_resolver_get_all(resolver, wid, &count);
@@ -1268,13 +1315,15 @@ static void on_new_content_resolved(GContentResolver *resolver, wgroup_id_t wid,
/* Si c'était la dernière résolution... */
+ g_mutex_lock(&handler->mutex);
+
handler->resolved++;
g_cond_broadcast(&handler->wait_cond);
- }
+ g_mutex_unlock(&handler->mutex);
- g_mutex_unlock(&handler->mutex);
+ }
}
--
cgit v0.11.2-87-g4458