[Evolution-hackers] camel-folder-summary locking ...

[Michael Meeks <michael meeks novell com>]

Hi guys,

	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:

Thread 121 (Thread 0xa7f37b70 (LWP 23631)):
#0  0xb68c13ef in g_hash_table_resize (hash_table=<value optimized out>) at ghash.c:424
#1  g_hash_table_maybe_resize (hash_table=<value optimized out>) at ghash.c:457
#2  g_hash_table_remove_internal (hash_table=<value optimized out>) at ghash.c:982
#3  0xb7c7fdba in message_info_free (s=0x8e7e438, info=0x90f89f0) at camel-folder-summary.c:3497
#4  0xb7c7fe75 in camel_message_info_free (o=0x90f89f0) at camel-folder-summary.c:4518
#5  0xb7c7ffce in remove_item (key=0x9142618 "217647", info=0x90f89f0, s=0x8e7e438) at camel-folder-summary.c:788
#6  0xb68c0b2c in g_hash_table_foreach_remove_or_steal (hash_table=0x8f7d0c0 = {...}, func=0xb7c7ff40 <remove_item>, user_data=0x8e7e438, notify=1)
    at ghash.c:1109
#7  0xb7c85356 in remove_cache (session=0x80ce680, msg=0xb093250) at camel-folder-summary.c:818
#8  0xb7ca0162 in session_thread_proxy (msg=0xb093250, session=0x80ce680) at camel-session.c:590
#9  0xb68fb036 in g_thread_pool_thread_proxy (data=0x8aa0cb8) at gthreadpool.c:265
---Type <return> to continue, or q <return> to quit---
#10 0xb68f99ef in g_thread_create_proxy (data=0xb0c1ee8) at gthread.c:635
#11 0xb764c725 in start_thread () from /lib/libpthread.so.0
#12 0xb680626e in clone () from /lib/libc.so.6

	Having said that, the code looks a bit nervous (judging by the
comments) in this area. Review much appreciated etc. Reading through the
code there though, I too began to get a bit nervous trying to keep track
of what locks are held.

eg.
	camel_folder_summary_migrate_infos
		+ doesn't seem to take the summary_lock - why ?

	likewise 'message_info_free'
		+ called from camel_message_info_free
			+ doesn't seem to take the lock ... - why ?
			  if it is just assumed then:
	
	+ most callers of camel_message_info_free *have* that lock,
		+ except eg. camel_folder_summary_remove ...
		+ or eg. camel_folder_summary_remove_index_fast
		+ or summary_header_save, or message_info_load,
	 	+ or msg_update_preview, 
		+ or camel_folder_summary_migrate_infos

	Soo ... if you read the GType / GObject code - which is similarly
twisted / complicated - then it uses name mangling to denote what lock
is held (personally, I rather like that). Thus you can see by reading
it:

	do_foo()
	{
		take_lock();
		do_foo_T();
		release_lock();
	}

	is safe, and

	do_foo_T() { do_baa(); }

	is essentially not :-) (the '_T' suffix needs propagating). ORBit2 does
the same where things get sticky.

	Failing that, the linux kernel seems in places to use comments of
the form:

 * The files->file_lock should be held on entry, and will be held on exit.

	Clearly, something to reduce the barrier to entry here such that the
code is more readable would be good ;-)

	HTH,

		Michael.

-- 
 michael meeks novell com  <><, Pseudo Engineer, itinerant idiot

diff --git a/camel/camel-folder-summary.c b/camel/camel-folder-summary.c
index 85265ac..787f229 100644
--- a/camel/camel-folder-summary.c
+++ b/camel/camel-folder-summary.c
@@ -778,18 +778,15 @@ 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);
+
+		/* free the entry later */
+		*to_free_list = g_slist_prepend (*to_free_list, info);
 		return TRUE;
 	}
 	CAMEL_SUMMARY_UNLOCK(info->summary, ref_lock);
@@ -807,6 +804,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, *l;
 
 	CAMEL_DB_RELEASE_SQLITE_MEMORY;
 	camel_exception_init (&ex);
@@ -819,7 +817,14 @@ 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);
+	g_hash_table_foreach_remove  (s->loaded_infos, (GHRFunc) remove_item, &to_free_list);
+
+	/* 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)));