Fixed a rare dead lock when loading several files at the same time.

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);
+    }
 
 }