[omniORB] omniORB 3/4 create_POA deadlocks

[Teemu Torma]

--------------Boundary-00=_3WBSLNR6MZ2FVBCINEZ8
Content-Type: text/plain;
  charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

In omniORB 3.0 (and 4, since the code is the same), create_POA deadlocks=20
if we are creating a poa and another one of the same name is being=20
destroyed.  create_POA tries to deal with this, but unfortunately the=20
locking conflicts between create_POA and do_destroy and will cause a=20
deadlock.  Also create_POA does nothing to prevent the old poa of the=20
same name being deleted whilst still referencing to it.

Please see the diffs against omniORB 3.0 to fix the deadlock problem. =20

Whilst testing my changes, I noticed another problem in destroying=20
poa's.  In destroy method, we check if pd_destroyed, throw an error,=20
else if pd_dying, wait for the poa to die. =20

Firstly, it does not seem reasonable to behave differently depending on=20
the current phase of the internal destruction, and secondly, this=20
causes problems if you destroy a poa under the root poa without waiting=20
for completion, and do orb shutdown.=20

In the latter case, root poa tries to delete the child poa which is=20
already being destroyed, and if the child destruction has hit the phase=20
when pd_destroyed =3D 1, an exception is thrown in a loop until the=20
destruction is complete (see do_destroy method).

Again, omniORB 4 looks identical to omniORB 3 regarding the destruction. =
=20
I did not do anything for that, since I have no idea what CORBA spec=20
says about the issue.

Teemu


--------------Boundary-00=_3WBSLNR6MZ2FVBCINEZ8
Content-Type: text/x-diff;
  charset="us-ascii";
  name="omniorb3-poa.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment; filename="omniorb3-poa.diff"

Index: src/lib/omniORB2/orbcore/poa.cc
===================================================================
RCS file: /trema/cvs/fk/tools/omniorb/src/lib/omniORB2/orbcore/Attic/poa.cc,v
retrieving revision 1.1.1.3
diff -c -u -r1.1.1.3 poa.cc
--- src/lib/omniORB2/orbcore/poa.cc	5 Jan 2001 13:38:49 -0000	1.1.1.3
+++ src/lib/omniORB2/orbcore/poa.cc	10 Oct 2002 21:12:47 -0000
@@ -415,47 +415,95 @@
 
   transfer_and_check_policies(policy, policies);
 
-  omni_tracedmutex_lock sync(poa_lock);
-  omni_tracedmutex_lock sync2(pd_lock);
+  poa_lock.lock ();
+  pd_lock.lock ();
+  CORBA::Boolean locked = 1;
+
+  try {
+    // If an adapter of the given name exists, but is in the
+    // process of being destroyed, we should block until
+    // that has completed, and then allow the new one to be
+    // created.  Ref CORBA 2.3 11.3.8.4
+
+    omniOrbPOA* p = find_child(adapter_name);
+    if( p ) {
+      // Lock it and keep one ref count for us.
+      p->pd_lock.lock ();
+      p->incrRefCount ();
+
+      if( p->pd_dying ) {
+
+	// Release other locks that we do not get deadlock
+	// during the possible destruction.
+	pd_lock.unlock ();
+	poa_lock.unlock ();
+	locked = 0;
+
+	if( omniORB::trace(10) ) {
+	  omniORB::logger l;
+	  l << "Waiting for destruction of POA(" << adapter_name << ").\n";
+	}
+
+	while( p->pd_destroyed != 2 )  p->pd_deathSignal.wait();
+
+	p->pd_lock.unlock();
+	p->decrRefCount();
+
+	if( omniORB::trace(10) ) {
+	  omniORB::logger l;
+	  l << "Continuing the creation of POA(" << adapter_name << ").\n";
+	}
+
+	pd_lock.lock();
+	poa_lock.lock();
+	locked = 1;
+
+	// Verify that it is gone.
+	OMNIORB_ASSERT(find_child(adapter_name) == 0);
+      }
+      else {
+	p->pd_lock.unlock();
+	p->decrRefCount();
+	throw AdapterAlreadyExists();
+      }
+    }
 
-  CHECK_NOT_DYING();
+    CHECK_NOT_DYING();
 
-  // If an adapter of the given name exists, but is in the
-  // process of being destroyed, we should block until
-  // that has completed, and then allow the new one to be
-  // created.  Ref CORBA 2.3 11.3.8.4
-
-  omniOrbPOA* p = find_child(adapter_name);
-  if( p ) {
-    omni_tracedmutex_lock sync(p->pd_lock);
-    if( p->pd_dying ) {
-      while( p->pd_destroyed != 2 )  p->pd_deathSignal.wait();
-      OMNIORB_ASSERT(find_child(adapter_name) == 0);
-    }
+    if( CORBA::is_nil(manager) )
+      manager = new omniOrbPOAManager();
     else
-      throw AdapterAlreadyExists();
-  }
+      PortableServer::POAManager::_duplicate(manager);
+
+    omniOrbPOA* poa = new omniOrbPOA(adapter_name,
+				     (omniOrbPOAManager*) manager,
+				     policy, this);
+
+    insert_child(poa);
+
+    pd_lock.unlock();
+    poa_lock.unlock();
+    locked = 0;
 
-  if( CORBA::is_nil(manager) )
-    manager = new omniOrbPOAManager();
-  else
-    PortableServer::POAManager::_duplicate(manager);
-
-  omniOrbPOA* poa = new omniOrbPOA(adapter_name, (omniOrbPOAManager*) manager,
-				   policy, this);
-
-  insert_child(poa);
-
-  poa->adapterActive();
-
-  // Need to ensure state is not changed from HOLDING if POA is
-  // being created by an adapter activator.  So in this case do
-  // not attach the new poa to the manager.
-  if( !is_adapteractivating_child(adapter_name) )
-    ((omniOrbPOAManager*) manager)->gain_poa(poa);
+    poa->adapterActive();
 
-  poa->incrRefCount();
-  return poa;
+    // Need to ensure state is not changed from HOLDING if POA is
+    // being created by an adapter activator.  So in this case do
+    // not attach the new poa to the manager.
+    if( !is_adapteractivating_child(adapter_name) )
+      ((omniOrbPOAManager*) manager)->gain_poa(poa);
+
+    poa->incrRefCount();
+
+    return poa;
+  }
+  catch (...) {
+    if( locked ) {
+      pd_lock.unlock();
+      poa_lock.unlock();
+    }
+    throw;
+  }
 }
 
 

--------------Boundary-00=_3WBSLNR6MZ2FVBCINEZ8--