Re: Proposal: giop-thread-invocation-stack

[Frank Rehberger <Frank Rehberger web de>]

Michael Meeks wrote:

>Hi Frank,
>
>On Fri, 2003-11-07 at 01:45, Frank Rehberger wrote:
>  
>
>>this is first proposal of thread aware handling of invocation stacks.
>>"make check" runs successfully, but it might be wise to test it with 
>>dev-build of gnome-2.6 also.
>>
>>any comments?
>>    
>>
>	I think that we need to do a re-factor first to make this more elegant.
>My feeling is that in the non-threaded case; we want to return a
>GIOPThread * from giop_thread_self that is the 'main thread' [ we can
>use a static local variable to store this one-off ].
>
For long term, yes, but currently I am short of time.  I think, at first 
the bug in multi-threaded environment should be fixed,  may I commit?

Cheers, Frank


? src/orb/orb-core/bak.corba-orb.c
? src/orb/orb-core/bak.corba-object.c
? src/orb/poa/data
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/ORBit2/ChangeLog,v
retrieving revision 1.607
diff -u -b -r1.607 ChangeLog
--- ChangeLog	5 Dec 2003 17:29:17 -0000	1.607
+++ ChangeLog	6 Dec 2003 01:26:25 -0000
@@ -1,3 +1,18 @@
+2003-12-06  Frank Rehberger  <frehberg@xtradyne.de>
+
+	* src/orb/poa/poa.c (ORBit_POA_invocation_stack_push)
+	(ORBit_POA_invocation_stack_pop, pobj_has_servant)
+	(ORBit_POA_invocation_stack_lookup_objid)
+	(ORBit_POA_invocation_stack_lookup_objref)
+	(ORBit_POA_invocation_stack_peek): move stack management into
+	module GIOP/giop.c for all concurrency-policies.
+
+	* src/orb/GIOP/giop.c (giop_thread_invocation_stack_first_equal)
+	(giop_thread_invocation_stack_push)
+	(giop_thread_invocation_stack_pop): invocation stack management,
+	in case of !giop_thread_safe(), robust handling of GIOPThread
+	object being NULL.
+	
 2003-12-06  Michael Meeks  <michael@ximian.com>
 
 	* src/orb/GIOP/giop.c (wakeup_mainloop): don't spin on
Index: src/orb/GIOP/giop.c
===================================================================
RCS file: /cvs/gnome/ORBit2/src/orb/GIOP/giop.c,v
retrieving revision 1.46
diff -u -b -r1.46 giop.c
--- src/orb/GIOP/giop.c	5 Dec 2003 17:29:19 -0000	1.46
+++ src/orb/GIOP/giop.c	6 Dec 2003 01:26:28 -0000
@@ -25,6 +25,9 @@
 static GMutex      *giop_pool_hash_lock = NULL;
 static GHashTable  *giop_pool_hash      = NULL;
 
+/* invocation stack for non-threded setup */ 
+GSList *giop_current_invocations = NULL;
+
 const char giop_version_ids [GIOP_NUM_VERSIONS][2] = {
 	{1,0},
 	{1,1},
@@ -192,6 +195,7 @@
 	g_free (dirname);
 }
 
+
 gboolean
 giop_thread_safe (void)
 {
@@ -282,6 +286,7 @@
 	tdata->keys = NULL;
 	tdata->async_ents = NULL;
 	tdata->request_queue = NULL;
+	tdata->current_invocations = NULL;
 
 	if (giop_main_thread)
 		tdata->request_handler = giop_main_thread->request_handler;
@@ -336,6 +341,8 @@
 		g_warning ("Leaked async ents");
 	if (tdata->request_queue)
 		g_warning ("Leaked request queue");
+	if (tdata->current_invocations)
+		g_warning ("Leaked invocation stack");
 #endif
 	if (tdata->invoke_policies)
 		g_queue_free (tdata->invoke_policies);
@@ -361,7 +368,6 @@
 	return tdata;
 }
 
-
 void
 giop_thread_key_add (GIOPThread *tdata, gpointer key)
 {
@@ -643,6 +649,9 @@
 			WAKEUP_POLL = -1;
 		}
 	}
+
+	if (giop_thread_list) 
+		g_warning ("Leaked thread list");
 }
 
 typedef struct {
@@ -825,3 +834,108 @@
 	    !link_thread_io ())
 		link_set_io_thread (TRUE);
 }
+
+gpointer
+giop_thread_invocation_stack_first_equal (GIOPThread* tdata,
+					  gboolean (*equal)(gconstpointer stack_ptr, gconstpointer user_data),
+					  gpointer user_data) 
+{
+	gpointer res = NULL;
+	GSList *l;
+
+	g_assert (tdata || !giop_thread_safe ());
+
+	if (giop_thread_safe ()) {
+		LINK_MUTEX_LOCK (tdata->lock);
+		for (l = tdata->current_invocations; l && !res; l = l->next) {
+			gpointer pobj = l->data;
+			
+			if (equal (pobj, user_data)) {
+				res = pobj;
+				break;
+			}
+		}
+		LINK_MUTEX_UNLOCK (tdata->lock);
+	} else {
+		for (l = giop_current_invocations; l && !res; l = l->next) {
+			gpointer pobj = l->data;
+			
+			if (equal (pobj, user_data)) {
+				res = pobj;
+				break;
+			}
+		}
+	}
+
+	return NULL;
+}
+
+gpointer
+giop_thread_invocation_stack_push (GIOPThread* tdata, gpointer elem)
+{
+	g_assert (tdata || !giop_thread_safe ());
+
+	if (giop_thread_safe ()) {
+		LINK_MUTEX_LOCK (tdata->lock);
+		tdata->current_invocations 
+			= g_slist_prepend (tdata->current_invocations, elem);
+		LINK_MUTEX_UNLOCK (tdata->lock);
+	} else {
+		/* non threaded */ 
+		giop_current_invocations 
+			= g_slist_prepend (giop_current_invocations, elem);
+	}
+
+	return elem;
+}
+
+gpointer
+giop_thread_invocation_stack_pop (GIOPThread* tdata)
+{
+	gpointer elem;
+	GSList* top = NULL;
+
+	g_assert (tdata || !giop_thread_safe ());
+
+	if (giop_thread_safe ()) {
+		g_assert (tdata->current_invocations!=NULL);
+
+		LINK_MUTEX_LOCK (tdata->lock);
+		top  = tdata->current_invocations;
+		elem = top->data;
+		tdata->current_invocations = (tdata->current_invocations)->next;
+		g_slist_free_1 (top);
+		LINK_MUTEX_UNLOCK (tdata->lock);
+	} else {
+		/* non threaded */ 
+		g_assert (giop_current_invocations!=NULL);
+
+		top  = giop_current_invocations;
+		elem = top->data;
+		giop_current_invocations = (giop_current_invocations)->next;
+		g_slist_free_1 (top);		
+	}
+
+	return elem;
+}
+
+gpointer
+giop_thread_invocation_stack_peek (GIOPThread *tdata)
+{
+	gpointer res = NULL;
+	
+	g_assert (tdata || !giop_thread_safe ());
+
+	if (giop_thread_safe ()) {
+		LINK_MUTEX_LOCK (tdata->lock);
+		if (tdata->current_invocations)
+			res = tdata->current_invocations->data;
+		LINK_MUTEX_UNLOCK (tdata->lock);
+	} else {
+		if (giop_current_invocations)
+			res = giop_current_invocations->data;
+	}
+
+	return res;
+}
+
Index: src/orb/poa/poa.c
===================================================================
RCS file: /cvs/gnome/ORBit2/src/orb/poa/poa.c,v
retrieving revision 1.113
diff -u -b -r1.113 poa.c
--- src/orb/poa/poa.c	3 Nov 2003 22:56:56 -0000	1.113
+++ src/orb/poa/poa.c	6 Dec 2003 01:26:33 -0000
@@ -86,76 +86,78 @@
                       CORBA_Environment  *ev);
 
 static void
-ORBit_POA_invocation_stack_push (PortableServer_POA  poa,
+ORBit_POA_invocation_stack_push (GIOPThread         *tdata,
                                  ORBit_POAObject     pobj)
 {
- 	LINK_MUTEX_LOCK (poa->orb->lock);
-	poa->orb->current_invocations =
-		g_slist_prepend (poa->orb->current_invocations, pobj);
-	LINK_MUTEX_UNLOCK (poa->orb->lock);
+	giop_thread_invocation_stack_push (tdata, pobj);
 }
                                                                                 
 static void
-ORBit_POA_invocation_stack_pop (PortableServer_POA  poa,
+ORBit_POA_invocation_stack_pop (GIOPThread      *tdata,
                                 ORBit_POAObject pobj)
 {
-	/* FIXME, we know it is the top element to be removed */ 
-	LINK_MUTEX_LOCK (poa->orb->lock);
-	poa->orb->current_invocations =
-		g_slist_remove (poa->orb->current_invocations, pobj);
-	LINK_MUTEX_UNLOCK (poa->orb->lock);
+	ORBit_POAObject stack_head;
+	
+	stack_head = giop_thread_invocation_stack_pop (tdata);
+	
+	g_assert (stack_head == pobj);
+}
+
+
+static gboolean
+pobj_has_servant (gconstpointer stack_elem, gconstpointer user_data)
+{
+	const ORBit_POAObject             pobj    = stack_elem;
+	const PortableServer_ServantBase *servant = user_data; 
+
+        return (pobj->servant == servant);
 }
 
 static  PortableServer_ObjectId*
-ORBit_POA_invocation_stack_lookup_objid (PortableServer_POA          poa,
+ORBit_POA_invocation_stack_lookup_objid (GIOPThread                 *tdata,
+					 PortableServer_POA          poa,
                                          PortableServer_ServantBase *servant)
 {
 	PortableServer_ObjectId *objid = NULL;
-	GSList *l;
 
-	LINK_MUTEX_LOCK (poa->orb->lock);
-	for (l = poa->orb->current_invocations; l; l = l->next) {
-		ORBit_POAObject pobj = l->data;
+	ORBit_POAObject pobj = NULL;
 		
-		if (pobj->servant == servant)
+	pobj = giop_thread_invocation_stack_first_equal (tdata,
+							 pobj_has_servant,
+							 servant);
+	if (pobj) 
+		/* no lock as pobj on own stack & object_id is invariant */ 
 			objid = ORBit_sequence_CORBA_octet_dup (pobj->object_id);
-	}
-	LINK_MUTEX_UNLOCK (poa->orb->lock);
 
 	return objid;
 }
 
 static  CORBA_Object
-ORBit_POA_invocation_stack_lookup_objref (PortableServer_POA           poa,
+ORBit_POA_invocation_stack_lookup_objref (GIOPThread                 *tdata,
+                                          PortableServer_POA           poa,
                                           PortableServer_ServantBase *servant)
 {
 	CORBA_Object  result = CORBA_OBJECT_NIL; 
-	GSList *l;
 
-	LINK_MUTEX_LOCK (poa->orb->lock);
-	for (l = poa->orb->current_invocations; l; l = l->next) {
-		ORBit_POAObject pobj = l->data;
+	ORBit_POAObject pobj 
+		= giop_thread_invocation_stack_first_equal (tdata,
+							    pobj_has_servant, 
+							    servant);
 		
-		if (pobj->servant == servant)
+	if (pobj) 
+		/* ORBit_POA_obj_to_ref() must lock critical sections */ 
 			result = ORBit_POA_obj_to_ref (poa, pobj, NULL, NULL);
-	}
-	LINK_MUTEX_UNLOCK (poa->orb->lock);
 
 	return result;
 }
 
-/* FIXME, I would prefer if argument would be of type PortableServer_POA */ 
+
 static  ORBit_POAObject
-ORBit_POA_invocation_stack_peek (CORBA_ORB        orb)
+ORBit_POA_invocation_stack_peek (GIOPThread *tdata)
 {
-	ORBit_POAObject pobj = CORBA_OBJECT_NIL;
+	ORBit_POAObject pobj;
 
-	LINK_MUTEX_LOCK (orb->lock);
-	if (orb->current_invocations == NULL)
-		pobj = CORBA_OBJECT_NIL;
-	else
-		pobj = (ORBit_POAObject) orb->current_invocations->data;
-	LINK_MUTEX_UNLOCK (orb->lock);
+	pobj = (ORBit_POAObject) giop_thread_invocation_stack_peek (tdata);
 	
 	return pobj;
 }
@@ -200,7 +202,7 @@
 
 	g_assert (obj && obj->parent.interface->type == ORBIT_ROT_POACURRENT);
 	
-	if ( ! (pobj = ORBit_POA_invocation_stack_peek (obj->orb)))
+	if ( ! (pobj = ORBit_POA_invocation_stack_peek (giop_thread_self ())))
 		CORBA_exception_set_system
 			(ev, ex_PortableServer_Current_NoContext,
 			 CORBA_COMPLETED_NO);
@@ -1215,6 +1217,9 @@
 	ORBitSmallSkeleton                   small_skel = NULL;
 	gpointer                             imp = NULL;
 
+	/* OPTME, pass through "tdata" from giop module */
+	GIOPThread                          *tdata = giop_thread_self ();
+
 	if (poa) {
 		ORBit_RootObject_duplicate (poa);
 		POA_LOCK (poa);
@@ -1303,7 +1308,7 @@
 	}
 
 	pobj->use_cnt++;
-	ORBit_POA_invocation_stack_push (poa, pobj);
+	ORBit_POA_invocation_stack_push (tdata, pobj);
 	
 	klass = ORBIT_SERVANT_TO_CLASSINFO (pobj->servant);
 
@@ -1371,8 +1376,7 @@
 			break;
 		}
 
-	ORBit_POA_invocation_stack_pop (poa, pobj);
-
+	ORBit_POA_invocation_stack_pop (tdata, pobj);
 	pobj->use_cnt--;
 
 	if (pobj->life_flags & ORBit_LifeF_NeedPostInvoke)
@@ -2256,7 +2260,9 @@
 		 * but it would only add more code...
 		 */
 		
-		objid = ORBit_POA_invocation_stack_lookup_objid (poa, servant);
+		objid = ORBit_POA_invocation_stack_lookup_objid (giop_thread_self (), 
+								 poa, 
+								 servant);
 
 	}
 
@@ -2316,7 +2322,10 @@
 		 */
 
 		
-		result = ORBit_POA_invocation_stack_lookup_objref (poa, servant);
+		result = ORBit_POA_invocation_stack_lookup_objref (giop_thread_self (),
+								   poa, 
+								   
+								   servant);
 
 	}
 
@@ -2460,6 +2469,7 @@
 {
 	ORBit_class_assignment_lock = link_mutex_new ();
 	_ORBit_poa_manager_lock = link_mutex_new ();
+	
 	giop_thread_set_main_handler (ORBit_POAObject_invoke_incoming_request);
 }