Re: [Evolution-hackers] automated testing of Evolution data server with SyncEvolution

[Patrick Ohly <Patrick Ohly gmx de>]

On Mi, 2006-10-18 at 19:24 +0200, Patrick Ohly wrote:
> I found out what the problem is, see
> http://bugzilla.gnome.org/show_bug.cgi?id=363102
[...]
> There is a mutex, but none of the CORBA implementation functions lock
> it. Any suggestions how to fix this? As I said in that issue, several
> solutions come to mind:
> - lock the mutex inside the high-level e_cal_backend_* functions

I went for this one. In wasn't always sure for all calls whether a mutex
lock was necessary or not, so I might have added too many (affecting
performance and limiting concurrency?) as well as not adding enough
(f.i. during initialization and file opening/closing).

A first test run of SyncEvolution tests for the calendar backend
completed successfully and Evolution itself also still works. I'll wait
until the modified E-D-S has passed the full test suite for some days,
then attach the patch to the bugzilla entry and also add bugs to the
Debian tracker as discussed earlier.

In the meantime please find the current patch against 1.8.1 attached. As
usual, any comments are welcome.

-- 
Bye, Patrick Ohly
--  
Patrick Ohly gmx de
http://www.estamos.de/

#
# Patch managed by http://www.holgerschurig.de/patcher.html
#

--- evolution-data-server-1.8.1/calendar/backends/file/e-cal-backend-file.c~idle-save-locking
+++ evolution-data-server-1.8.1/calendar/backends/file/e-cal-backend-file.c
@@ -70,7 +70,12 @@
 	gboolean is_dirty;
 	guint dirty_idle_id;
 
-	GMutex *idle_save_mutex;
+	/* locked in high-level functions to ensure data is consistent
+	 * in idle and CORBA thread(s?); because high-level functions
+	 * may call other high-level functions the mutex must allow
+	 * recursive locking
+	 */
+	GStaticRecMutex idle_save_rmutex;
 
 	/* Toplevel VCALENDAR component */
 	icalcomponent *icalcomp;
@@ -135,10 +140,10 @@
 	g_assert (priv->uri != NULL);
 	g_assert (priv->icalcomp != NULL);
 
-	g_mutex_lock (priv->idle_save_mutex);
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
 	if (!priv->is_dirty) {
 		priv->dirty_idle_id = 0;
-		g_mutex_unlock (priv->idle_save_mutex);
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 		return FALSE;
 	}
 
@@ -193,18 +198,18 @@
 	priv->is_dirty = FALSE;
 	priv->dirty_idle_id = 0;
 
-	g_mutex_unlock (priv->idle_save_mutex);
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 
 	return FALSE;
 
  error_malformed_uri:
-	g_mutex_unlock (priv->idle_save_mutex);
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	e_cal_backend_notify_error (E_CAL_BACKEND (cbfile),
 				  _("Cannot save calendar data: Malformed URI."));
 	return TRUE;
 
  error:
-	g_mutex_unlock (priv->idle_save_mutex);
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	e_cal_backend_notify_error (E_CAL_BACKEND (cbfile), gnome_vfs_result_to_string (result));
 	return TRUE;
 }
@@ -216,13 +221,13 @@
 
 	priv = cbfile->priv;
 
-	g_mutex_lock (priv->idle_save_mutex);
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
 	priv->is_dirty = TRUE;
 
 	if (!priv->dirty_idle_id)
 		priv->dirty_idle_id = g_idle_add ((GSourceFunc) save_file_when_idle, cbfile);
 
-	g_mutex_unlock (priv->idle_save_mutex);
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 }
 
 static void
@@ -290,10 +295,7 @@
 		priv->dirty_idle_id = 0;
 	}
 
-	if (priv->idle_save_mutex) {
-		g_mutex_free (priv->idle_save_mutex);
-		priv->idle_save_mutex = NULL;
-	}
+	g_static_rec_mutex_free (&priv->idle_save_rmutex);
 
 	if (priv->uri) {
 	        g_free (priv->uri);
@@ -1085,9 +1087,13 @@
 	g_return_val_if_fail (uid != NULL, GNOME_Evolution_Calendar_ObjectNotFound);
 	g_assert (priv->comp_uid_hash != NULL);
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	obj_data = g_hash_table_lookup (priv->comp_uid_hash, uid);
-	if (!obj_data)
+	if (!obj_data) {
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 		return GNOME_Evolution_Calendar_ObjectNotFound;
+	}
 
 	if (rid && *rid) {
 		ECalComponent *comp;
@@ -1103,8 +1109,10 @@
 			icalcomp = e_cal_util_construct_instance (
 				e_cal_component_get_icalcomponent (obj_data->full_object),
 				itt);
-			if (!icalcomp)
+			if (!icalcomp) {
+				g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 				return GNOME_Evolution_Calendar_ObjectNotFound;
+                        }
 
 			*object = g_strdup (icalcomponent_as_ical_string (icalcomp));
 
@@ -1130,6 +1138,7 @@
 			*object = e_cal_component_get_as_string (obj_data->full_object);
 	}
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -1148,23 +1157,30 @@
 	g_return_val_if_fail (priv->icalcomp != NULL, GNOME_Evolution_Calendar_NoSuchCal);
 	g_return_val_if_fail (tzid != NULL, GNOME_Evolution_Calendar_ObjectNotFound);
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	if (!strcmp (tzid, "UTC")) {
 		zone = icaltimezone_get_utc_timezone ();
 	} else {
 		zone = icalcomponent_get_timezone (priv->icalcomp, tzid);
 		if (!zone) {
 			zone = icaltimezone_get_builtin_timezone_from_tzid (tzid);
-			if (!zone)
+			if (!zone) {
+				g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 				return GNOME_Evolution_Calendar_ObjectNotFound;
+			}
 		}
 	}
 	
 	icalcomp = icaltimezone_get_component (zone);
-	if (!icalcomp)
+	if (!icalcomp) {
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 		return GNOME_Evolution_Calendar_InvalidObject;
+	}
 
 	*object = g_strdup (icalcomponent_as_ical_string (icalcomp));
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -1192,11 +1208,14 @@
 
 		zone = icaltimezone_new ();
 		icaltimezone_set_component (zone, tz_comp);
+
+		g_static_rec_mutex_lock (&priv->idle_save_rmutex);
 		if (!icalcomponent_get_timezone (priv->icalcomp,
 						 icaltimezone_get_tzid (zone))) {
 			icalcomponent_add_component (priv->icalcomp, tz_comp);
 			save (cbfile);
 		}
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 
 		icaltimezone_free (zone, 1);
 	}
@@ -1226,11 +1245,13 @@
 	zone = icaltimezone_new ();
 	icaltimezone_set_component (zone, tz_comp);
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
 	if (priv->default_zone != icaltimezone_get_utc_timezone ())
 		icaltimezone_free (priv->default_zone, 1);
 
 	/* Set the default timezone to it. */
 	priv->default_zone = zone;
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 
 	return GNOME_Evolution_Calendar_Success;
 }
@@ -1303,7 +1324,9 @@
 	if (!match_data.obj_sexp)
 		return GNOME_Evolution_Calendar_InvalidQuery;
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
 	g_hash_table_foreach (priv->comp_uid_hash, (GHFunc) match_object_sexp, &match_data);
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 
 	*objects = match_data.obj_list;
 
@@ -1350,7 +1373,9 @@
 		return;
 	}
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
 	g_hash_table_foreach (priv->comp_uid_hash, (GHFunc) match_object_sexp, &match_data);
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 
 	/* notify listeners of all objects */
 	if (match_data.obj_list) {
@@ -1493,6 +1518,8 @@
 	g_return_val_if_fail (start != -1 && end != -1, GNOME_Evolution_Calendar_InvalidRange);
 	g_return_val_if_fail (start <= end, GNOME_Evolution_Calendar_InvalidRange);
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	*freebusy = NULL;
 	
 	if (users == NULL) {
@@ -1517,6 +1544,8 @@
 		}		
 	}
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
+
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -1564,6 +1593,7 @@
 
 	priv = cbfile->priv;
 
+
 	/* FIXME Will this always work? */
 	unescaped_uri = gnome_vfs_unescape_string (priv->uri, "");
 	filename = g_strdup_printf ("%s-%s.db", unescaped_uri, change_id);
@@ -1575,6 +1605,8 @@
 	
 	g_free (filename);
 	
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	/* Calculate adds and modifies */
 	for (i = priv->comp; i != NULL; i = i->next) {
 		const char *uid;
@@ -1615,6 +1647,7 @@
 	e_xmlhash_write (ehash);
   	e_xmlhash_destroy (ehash);
 	
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -1669,6 +1702,8 @@
 
 	g_return_val_if_fail (priv->icalcomp != NULL, NULL);
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	if (!strcmp (tzid, "UTC"))
 	        zone = icaltimezone_get_utc_timezone ();
 	else {
@@ -1677,6 +1712,7 @@
 			zone = icaltimezone_get_builtin_timezone_from_tzid (tzid);
 	}
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return zone;
 }
 
@@ -1755,6 +1791,8 @@
 		return GNOME_Evolution_Calendar_InvalidObject;
 	}
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	/* Get the UID */
 	comp_uid = icalcomponent_get_uid (icalcomp);
 	if (!comp_uid) {
@@ -1763,6 +1801,7 @@
 		new_uid = e_cal_component_gen_uid ();
 		if (!new_uid) {
 			icalcomponent_free (icalcomp);
+			g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 			return GNOME_Evolution_Calendar_InvalidObject;
 		}
 
@@ -1775,6 +1814,7 @@
 	/* check the object is not in our cache */
 	if (lookup_component (cbfile, comp_uid)) {
 		icalcomponent_free (icalcomp);
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 		return GNOME_Evolution_Calendar_ObjectIdAlreadyExists;
 	}
 
@@ -1801,6 +1841,7 @@
 		*uid = g_strdup (comp_uid);
 	*calobj = e_cal_component_get_as_string (comp);
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -1869,12 +1910,15 @@
 		return GNOME_Evolution_Calendar_InvalidObject;
 	}
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	/* Get the uid */
 	comp_uid = icalcomponent_get_uid (icalcomp);
 
 	/* Get the object from our cache */
 	if (!(obj_data = g_hash_table_lookup (priv->comp_uid_hash, comp_uid))) {
 		icalcomponent_free (icalcomp);
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 		return GNOME_Evolution_Calendar_ObjectNotFound;
 	}
 
@@ -1912,6 +1956,7 @@
 
 			save (cbfile);
 
+			g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 			return GNOME_Evolution_Calendar_Success;
 		}
 
@@ -2048,6 +2093,7 @@
 
 	save (cbfile);
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -2128,9 +2174,13 @@
 
 	*old_object = *object = NULL;
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	obj_data = g_hash_table_lookup (priv->comp_uid_hash, uid);
-	if (!obj_data)
+	if (!obj_data) {
+		g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 		return GNOME_Evolution_Calendar_ObjectNotFound;
+	}
 	
 	if (rid && *rid)
 		recur_id = rid;
@@ -2159,8 +2209,10 @@
 		break;
 	case CALOBJ_MOD_THISANDPRIOR :
 	case CALOBJ_MOD_THISANDFUTURE :
-		if (!recur_id || !*recur_id)
+		if (!recur_id || !*recur_id) {
+			g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 			return GNOME_Evolution_Calendar_ObjectNotFound;
+		}
 
 		*old_object = e_cal_component_get_as_string (comp);
 
@@ -2190,6 +2242,7 @@
 
 	save (cbfile);
 
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return GNOME_Evolution_Calendar_Success;
 }
 
@@ -2349,6 +2402,8 @@
 	if (!toplevel_comp)
 		return GNOME_Evolution_Calendar_InvalidObject;
 
+	g_static_rec_mutex_lock (&priv->idle_save_rmutex);
+
 	kind = icalcomponent_isa (toplevel_comp);
 	if (kind != ICAL_VCALENDAR_COMPONENT) {
 		/* If its not a VCALENDAR, make it one to simplify below */
@@ -2538,7 +2593,7 @@
 
  error:
 	g_hash_table_destroy (tzdata.zones);
-	
+	g_static_rec_mutex_unlock (&priv->idle_save_rmutex);
 	return status;
 }
 
@@ -2566,7 +2621,7 @@
 	priv->read_only = FALSE;
 	priv->is_dirty = FALSE;
 	priv->dirty_idle_id = 0;
-	priv->idle_save_mutex = g_mutex_new ();
+	g_static_rec_mutex_init (&priv->idle_save_rmutex);
 	priv->icalcomp = NULL;
 	priv->comp_uid_hash = NULL;
 	priv->comp = NULL;