On Fri, 2010-02-26 at 22:20 +0000, Michael Meeks wrote: 
>       I just hit a nasty in camel-folder-summary; suggested patch attached,
> seemingly a simple problem of a re-enterancy hazard in the same thread
> with a ghashtable:

        Ok; this was fixed by Chen's reversion of freeing the uid and NULL'ing
it (recently). However reading the code some more, I think we can do a
lot better, and more cleanly.

        I attach the patch; discussed with Chen & Srini; I just committed to
gnome-2-28 and master.



 michael.me...@novell.com  <><, Pseudo Engineer, itinerant idiot

>From c5a86b33ef7d10d25b227221efec8106d8c82ed9 Mon Sep 17 00:00:00 2001
From: Michael Meeks <michael.me...@novell.com>
Date: Mon, 1 Mar 2010 11:23:18 +0000
Subject: [PATCH] Fix unpleasant issue with (remove_item) more elegantly. Instead of
 taking an untaking the ref-lock thousands of times, and trying to
 free messages inside the loop (while cobbering their uid to encourage
 message_info_free not to do an unsave hash table removal, inside the
 g_hash_table_foreach_remove loop).
 Instead - we now just build a list very quickly of the messages we
 want to drop. As such, the hash table iteration will be ~instant,
 and we do not need to take / drop the lock each iteration, simplifying
 and perhaps accelerating the code. We then free the messages later
 before releasing the summary lock.

 camel/camel-folder-summary.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/camel/camel-folder-summary.c b/camel/camel-folder-summary.c
index 85265ac..3218a88 100644
--- a/camel/camel-folder-summary.c
+++ b/camel/camel-folder-summary.c
@@ -778,21 +778,13 @@ cfs_count_dirty (CamelFolderSummary *s)
 /* FIXME[disk-summary] I should have a better LRU algorithm  */
 static gboolean
-remove_item (gchar *key, CamelMessageInfoBase *info, CamelFolderSummary *s)
+remove_item (gchar *key, CamelMessageInfoBase *info, GSList **to_free_list)
 	d(printf("%d(%d)\t", info->refcount, info->dirty)); /* camel_message_info_dump (info); */
-	CAMEL_SUMMARY_LOCK(info->summary, ref_lock);
 	if (info->refcount == 1 && !info->dirty && !(info->flags & CAMEL_MESSAGE_FOLDER_FLAGGED)) {
-		CAMEL_SUMMARY_UNLOCK(info->summary, ref_lock);
-		/* Hackit so that hashtable isn;t corrupted. */
-		/* FIXME: These uid strings are not yet freed. We should get this done soon. */
-		camel_pstring_free (info->uid);
-		info->uid = NULL;
-		/* Noone seems to need it. Why not free it then. */
-		camel_message_info_free (info);
+		*to_free_list = g_slist_prepend (*to_free_list, info);
 		return TRUE;
-	CAMEL_SUMMARY_UNLOCK(info->summary, ref_lock);
 	return FALSE;
@@ -807,6 +799,7 @@ remove_cache (CamelSession *session, CamelSessionThreadMsg *msg)
 	struct _folder_summary_free_msg *m = (struct _folder_summary_free_msg *)msg;
 	CamelFolderSummary *s = m->summary;
 	CamelException ex;
+	GSList *to_free_list = NULL, *l;
 	camel_exception_init (&ex);
@@ -819,7 +812,17 @@ remove_cache (CamelSession *session, CamelSessionThreadMsg *msg)
 	dd(printf("removing cache for  %s %d %p\n", s->folder ? s->folder->full_name : s->summary_path, g_hash_table_size (s->loaded_infos), (gpointer) s->loaded_infos));
 	/* FIXME[disk-summary] hack. fix it */
 	CAMEL_SUMMARY_LOCK (s, summary_lock);
-	g_hash_table_foreach_remove  (s->loaded_infos, (GHRFunc) remove_item, s);
+	CAMEL_SUMMARY_LOCK(s, ref_lock);
+	g_hash_table_foreach_remove  (s->loaded_infos, (GHRFunc) remove_item, &to_free_list);
+	CAMEL_SUMMARY_UNLOCK(s, ref_lock);
+	/* Deferred freeing as _free function will try to remove
+	   entries from the hash_table in foreach_remove otherwise */
+	for (l = to_free_list; l; l = l->next)
+		camel_message_info_free (l->data);
+	g_slist_free (to_free_list);
 	CAMEL_SUMMARY_UNLOCK (s, summary_lock);
 	dd(printf("done .. now %d\n",g_hash_table_size (s->loaded_infos)));

Evolution-hackers mailing list

Reply via email to