Mon Dec 6 16:00:46 2004 Iliyan Jeliazkov <jeliazkov_i@ociweb.com>

  This change solves a problem with the bi-dir policy validators, in the
  context of multiple ORBs by eliminating cached, ORB-specific state in the
  TAO_BiDirGIOP_Loader singleton. Thus it also eliminates the possibility for a
  race condition between two threads that my be creating and destroying ORB
  instances simultaneously.

13 files changed, 277 insertions, 51 deletions

diff --git a/TAO/ChangeLog b/TAO/ChangeLog
index a78fe567445..e3da9e7271a 100644
--- a/TAO/ChangeLog
+++ b/TAO/ChangeLog
@@ -1,8 +1,66 @@
+Mon Dec  6 16:00:46 2004  Iliyan Jeliazkov  <jeliazkov_i@ociweb.com>
+
+          This change solves a problem with the bi-dir policy
+          validators, in the context of multiple ORBs by
+          eliminating cached, ORB-specific state in the
+          TAO_BiDirGIOP_Loader singleton. Thus it also
+          eliminates the possibility for a race condition
+          between two threads that my be creating and
+          destroying ORB instances simultaneously.
+
+        * tao/BiDir_Adapter.h:
+
+          Changed the method decl to support passing up any exceptions for
+          platforms that do not support them natively.
+
+        * tao/BiDir_GIOP/BiDirGIOP.h:
+        * tao/BiDir_GIOP/BiDirGIOP.cpp:
+
+          Removed state, represented by the bi-dir policy validator
+          instance, which was carried around in the Loader even
+          after the corresponding ORB was destructed. This caused
+          problems in processes with >1 ORB, where the new ORB was
+          re-using an validator instance, associated with another ORB.
+          In certain cases that validator was also already deleted.
+          Now a bi-dir policy validator gets instantiated and
+          registered during the call to
+          TAO_BiDirGIOP_Loader::load_policy_validators,
+          instead of piggy-backing on register_orb_initializer and
+          caching it.
+
+        * tao/ORB_Core.h:
+        * tao/ORB_Core.cpp:
+        * tao/Policy_Validator.h:
+        * tao/Policy_Validator.cpp:
+
+           Provided accessor for the ORB reference to enable the
+           bi-dir policy validator instantiation in
+           TAO_BiDirGIOP_Loader::load_policy_validators
+
+        * tao/PortableServer/POA_Policy_Set.cpp:
+
+          Changed the invocation of the method to pass up any exceptions on
+          platforms that do not support them natively.
+
+        * tests/BiDirectional_MultipleORB/README:
+        * tests/BiDirectional_MultipleORB/destroy.mpc:
+        * tests/BiDirectional_MultipleORB/destroy.cpp:
+        * tests/BiDirectional_MultipleORB/run_test.pl:
+
+          This is a test that creates a bidir GIOP policy
+          for a POA and then attempts to repeat this, after first
+          destructing and re-creating the ORB. This used to fail
+          by breaking an assertion, because the second ORB was
+          tryig to register a policy validator object instance,
+          which lingered since the time the first ORB was created.
+          In some cases it broke with SEGV, as when trying to access
+          a previously deleted bi-dir policy validator.
+          
 Mon Dec  6 13:14:32 2004  Jeff Parsons <j.parsons@vanderbilt.edu>
 
         * orbsvcs/IFR_Service/IFR_Service.mpc:
-        
-          Removed inherited .mpb files iortable, svc_utils, and 
+
+          Removed inherited .mpb files iortable, svc_utils, and
           typecodefactory, since they are already pulled in by
           ifrservice. Also added empty IDL_Files block. Because
           of all the inherited .mpb files, any IDL file in the
diff --git a/TAO/tao/BiDir_Adapter.h b/TAO/tao/BiDir_Adapter.h
index f28ca8f2107..53cdcaafd96 100644
--- a/TAO/tao/BiDir_Adapter.h
+++ b/TAO/tao/BiDir_Adapter.h
@@ -52,7 +52,7 @@ public:
                         ACE_ENV_ARG_DECL)
     ACE_THROW_SPEC ((CORBA::SystemException)) = 0;
 
-  virtual void load_policy_validators (TAO_Policy_Validator &validator)
+  virtual void load_policy_validators (TAO_Policy_Validator &validator ACE_ENV_ARG_DECL)
     ACE_THROW_SPEC ((CORBA::SystemException)) = 0;
 
 };
diff --git a/TAO/tao/BiDir_GIOP/BiDirGIOP.cpp b/TAO/tao/BiDir_GIOP/BiDirGIOP.cpp
index 945b3e00b14..f962c57ed50 100644
--- a/TAO/tao/BiDir_GIOP/BiDirGIOP.cpp
+++ b/TAO/tao/BiDir_GIOP/BiDirGIOP.cpp
@@ -11,18 +11,14 @@ ACE_RCSID (BiDir_GIOP,
 
 
 // Set the flag to zero to start with
-int TAO_BiDirGIOP_Loader::validator_loaded_ = 0;
 int TAO_BiDirGIOP_Loader::is_activated_ = 0;
 
 TAO_BiDirGIOP_Loader::TAO_BiDirGIOP_Loader (void)
-  : validator_ (0)
 {
 }
 
 TAO_BiDirGIOP_Loader::~TAO_BiDirGIOP_Loader (void)
 {
-  /*  if (this->validator_)
-      delete this->validator_;*/
 }
 
 int
@@ -32,6 +28,8 @@ TAO_BiDirGIOP_Loader::activate (CORBA::ORB_ptr orb,
                                 ACE_ENV_ARG_DECL)
   ACE_THROW_SPEC ((CORBA::SystemException))
 {
+  ACE_UNUSED_ARG (orb);
+
   if (TAO_BiDirGIOP_Loader::is_activated_ == 0 && TAO_DEF_GIOP_MINOR >= 2)
     {
       PortableInterceptor::ORBInitializer_ptr tmp_orb_initializer =
@@ -55,19 +53,6 @@ TAO_BiDirGIOP_Loader::activate (CORBA::ORB_ptr orb,
         ACE_ENV_ARG_PARAMETER);
       ACE_CHECK_RETURN (-1);
 
-      TAO_ORB_Core *orb_core =
-        orb->orb_core ();
-
-      ACE_NEW_THROW_EX (this->validator_,
-                        TAO_BiDirPolicy_Validator (*orb_core),
-                        CORBA::NO_MEMORY (
-                            CORBA::SystemException::_tao_minor_code (
-                                TAO_DEFAULT_MINOR_CODE,
-                                ENOMEM),
-                            CORBA::COMPLETED_NO));
-      ACE_CHECK_RETURN (-1);
-
-
       TAO_BiDirGIOP_Loader::is_activated_ = 1;
     }
 
@@ -78,9 +63,27 @@ void
 TAO_BiDirGIOP_Loader::load_policy_validators (TAO_Policy_Validator &val)
   ACE_THROW_SPEC ((CORBA::SystemException))
 {
-  // Add our validator
-  if (!validator_loaded_)
-    val.add_validator (this->validator_);
+  // Is this true? Does the GIOP protocol version matter here?
+  if (TAO_DEF_GIOP_MINOR < 2)
+    return;
+
+  TAO_BiDirPolicy_Validator *validator = 0;
+  ACE_NEW_THROW_EX (validator,
+                    TAO_BiDirPolicy_Validator (val.orb_core ()),
+                    CORBA::NO_MEMORY (
+                        CORBA::SystemException::_tao_minor_code (
+                            TAO_DEFAULT_MINOR_CODE,
+                            ENOMEM),
+                        CORBA::COMPLETED_NO));
+  ACE_CHECK;
+
+  // We may be adding another TAO_BiDirPolicy_Validator instance for the
+  // same ORB (different POA). In cases where huge numbers of bi-directional POA instances
+  // are created, having a validator instance per POA may introduce additional delays in 
+  // policy validation and hence, the overal policy creation time. Since this is out of the
+  // critical invocation processing path, I plan to keep the design simple and not try to
+  // avoid an ineficiency of such small proportions.
+  val.add_validator (validator);
 }
 
 int
@@ -89,24 +92,11 @@ TAO_BiDirGIOP_Loader::Initializer (void)
   return ACE_Service_Config::process_directive (ace_svc_desc_TAO_BiDirGIOP_Loader);
 }
 
-/*static */ int
-TAO_BiDirGIOP_Loader::validator_loaded (void)
-{
-  return validator_loaded_;
-}
-
-/*static */ void
-TAO_BiDirGIOP_Loader::validator_loaded (int f)
-{
-  // @@ TODO: Do we need synchronization?
-  validator_loaded_ = f;
-}
-
-
 ACE_STATIC_SVC_DEFINE (TAO_BiDirGIOP_Loader,
                        ACE_TEXT ("BiDirGIOP_Loader"),
                        ACE_SVC_OBJ_T,
                        &ACE_SVC_NAME (TAO_BiDirGIOP_Loader),
                        ACE_Service_Type::DELETE_THIS | ACE_Service_Type::DELETE_OBJ,
                        0)
+
 ACE_FACTORY_DEFINE (TAO_BiDirGIOP, TAO_BiDirGIOP_Loader)
diff --git a/TAO/tao/BiDir_GIOP/BiDirGIOP.h b/TAO/tao/BiDir_GIOP/BiDirGIOP.h
index 8fa5d711a18..2d5fff19ff7 100644
--- a/TAO/tao/BiDir_GIOP/BiDirGIOP.h
+++ b/TAO/tao/BiDir_GIOP/BiDirGIOP.h
@@ -50,23 +50,15 @@ public:
                         ACE_ENV_ARG_DECL)
     ACE_THROW_SPEC ((CORBA::SystemException));
 
-  virtual void load_policy_validators (TAO_Policy_Validator &validator)
+  virtual void load_policy_validators (TAO_Policy_Validator &validator
+                                       ACE_ENV_ARG_DECL)
     ACE_THROW_SPEC ((CORBA::SystemException));
 
   /// Used to force the initialization of the ORB code.
   static int Initializer (void);
 
-  /// Accessor for the <validator_loaded_> flag
-  static int validator_loaded (void);
-  static void validator_loaded (int f);
 private:
 
-  /// Our policy validator
-  TAO_BiDirPolicy_Validator *validator_;
-
-  /// Flag to indicate whether validator has been loaded
-  static int validator_loaded_;
-
   /// Flag to indicate whether the BiDirGIOP library has been
   /// activated.
   static int is_activated_;
diff --git a/TAO/tao/ORB_Core.cpp b/TAO/tao/ORB_Core.cpp
index 46be618d904..2512e95d2c5 100644
--- a/TAO/tao/ORB_Core.cpp
+++ b/TAO/tao/ORB_Core.cpp
@@ -1661,11 +1661,12 @@ TAO_ORB_Core::create_stub_object (TAO_MProfile &mprofile,
 }
 
 void
-TAO_ORB_Core::load_policy_validators (TAO_Policy_Validator &validator)
+TAO_ORB_Core::load_policy_validators (TAO_Policy_Validator &validator
+                                      ACE_ENV_ARG_DECL)
 {
   // Call the BiDir library if it has been loaded
   if (this->bidir_adapter_)
-    this->bidir_adapter_->load_policy_validators (validator);
+    this->bidir_adapter_->load_policy_validators (validator ACE_ENV_ARG_PARAMETER);
 }
 
 CORBA::Object_ptr
diff --git a/TAO/tao/ORB_Core.h b/TAO/tao/ORB_Core.h
index 54330c25851..3fa71e145a6 100644
--- a/TAO/tao/ORB_Core.h
+++ b/TAO/tao/ORB_Core.h
@@ -963,7 +963,8 @@ public:
 
   /// Call the libraries to handover the validators if they havent
   /// registered yet with the list of validators.
-  void load_policy_validators (TAO_Policy_Validator &validator);
+  void load_policy_validators (TAO_Policy_Validator &validator
+                               ACE_ENV_ARG_DECL);
 
   /// Return the flushing strategy
   /**
diff --git a/TAO/tao/Policy_Validator.cpp b/TAO/tao/Policy_Validator.cpp
index 4e78e8223db..cd51a8a1426 100644
--- a/TAO/tao/Policy_Validator.cpp
+++ b/TAO/tao/Policy_Validator.cpp
@@ -25,6 +25,12 @@ TAO_Policy_Validator::~TAO_Policy_Validator (void)
     }
 }
 
+TAO_ORB_Core & 
+TAO_Policy_Validator::orb_core() const
+{ 
+  return this->orb_core_;
+}
+
 void
 TAO_Policy_Validator::add_validator (TAO_Policy_Validator *validator)
 {
diff --git a/TAO/tao/Policy_Validator.h b/TAO/tao/Policy_Validator.h
index 52f91e4847f..2c1afd96800 100644
--- a/TAO/tao/Policy_Validator.h
+++ b/TAO/tao/Policy_Validator.h
@@ -89,6 +89,11 @@ public:
   void add_validator (TAO_Policy_Validator *validator);
 
 
+ /**
+  * Accessor for the stored ORB core reference
+  */
+ TAO_ORB_Core & orb_core() const;
+
 protected:
   virtual void validate_impl (TAO_Policy_Set &policies
                               ACE_ENV_ARG_DECL) = 0;
diff --git a/TAO/tao/PortableServer/POA_Policy_Set.cpp b/TAO/tao/PortableServer/POA_Policy_Set.cpp
index 9907212b534..0c37b9d5535 100644
--- a/TAO/tao/PortableServer/POA_Policy_Set.cpp
+++ b/TAO/tao/PortableServer/POA_Policy_Set.cpp
@@ -60,7 +60,7 @@ TAO_POA_Policy_Set::validate_policies (TAO_Policy_Validator &validator,
 {
   // Just give a last chance for all the unloaded validators in other
   // libraries to be registered
-  orb_core.load_policy_validators (validator);
+  orb_core.load_policy_validators (validator ACE_ENV_ARG_PARAMETER);
 
   // Validate that all of the specified policies make sense.
   validator.validate (this->impl_  ACE_ENV_ARG_PARAMETER);
diff --git a/TAO/tests/BiDirectional_MultipleORB/README b/TAO/tests/BiDirectional_MultipleORB/README
new file mode 100644
index 00000000000..2d26bcc817e
--- /dev/null
+++ b/TAO/tests/BiDirectional_MultipleORB/README
@@ -0,0 +1,23 @@
+# $Id$
+
+This is a test that creates a birectional GIOP policy for a POA and then
+attempts to repeat this, after destructing and re-creating the ORB. This used to
+fail by breaking an assertion, because the second ORB was tryig to register a policy 
+validator object instance, which lingered since the time the first ORB was created. 
+
+The validators are chained in a linked list, and any attempt to register a new one,
+which already points to another, is considered an error:
+
+ACE_ASSERT: (24189|4143901376) file Policy_Validator.cpp, line 28 assertion failed for 'validator->next_ == 0'.Aborting...
+
+In other instances it broke with SEGV, when trying to access an previously deleted 
+bi-dir policy validator.
+
+For additional detail reffer to RT4667 and RT4672.
+
+Start the test like this:
+
+$ destroy
+
+It should complete ok
+
diff --git a/TAO/tests/BiDirectional_MultipleORB/destroy.cpp b/TAO/tests/BiDirectional_MultipleORB/destroy.cpp
new file mode 100644
index 00000000000..8cdf84b2401
--- /dev/null
+++ b/TAO/tests/BiDirectional_MultipleORB/destroy.cpp
@@ -0,0 +1,123 @@
+// $Id$
+
+//========================================================================
+//
+// = LIBRARY
+//     TAO/tests/BiDir_Multiple_ORB
+//
+// = FILENAME
+//     destroy.cpp
+//
+// = DESCRIPTION
+//     Modified ORB destruction test.
+//
+// = AUTHOR
+//     Andrew Schnable <Andrew.Schnable@veritas.com>
+//     Iliyan Jeliazkov <jeliazkov_i@ociweb.com>
+//
+//=========================================================================
+
+#include "tao/corba.h"
+#include "tao/PortableServer/PortableServer.h"
+#include "tao/BiDir_GIOP/BiDirGIOP.h"
+
+ACE_RCSID(BiDir_Multiple_ORB, destroy, "$Id$")
+
+int
+test_with_bidir_poa (int argc,
+                     char **argv,
+                     const char *orb_name,
+                     int destroy_orb)
+{
+  ACE_DECLARE_NEW_CORBA_ENV;
+
+  ACE_TRY
+    {
+      CORBA::ORB_var orb =
+        CORBA::ORB_init (argc, argv, orb_name ACE_ENV_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+      CORBA::Object_var obj =
+        orb->resolve_initial_references ("RootPOA"
+                                         ACE_ENV_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+      PortableServer::POA_var root_poa =
+        PortableServer::POA::_narrow (obj.in () ACE_ENV_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+      PortableServer::POAManager_var poa_manager =
+        root_poa->the_POAManager (ACE_ENV_SINGLE_ARG_PARAMETER);
+
+      ACE_TRY_CHECK;
+
+      CORBA::PolicyList policies (1);
+      policies.length (1);
+
+      CORBA::Any pol;
+      pol <<= BiDirPolicy::BOTH;
+      policies[0] =
+        orb->create_policy (BiDirPolicy::BIDIRECTIONAL_POLICY_TYPE,
+                            pol
+                            ACE_ENV_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+      // Create POA as child of RootPOA with the above policies.  This POA
+      // will receive request in the same connection in which it sent
+      // the request
+      PortableServer::POA_var child_poa =
+        root_poa->create_POA ("childPOA",
+                              poa_manager.in (),
+                              policies
+                              ACE_ENV_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+      // Creation of childPOA is over. Destroy the Policy objects.
+      for (CORBA::ULong i = 0;
+           i < policies.length ();
+           ++i)
+        {
+          policies[i]->destroy (ACE_ENV_SINGLE_ARG_PARAMETER);
+          ACE_TRY_CHECK;
+        }
+
+      poa_manager->activate (ACE_ENV_SINGLE_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+
+      root_poa->destroy (1, 1 ACE_ENV_ARG_PARAMETER);
+      ACE_TRY_CHECK;
+
+      if (destroy_orb)
+        {
+          orb->destroy (ACE_ENV_SINGLE_ARG_PARAMETER);
+          ACE_TRY_CHECK;
+        }
+    }
+  ACE_CATCHANY
+    {
+      ACE_PRINT_EXCEPTION (ACE_ANY_EXCEPTION,
+                           "Exception raised");
+      ACE_CHECK_RETURN (-1);
+    }
+  ACE_ENDTRY;
+
+  return 0;
+}
+
+int
+main (int argc, char **argv)
+{
+  int result = 0;
+  
+  for (int i=0; i<10; i++)
+  {
+    result = test_with_bidir_poa (argc, argv, "poa_1", 1);
+    ACE_ASSERT (result == 0);
+
+    result = test_with_bidir_poa (argc, argv, "poa_2", 1);
+    ACE_ASSERT (result == 0);
+  }
+  ACE_DEBUG ((LM_DEBUG, "Completed OK\n"));
+  return result;
+}
diff --git a/TAO/tests/BiDirectional_MultipleORB/destroy.mpc b/TAO/tests/BiDirectional_MultipleORB/destroy.mpc
new file mode 100644
index 00000000000..1886f7e76cc
--- /dev/null
+++ b/TAO/tests/BiDirectional_MultipleORB/destroy.mpc
@@ -0,0 +1,6 @@
+project: taoexe, portableserver, bidir_giop {
+  Source_Files {
+    destroy.cpp
+  }
+}
+
diff --git a/TAO/tests/BiDirectional_MultipleORB/run_test.pl b/TAO/tests/BiDirectional_MultipleORB/run_test.pl
new file mode 100755
index 00000000000..cf56e4473d7
--- /dev/null
+++ b/TAO/tests/BiDirectional_MultipleORB/run_test.pl
@@ -0,0 +1,21 @@
+eval '(exit $?0)' && eval 'exec perl -S $0 ${1+"$@"}'
+    & eval 'exec perl -S $0 $argv:q'
+    if 0;
+
+# $Id$
+# -*- perl -*-
+
+use lib '../../../bin';
+use PerlACE::Run_Test;
+
+$status = 0;
+$CL = new PerlACE::Process ("destroy");
+
+$client = $CL->SpawnWaitKill (10);
+
+if ($client != 0) {
+    print STDERR "ERROR: client returned $client\n";
+    $status = 1;
+}
+
+exit $status;