Re: dataset thread savety

[Sebastian Wilhelmi <wilhelmi ira uka de>]

Hi,

> in your initial thread-safety attempt at gobject, you did:
> 
> +/* Making this lock more fine grained (per pspec) doesn't seem
> + * necessary. These calls has to be protected though
> + */
> +G_LOCK_DEFINE_STATIC (pspec_qdata);
> +
>  gpointer
>  g_param_spec_get_qdata (GParamSpec *pspec,
>                         GQuark      quark)
>  {
> +  gpointer data;
> +
>    g_return_val_if_fail (G_IS_PARAM_SPEC (pspec), NULL);
> 
> -  return quark ? g_datalist_id_get_data (&pspec->qdata, quark) : NULL;
> +  G_LOCK (pspec_qdata);
> +  data = quark ? g_datalist_id_get_data (&pspec->qdata, quark) : NULL;
> +  G_UNLOCK (pspec_qdata);
> +
> +  return data;
>  }
> 
> i don't think this makes much sense.
> g_datalist_ is already guarded by the global dataset lock (and suffers
> from it) for the modification functions, the only exceptionally non-guarded
> functions in gdataset.c are g_datalist_id_get_data() and g_datalist_foreach().

Yes, I agree. When I added that locks I didn't think about that, but in fact
it seems sensible in this very case to make those two glib structures fully
thread safe with the notable exception of _foreach. See below
 
> rather than forcing users into having their own locks around these two
> functions only, i think we should move to a global rw lock for datalists
> that also protects g_datalist_id_get_data() and g_datalist_foreach() (for the
> latter, it's probably best to walk the list one node ahead like we do
> for most other foreach functions, untill we move to array storage).

I have patched gdataset.c that way. It is appended. 

There are no problems for the get_data functions. However the foreach
functions cause me some headache. I'd say, it is impossible to make them
thread-safe without calling 'func' within the global lock, which would either
require recursive mutexes or disallowing the call of g_data* functions from
within 'func', the latter being a bad idea I think.

Even if we would copy the datalist to an alloca'ed array and walk it there we
could still call 'func' for values, that have already disappeared from the
original list and might already been freed or otherwise inaccessable. So I
have implemented the 'one node ahead' walking of the list, which at least ist
cheap.

What do you all think?

Ciao,
Sebastian
-- 
Sebastian Wilhelmi
mailto:wilhelmi ira uka de
http://goethe.ira.uka.de/~wilhelmi

Index: gdataset.c
===================================================================
RCS file: /cvs/gnome/glib/gdataset.c,v
retrieving revision 1.16
diff -u -b -B -r1.16 gdataset.c
--- gdataset.c	2000/07/26 11:01:59	1.16
+++ gdataset.c	2001/02/06 11:57:11
@@ -421,6 +421,8 @@
 g_dataset_id_get_data (gconstpointer  dataset_location,
 		       GQuark         key_id)
 {
+  gpointer ret_data = NULL;
+
   g_return_val_if_fail (dataset_location != NULL, NULL);
   
   G_LOCK (g_dataset_global);
@@ -436,32 +438,39 @@
 	  for (list = dataset->datalist; list; list = list->next)
 	    if (list->id == key_id)
 	      {
-		G_UNLOCK (g_dataset_global);
-		return list->data;
+		ret_data = list->data;
+		break;
 	      }
 	}
     }
   G_UNLOCK (g_dataset_global);
  
-  return NULL;
+  return ret_data;
 }
 
 gpointer
 g_datalist_id_get_data (GData	 **datalist,
 			GQuark     key_id)
 {
+  gpointer ret_data = NULL;
+  
   g_return_val_if_fail (datalist != NULL, NULL);
   
+  G_LOCK (g_dataset_global);
   if (key_id)
     {
       register GData *list;
       
       for (list = *datalist; list; list = list->next)
 	if (list->id == key_id)
-	  return list->data;
+	  {
+	    ret_data = list->data;
+	    break;
+	  }
     }
+  G_UNLOCK (g_dataset_global);
   
-  return NULL;
+  return ret_data;
 }
 
 void
@@ -481,16 +490,22 @@
       G_UNLOCK (g_dataset_global);
       if (dataset)
 	{
-	  register GData *list;
+	  register GData *list = dataset->datalist;
 	  
-	  for (list = dataset->datalist; list; list = list->next)
-	      func (list->id, list->data, user_data);
-	}
-    }
-  else
+	  while (list)
     {
+	      register GQuark id = list->id;
+	      register gpointer data = list->data;
+	      
+	      list = list->next;
+	      
       G_UNLOCK (g_dataset_global);
+	      func (id, data, user_data);
+	      G_LOCK (g_dataset_global);
+	    }
     }
+    }
+  G_UNLOCK (g_dataset_global);
 }
 
 void
@@ -503,8 +518,20 @@
   g_return_if_fail (datalist != NULL);
   g_return_if_fail (func != NULL);
   
-  for (list = *datalist; list; list = list->next)
-    func (list->id, list->data, user_data);
+  G_LOCK (g_dataset_global);
+  list = *datalist;
+  while (list)
+    {
+      register GQuark id = list->id;
+      register gpointer data = list->data;
+      
+      list = list->next;
+      
+      G_UNLOCK (g_dataset_global);
+      func (id, data, user_data);
+      G_LOCK (g_dataset_global);
+    }
+  G_UNLOCK (g_dataset_global);
 }
 
 void